Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-28716: Users of QuotaRetriever should pass an existing connection (master) #6065

Merged
merged 4 commits into from
Jul 19, 2024
Merged

HBASE-28716: Users of QuotaRetriever should pass an existing connection (master) #6065

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a247c98
Use existing Connection in all cases with QuotaRetriever
charlesconnell Jul 9, 2024
dd42a14
Bring back public methods
charlesconnell Jul 17, 2024
80dd227
Expose QuotaRetriever constructor, use it via try-with-resources
charlesconnell Jul 18, 2024
6caf25e
silly mistake
charlesconnell Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 8 additions & 27 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/QuotaRetriever.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,7 @@
import java.util.Iterator;
import java.util.Objects;
import java.util.Queue;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.client.Connection;
import org.apache.hadoop.hbase.client.ConnectionFactory;
import org.apache.hadoop.hbase.client.Result;
import org.apache.hadoop.hbase.client.ResultScanner;
import org.apache.hadoop.hbase.client.Scan;
Expand All @@ -52,21 +50,9 @@ public class QuotaRetriever implements Closeable, Iterable<QuotaSettings> {
private Connection connection;
private Table table;

/**
* Should QutoaRetriever manage the state of the connection, or leave it be.
*/
private boolean isManagedConnection = false;

QuotaRetriever() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a weird API. This class is decorated as IA.Public but it's only created via these static factory method? Any what's with using the default constructor + an init method -- what happened to RAII ?’

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are non-public methods of an IA.Public class required to go through a deprecation cycle, or only public methods?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only public methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then this PR is complying with the deprecation policy now

}

void init(final Configuration conf, final Scan scan) throws IOException {
// Set this before creating the connection and passing it down to make sure
// it's cleaned up if we fail to construct the Scanner.
this.isManagedConnection = true;
init(ConnectionFactory.createConnection(conf), scan);
}

void init(final Connection conn, final Scan scan) throws IOException {
this.connection = Objects.requireNonNull(conn);
this.table = this.connection.getTable(QuotaTableUtil.QUOTA_TABLE_NAME);
Expand All @@ -88,13 +74,8 @@ public void close() throws IOException {
this.table.close();
this.table = null;
}
// Null out the connection on close() even if we didn't explicitly close it
// Null out the connection on close() even though we don't explicitly close it
// to maintain typical semantics.
if (isManagedConnection) {
if (this.connection != null) {
this.connection.close();
}
}
this.connection = null;
}

Expand Down Expand Up @@ -156,26 +137,26 @@ public void remove() {

/**
* Open a QuotaRetriever with no filter, all the quota settings will be returned.
* @param conf Configuration object to use.
* @param conn Connection object to use.
* @return the QuotaRetriever
* @throws IOException if a remote or network exception occurs
*/
public static QuotaRetriever open(final Configuration conf) throws IOException {
return open(conf, null);
public static QuotaRetriever open(final Connection conn) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is IA.Public, you cannot make these blanket changes in one pass. You'll need to observe a deprecation cycle through a major release in order to make breaking changes to the public API.

We document this in depth over on https://hbase.apache.org/book.html#hbase.versioning

If we're going through the trouble to make breaking changes, let's push RAII and do away with the parameterless constructor + init method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for the deprecation cycle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going through the trouble to make breaking changes, let's push RAII and do away with the parameterless constructor + init method.

Since we're opening this worm-can, how about we get rid of these static constructor methods and use constructors like a normal object?

return open(conn, null);
}

/**
* Open a QuotaRetriever with the specified filter.
* @param conf Configuration object to use.
* Open a QuotaRetriever with the specified filter using an existing Connection
* @param conn Connection object to use.
* @param filter the QuotaFilter
* @return the QuotaRetriever
* @throws IOException if a remote or network exception occurs
*/
public static QuotaRetriever open(final Configuration conf, final QuotaFilter filter)
public static QuotaRetriever open(final Connection conn, final QuotaFilter filter)
throws IOException {
Scan scan = QuotaTableUtil.makeScan(filter);
QuotaRetriever scanner = new QuotaRetriever();
scanner.init(conf, scan);
scanner.init(conn, scan);
return scanner;
}
}
2 changes: 1 addition & 1 deletion hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/SnapshotQuotaObserverChore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ Multimap<TableName, String> getSnapshotsToComputeSize() throws IOException {
filter.addTypeFilter(QuotaType.SPACE);
try (Admin admin = conn.getAdmin()) {
// Pull all of the tables that have quotas (direct, or from namespace)
for (QuotaSettings qs : QuotaRetriever.open(conf, filter)) {
for (QuotaSettings qs : QuotaRetriever.open(conn, filter)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind also promoting uses of the QuotaRetriever object up to try-with-resource blocks? In this particular case, it looks like we never close the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you got it!

if (qs.getQuotaType() == QuotaType.SPACE) {
String ns = qs.getNamespace();
TableName tn = qs.getTableName();
Expand Down
3 changes: 1 addition & 2 deletions hbase-server/src/main/resources/hbase-webapps/master/quotas.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
%>
<%
HMaster master = (HMaster) getServletContext().getAttribute(HMaster.MASTER);
Configuration conf = master.getConfiguration();
pageContext.setAttribute("pageTitle", "HBase Master Quotas: " + master.getServerName());
List<ThrottleSettings> regionServerThrottles = new ArrayList<>();
List<ThrottleSettings> namespaceThrottles = new ArrayList<>();
Expand All @@ -39,7 +38,7 @@
boolean exceedThrottleQuotaEnabled = false;
if (quotaManager != null) {
exceedThrottleQuotaEnabled = quotaManager.isExceedThrottleQuotaEnabled();
try (QuotaRetriever scanner = QuotaRetriever.open(conf, null)) {
try (QuotaRetriever scanner = QuotaRetriever.open(master.getConnection(), null)) {
for (QuotaSettings quota : scanner) {
if (quota instanceof ThrottleSettings) {
ThrottleSettings throttle = (ThrottleSettings) quota;
Expand Down
8 changes: 4 additions & 4 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/SpaceQuotaHelperForTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ static void updateConfigForQuotas(Configuration conf) {
* Returns the number of quotas defined in the HBase quota table.
*/
long listNumDefinedQuotas(Connection conn) throws IOException {
QuotaRetriever scanner = QuotaRetriever.open(conn.getConfiguration());
QuotaRetriever scanner = QuotaRetriever.open(conn);
try {
return Iterables.size(scanner);
} finally {
Expand Down Expand Up @@ -353,7 +353,7 @@ void removeAllQuotas(Connection conn) throws IOException {
waitForQuotaTable(conn);
} else {
// Or, clean up any quotas from previous test runs.
QuotaRetriever scanner = QuotaRetriever.open(conn.getConfiguration());
QuotaRetriever scanner = QuotaRetriever.open(conn);
try {
for (QuotaSettings quotaSettings : scanner) {
final String namespace = quotaSettings.getNamespace();
Expand All @@ -379,8 +379,8 @@ void removeAllQuotas(Connection conn) throws IOException {
}

QuotaSettings getTableSpaceQuota(Connection conn, TableName tn) throws IOException {
try (QuotaRetriever scanner = QuotaRetriever.open(conn.getConfiguration(),
new QuotaFilter().setTableFilter(tn.getNameAsString()))) {
try (QuotaRetriever scanner =
QuotaRetriever.open(conn, new QuotaFilter().setTableFilter(tn.getNameAsString()))) {
for (QuotaSettings setting : scanner) {
if (setting.getTableName().equals(tn) && setting.getQuotaType() == QuotaType.SPACE) {
return setting;
Expand Down
4 changes: 2 additions & 2 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotasObserver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ public boolean namespaceExists(String ns) throws IOException {
}

public int getNumSpaceQuotas() throws Exception {
QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration());
QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConnection());
int numSpaceQuotas = 0;
for (QuotaSettings quotaSettings : scanner) {
if (quotaSettings.getQuotaType() == QuotaType.SPACE) {
Expand All @@ -338,7 +338,7 @@ public int getNumSpaceQuotas() throws Exception {
}

public int getThrottleQuotas() throws Exception {
QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration());
QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConnection());
int throttleQuotas = 0;
for (QuotaSettings quotaSettings : scanner) {
if (quotaSettings.getQuotaType() == QuotaType.THROTTLE) {
Expand Down
22 changes: 11 additions & 11 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public void testThrottleType() throws Exception {
QuotaSettingsFactory.throttleUser(userName, ThrottleType.WRITE_NUMBER, 12, TimeUnit.MINUTES));
admin.setQuota(QuotaSettingsFactory.bypassGlobals(userName, true));

try (QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration())) {
try (QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConnection())) {
int countThrottle = 0;
int countGlobalBypass = 0;
for (QuotaSettings settings : scanner) {
Expand Down Expand Up @@ -169,7 +169,7 @@ public void testSimpleScan() throws Exception {
TimeUnit.MINUTES));
admin.setQuota(QuotaSettingsFactory.bypassGlobals(userName, true));

try (QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration())) {
try (QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConnection())) {
int countThrottle = 0;
int countGlobalBypass = 0;
for (QuotaSettings settings : scanner) {
Expand Down Expand Up @@ -345,7 +345,7 @@ public void testSetGetRemoveSpaceQuota() throws Exception {
}

// Verify we can retrieve it via the QuotaRetriever API
QuotaRetriever scanner = QuotaRetriever.open(admin.getConfiguration());
QuotaRetriever scanner = QuotaRetriever.open(admin.getConnection());
try {
assertSpaceQuota(sizeLimit, violationPolicy, Iterables.getOnlyElement(scanner));
} finally {
Expand All @@ -367,7 +367,7 @@ public void testSetGetRemoveSpaceQuota() throws Exception {
}

// Verify that we can also not fetch it via the API
scanner = QuotaRetriever.open(admin.getConfiguration());
scanner = QuotaRetriever.open(admin.getConnection());
try {
assertNull("Did not expect to find a quota entry", scanner.next());
} finally {
Expand Down Expand Up @@ -399,7 +399,7 @@ public void testSetModifyRemoveSpaceQuota() throws Exception {
}

// Verify we can retrieve it via the QuotaRetriever API
QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConfiguration());
QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConnection());
try {
assertSpaceQuota(originalSizeLimit, violationPolicy, Iterables.getOnlyElement(quotaScanner));
} finally {
Expand Down Expand Up @@ -427,7 +427,7 @@ public void testSetModifyRemoveSpaceQuota() throws Exception {
}

// Verify we can retrieve the new quota via the QuotaRetriever API
quotaScanner = QuotaRetriever.open(admin.getConfiguration());
quotaScanner = QuotaRetriever.open(admin.getConnection());
try {
assertSpaceQuota(newSizeLimit, newViolationPolicy, Iterables.getOnlyElement(quotaScanner));
} finally {
Expand All @@ -449,7 +449,7 @@ public void testSetModifyRemoveSpaceQuota() throws Exception {
}

// Verify that we can also not fetch it via the API
quotaScanner = QuotaRetriever.open(admin.getConfiguration());
quotaScanner = QuotaRetriever.open(admin.getConnection());
try {
assertNull("Did not expect to find a quota entry", quotaScanner.next());
} finally {
Expand Down Expand Up @@ -549,7 +549,7 @@ public void testSetAndRemoveRegionServerQuota() throws Exception {
admin.setQuota(QuotaSettingsFactory.throttleRegionServer(regionServer, ThrottleType.READ_NUMBER,
30, TimeUnit.SECONDS));
int count = 0;
QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration(), rsFilter);
QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConnection(), rsFilter);
try {
for (QuotaSettings settings : scanner) {
assertTrue(settings.getQuotaType() == QuotaType.THROTTLE);
Expand Down Expand Up @@ -733,14 +733,14 @@ private void verifyRecordNotPresentInQuotaTable() throws Exception {
private void verifyFetchableViaAPI(Admin admin, ThrottleType type, long limit, TimeUnit tu)
throws Exception {
// Verify we can retrieve the new quota via the QuotaRetriever API
try (QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConfiguration())) {
try (QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConnection())) {
assertRPCQuota(type, limit, tu, Iterables.getOnlyElement(quotaScanner));
}
}

private void verifyNotFetchableViaAPI(Admin admin) throws Exception {
// Verify that we can also not fetch it via the API
try (QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConfiguration())) {
try (QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConnection())) {
assertNull("Did not expect to find a quota entry", quotaScanner.next());
}
}
Expand Down Expand Up @@ -830,7 +830,7 @@ private void assertSpaceQuota(long sizeLimit, SpaceViolationPolicy violationPoli
}

private int countResults(final QuotaFilter filter) throws Exception {
QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration(), filter);
QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConnection(), filter);
try {
int count = 0;
for (QuotaSettings settings : scanner) {
Expand Down