Skip to content

HBASE-28716: Users of QuotaRetriever should pass an existing connection (branch-2) #6054

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

Closed
charlesconnell wants to merge 2 commits into from
Closed

HBASE-28716: Users of QuotaRetriever should pass an existing connection (branch-2) #6054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d12623b
HBaseAdmin should pass QuotaRetriever an existing connection
charlesconnell Jun 30, 2024
29a462e
Use existing Connection in all cases with QuotaRetriever
charlesconnell Jul 9, 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
4 changes: 2 additions & 2 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3153,13 +3153,13 @@ protected Void rpcCall() throws Exception {

@Override
public QuotaRetriever getQuotaRetriever(final QuotaFilter filter) throws IOException {
return QuotaRetriever.open(conf, filter);
return QuotaRetriever.open(connection, filter);
}

@Override
public List<QuotaSettings> getQuota(QuotaFilter filter) throws IOException {
List<QuotaSettings> quotas = new ArrayList<>();
try (QuotaRetriever retriever = QuotaRetriever.open(conf, filter)) {
try (QuotaRetriever retriever = QuotaRetriever.open(connection, filter)) {
Iterator<QuotaSettings> iterator = retriever.iterator();
Copy link
Contributor

@pankaj72981 pankaj72981 Jul 9, 2024

Choose a reason for hiding this comment

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

Changes looks good to me.

However similar optimization can be done in master branch as well, below API is used multiple places
org.apache.hadoop.hbase.quotas.QuotaRetriever#open(org.apache.hadoop.conf.Configuration, org.apache.hadoop.hbase.quotas.QuotaFilter)

We can reuse the existing connection instead of creating new one always.

Copy link
Contributor Author

@charlesconnell charlesconnell Jul 9, 2024

Choose a reason for hiding this comment

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

Okay, I'm happy to expand the scope of this ticket.

while (iterator.hasNext()) {
quotas.add(iterator.next());
Expand Down
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() {
}

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 {
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)) {
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 @@ -124,7 +124,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 @@ -436,7 +436,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 @@ -462,8 +462,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 @@ -325,7 +325,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 @@ -336,7 +336,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