-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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-28349 Count atomic operations against read quotas #5668
Conversation
🎊 +1 overall
This message was automatically generated. |
hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/RegionServerRpcQuotaManager.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestAtomicReadQuota.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
* @return the OperationQuota | ||
* @throws RpcThrottlingException if the operation cannot be executed due to quota exceeded. | ||
*/ | ||
public OperationQuota checkQuota(final Region region, final OperationQuota.OperationType type) | ||
throws IOException, RpcThrottlingException { | ||
public OperationQuota checkQuota(final Region region, final OperationQuota.OperationType type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we should add another OperationType CHECK_AND_MUTATE. We can pass that in here instead of adding an isAtomic boolean. We can also add a util in QuotaUtil to return an OperationType for an Action + hasCondition boolean.
We can then use that same api in the other checkQuota below. Something like:
for (final ClientProtos.Action action : actions) {
OperationType type = QuotaUtil.getOperationType(action, hasCondition);
if (type.isRead()) {
numReads++;
}
if (type.isWrite()) {
numWrites++;
}
}
The goal with this change is to DRY up the mutation type checking, and also to clarify a bit. Currently we are using the isAtomic
boolean name which I'm realizing is not accurate. RowMutations
is an example of an atomic operation which is not conditional (thus has no reads).
@@ -2679,7 +2679,8 @@ public MultiResponse multi(final RpcController rpcc, final MultiRequest request) | |||
|
|||
try { | |||
region = getRegion(regionSpecifier); | |||
quota = getRpcQuotaManager().checkQuota(region, regionAction.getActionList()); | |||
quota = getRpcQuotaManager().checkQuota(region, regionAction.getActionList(), | |||
regionAction.getAtomic()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per my other comment about RowMutations I think we actually want to change this and the other usage of getAtomic to hasCondition. I'm realizing that getAtomic seems specific to RowMutations, which may or may not be conditional depending of if the RowMutations is called via mutateRow, batch, or checkAndMutate (only the last one is conditional).
With this, we'll need to update your RowMutations test to ensure that it doesnt count against read capacity unless its a checkAndMutate
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestAtomicReadQuota.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I will merge once the pre-commit comes back green
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
…otas (apache#5668) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
Right now atomic operations are just treated as a single write from the quota perspective. Since an atomic operation also encompasses a read, it would make sense to increment readNum and readSize counts appropriately.
cc @bbeaudreault @hgromer @eab148 @bozzkar