rmdmattingly commented on code in PR #6451:
URL: https://github.com/apache/hbase/pull/6451#discussion_r1834626996
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaUserOverride.java:
##########
@@ -78,39 +78,32 @@ public static void tearDownAfterClass() throws Exception {
@Test
public void testUserGlobalThrottleWithCustomOverride() throws Exception {
final Admin admin = TEST_UTIL.getAdmin();
- final String userOverrideWithQuota = User.getCurrent().getShortName() +
"123";
+ final String userOverrideWithQuota = User.getCurrent().getShortName();
Review Comment:
I made a few small test changes to ensure that refreshes were called
appropriately. Because of the logic in
`ThrottleQuotaTestUtil.triggerUserCacheRefresh` assuming that we're waiting on
throttling for the current user, I also had to change around the procedure of
this test a bit to ensure that the current user is who we're throttling (rather
than the current user plus a suffix, like we were before). But the logic is
still sound
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java:
##########
@@ -395,21 +429,40 @@ private <K, V extends QuotaState> void fetch(final String
type,
* over table quota, use [1 / TotalTableRegionNum * MachineTableRegionNum]
as machine factor.
*/
private void updateQuotaFactors() {
- // Update machine quota factor
- ClusterMetrics clusterMetrics;
- try {
- clusterMetrics = rsServices.getConnection().getAdmin()
- .getClusterMetrics(EnumSet.of(Option.SERVERS_NAME,
Option.TABLE_TO_REGIONS_COUNT));
- } catch (IOException e) {
- LOG.warn("Failed to get cluster metrics needed for updating quotas",
e);
- return;
+ boolean hasTableQuotas = !tableQuotaCache.entrySet().isEmpty()
+ ||
userQuotaCache.values().stream().anyMatch(UserQuotaState::hasTableLimiters);
+ if (hasTableQuotas) {
+ updateTableMachineQuotaFactors();
+ } else {
+ updateOnlyMachineQuotaFactors();
Review Comment:
This check ensures that we only pull down every region state if we actually
need to. Without table machine quotas, there is no point
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java:
##########
@@ -436,6 +489,52 @@ private void updateQuotaFactors() {
}
}
}
+
+ private void updateMachineQuotaFactors(int rsSize) {
+ if (rsSize != 0) {
+ // TODO if use rs group, the cluster limit should be shared by the rs
group
+ machineQuotaFactor = 1.0 / rsSize;
+ }
+ }
+ }
+
+ static class RefreshableExpiringValueCache<T> {
Review Comment:
I made this class because I don't think there's a good keyless equivalent to
a LoadingCache, and a memoized supplier does not offer all of the functionality
that I'd like (on-demand refresh, invalidation)
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java:
##########
@@ -140,8 +150,7 @@ public QuotaLimiter getUserLimiter(final
UserGroupInformation ugi, final TableNa
*/
public UserQuotaState getUserQuotaState(final UserGroupInformation ugi) {
return computeIfAbsent(userQuotaCache, getQuotaUserName(ugi),
- () ->
QuotaUtil.buildDefaultUserQuotaState(rsServices.getConfiguration(), 0L),
- this::triggerCacheRefresh);
+ () ->
QuotaUtil.buildDefaultUserQuotaState(rsServices.getConfiguration(), 0L));
Review Comment:
The change here, and in getQuotaState, are critical. These changes ensure
that each cache miss does not trigger an immediate refresh — particularly given
that cache entries are evicted after 5 refresh periods, this approach is too
heavy handed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
