[
https://issues.apache.org/jira/browse/HBASE-30485?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated HBASE-30485:
-----------------------------------
Labels: pull-request-available (was: )
> ExceedOperationQuota passes writeCapacityUnitConsumed as the readCapacityUnit
> argument of grabQuota
> ---------------------------------------------------------------------------------------------------
>
> Key: HBASE-30485
> URL: https://issues.apache.org/jira/browse/HBASE-30485
> Project: HBase
> Issue Type: Bug
> Components: Quotas
> Affects Versions: 3.0.0-beta-2, 2.6.6
> Reporter: Dimitri BEKALE
> Priority: Major
> Labels: pull-request-available
>
> When exceed-throttle is enabled (\{{Admin.exceedThrottleQuotaSwitch(true)}}),
> {\{RegionServerRpcQuotaManager}} uses \{{ExceedOperationQuota}}. Its
> \{{checkQuota}} method debits
> the limiters with \{{grabQuota}}, and both calls pass the write capacity-unit
> estimate in the read
> capacity-unit position
> ([ExceedOperationQuota.java:97-104|https://github.com/apache/hbase/blob/09474ab77bfa8bede96131164fd4451f093a1a10/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/ExceedOperationQuota.java#L97-L104]):
> {code:java}
> regionServerLimiter.grabQuota(numWrites, writeConsumed, numReads + numScans,
> readConsumed,
> writeCapacityUnitConsumed, writeCapacityUnitConsumed, isAtomic,
> handlerUsageTimeConsumed);
> ...
> limiter.grabQuota(numWrites, writeConsumed, numReads + numScans, readConsumed,
> writeCapacityUnitConsumed, writeCapacityUnitConsumed, isAtomic, 0L);
> {code}
> The signature is {{grabQuota(writeReqs, writeSize, readReqs, readSize,
> writeCapacityUnit,
> readCapacityUnit, isAtomic, estimateHandlerThreadUsageMs)}}
> (\{{QuotaLimiter.java:70}}): the fifth
> and sixth arguments are the write and read capacity units. The
> other three call sites in the same class hierarchy pass
> \{{readCapacityUnitConsumed}} there:
> {\{ExceedOperationQuota.checkQuota}} step 2 (line 82),
> \{{DefaultOperationQuota.checkQuota}} (line
> 151), and \{{ExceedOperationQuota.close()}} via \{{consumeRead}}.
> h3. Effect
> {\{TimeBasedLimiter.grabQuota}} only consumes \{{readCapacityUnitLimiter}}
> when the
> {\{readCapacityUnit}} argument is positive, so with exceed-throttle on and
> capacity-unit quotas set
> on the region server:
> * A pure read (\{{writeCapacityUnitConsumed == 0}}) debits *zero* read
> capacity units. The quota is
> checked against the correct estimate in step 2 but never charged in step 4,
> so read CU throttling
> does not throttle. When \{{exceed}} is true (step 5) the same happens to
> the user/table/namespace
> limiters.
> * A pure write debits the read CU limiter by the *write* CU estimate and the
> request CU limiter
> twice, so writes spuriously exhaust the read CU quota.
> h3. Origin and affected versions
> Introduced with the feature in HBASE-21783 (\{{9a55cbb2c1df}}, 2019-02-21,
> shipped in 2.2.0) — the
> original hunk already had \{{writeCapacityUnitConsumed,
> writeCapacityUnitConsumed}}. HBASE-29231
> reformatted the lines in 2025 and kept the duplicate. Present on \{{master}},
> \{{branch-3}},
> {\{branch-2}}, \{{branch-2.6}} and \{{rel/2.6.6}}.
> {\{TestQuotaThrottle.testExceedThrottleQuota}} only exercises
> \{{READ_NUMBER}} / \{{WRITE_NUMBER}},
> and the capacity-unit tests never enable exceed-throttle, which is presumably
> why this has not been
> noticed.
> h3. Reproducer
> A unit test in the style of \{{TestDefaultOperationQuota}}: region-server
> limiter with
> {\{READ_CAPACITY_UNIT}} soft limit 100/s and an unlimited
> \{{WRITE_CAPACITY_UNIT}}, an
> {\{ExceedOperationQuota}} with no other limiters, two consecutive
> \{{checkBatchQuota(0, 1, false)}}
> calls. One read is estimated at one 65536-byte block = 64 CU at the default
> 1024-byte CU size, so
> the first read fits and should leave 36 CU; the second needs 64 and must throw
> {\{RpcThrottlingException}}. On current master the limiter is debited 0 read
> CU and the second call
> passes:
> {noformat}
> Tests run: 14, Failures: 1, Errors: 0, Skipped: 0
> TestDefaultOperationQuota.testExceedQuotaGrabsReadCapacityUnits:318
> Expected org.apache.hadoop.hbase.quotas.RpcThrottlingException to be
> thrown, but nothing was thrown.
> {noformat}
> With the two-line fix below the same run is 14/14 green (the 13 existing
> tests pass in both runs).
> Verified on \{{master}} at \{{09474ab77b}} with JDK 21.
> {code:java}
> @Test
> public void testExceedQuotaGrabsReadCapacityUnits() throws
> RpcThrottlingException {
> QuotaProtos.Throttle rsThrottle = QuotaProtos.Throttle.newBuilder()
> .setReadCapacityUnit(QuotaProtos.TimedQuota.newBuilder().setSoftLimit(100)
> .setTimeUnit(HBaseProtos.TimeUnit.SECONDS).build())
>
> .setWriteCapacityUnit(QuotaProtos.TimedQuota.newBuilder().setSoftLimit(Long.MAX_VALUE)
> .setTimeUnit(HBaseProtos.TimeUnit.SECONDS).build())
> .build();
> QuotaLimiter rsLimiter = TimeBasedLimiter.fromThrottle(conf, rsThrottle);
> // no user/table/namespace limiters: only the region server quota applies
> ExceedOperationQuota quota =
> new ExceedOperationQuota(new Configuration(), 65536,
> DEFAULT_REQUESTS_PER_SECOND, rsLimiter);
> // one read = one 65536-byte block = 64 read CU; fits in 100 and must leave
> 36
> quota.checkBatchQuota(0, 1, false);
> // the second read needs 64 CU but only 36 remain, so it must be throttled
> assertThrows(RpcThrottlingException.class, () -> quota.checkBatchQuota(0,
> 1, false));
> }
> {code}
> h3. Proposed fix
> {code}
> - writeCapacityUnitConsumed, writeCapacityUnitConsumed, isAtomic,
> handlerUsageTimeConsumed);
> + writeCapacityUnitConsumed, readCapacityUnitConsumed, isAtomic,
> handlerUsageTimeConsumed);
> ...
> - writeCapacityUnitConsumed, writeCapacityUnitConsumed, isAtomic,
> 0L);
> + writeCapacityUnitConsumed, readCapacityUnitConsumed, isAtomic,
> 0L);
> {code}
> I ran only this test class, not the full suite or the integration test
> \{{TestQuotaThrottle}}.
> Happy to open a PR with the fix and the test if that is welcome.
> h3. Provenance
> This was found during an academic study of LLM-assisted code review at
> Jeonbuk National University, Jeonju, South Korea. An automated LLM-based
> reviewer produced the initial analysis and I verified it by reading the
> source at \{{09474ab77bfa}} and the \{{QuotaLimiter}} signature, and by
> running the unit test above against the unpatched and patched class. I
> believe it is a genuine defect and will respect your triage either way.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
