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 2 commits
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
44 changes: 36 additions & 8 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 @@ -55,19 +55,21 @@ public class QuotaRetriever implements Closeable, Iterable<QuotaSettings> {
/**
* Should QutoaRetriever manage the state of the connection, or leave it be.
*/
private boolean isManagedConnection = false;
private final boolean isManagedConnection;

QuotaRetriever() {
QuotaRetriever(final Connection conn, final Scan scan) throws IOException {
isManagedConnection = false;
init(conn, scan);
}

void init(final Configuration conf, final Scan scan) throws IOException {
QuotaRetriever(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;
isManagedConnection = true;
init(ConnectionFactory.createConnection(conf), scan);
}

void init(final Connection conn, final Scan scan) throws IOException {
private void init(final Connection conn, final Scan scan) throws IOException {
this.connection = Objects.requireNonNull(conn);
this.table = this.connection.getTable(QuotaTableUtil.QUOTA_TABLE_NAME);
try {
Expand Down Expand Up @@ -159,23 +161,49 @@ public void remove() {
* @param conf Configuration object to use.
* @return the QuotaRetriever
* @throws IOException if a remote or network exception occurs
* @deprecated Since 3.0.0, will be removed in 4.0.0. Use {@link #open(Connection)} instead.
*/
@Deprecated
public static QuotaRetriever open(final Configuration conf) throws IOException {
return open(conf, null);
}

/**
* Open a QuotaRetriever with no filter, all the quota settings will be returned.
* @param conn Connection object to use.
* @return the QuotaRetriever
* @throws IOException if a remote or network exception occurs
*/
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.
* @param filter the QuotaFilter
* @return the QuotaRetriever
* @throws IOException if a remote or network exception occurs
* @deprecated Since 3.0.0, will be removed in 4.0.0. Use {@link #open(Connection, QuotaFilter)}
* instead.
*/
@Deprecated
public static QuotaRetriever open(final Configuration conf, final QuotaFilter filter)
throws IOException {
Scan scan = QuotaTableUtil.makeScan(filter);
QuotaRetriever scanner = new QuotaRetriever();
scanner.init(conf, scan);
return scanner;
return new QuotaRetriever(conf, scan);
}

/**
* Open a QuotaRetriever with the specified filter.
* @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 Connection conn, final QuotaFilter filter)
throws IOException {
Scan scan = QuotaTableUtil.makeScan(filter);
return new QuotaRetriever(conn, scan);
}
}
3 changes: 1 addition & 2 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaObserverChore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,8 +475,7 @@ void pruneOldRegionReports() {
TablesWithQuotas fetchAllTablesWithQuotasDefined() throws IOException {
final Scan scan = QuotaTableUtil.makeScan(null);
final TablesWithQuotas tablesWithQuotas = new TablesWithQuotas(conn, conf);
try (final QuotaRetriever scanner = new QuotaRetriever()) {
scanner.init(conn, scan);
try (final QuotaRetriever scanner = new QuotaRetriever(conn, scan)) {
for (QuotaSettings quotaSettings : scanner) {
// Only one of namespace and tablename should be 'null'
final String namespace = quotaSettings.getNamespace();
Expand Down
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