Uh oh!
There was an error while loading. Please reload this page.
HBASE-28346: Expose checkQuota to Coprocessor Endpoints - #6066
Conversation
rmdmattingly
left a comment
There was a problem hiding this comment.
It might be nice to create a basic minicluster test that implements a coproc using this framework and runs into quotas as expected
| OperationQuota checkScanQuota(Scan scan, long maxBlockBytesScanned, | ||
| long prevBlockBytesScannedDifference) throws IOException, RpcThrottlingException; |
There was a problem hiding this comment.
We'll probably want to add support for checking batch quotas as well. I guess maybe that can be done trivially via the RpcQuotaManager, but could be nice for API consistency maybe?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
charlesconnell
commented
Jul 10, 2024
Added a new test |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ndimiduk
left a comment
There was a problem hiding this comment.
Looks alright by me. I just have the open question about exposing our shaded-protobufs to coprocessor users.
| import org.apache.yetus.audience.InterfaceAudience; | ||
| import org.apache.yetus.audience.InterfaceStability; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos; |
There was a problem hiding this comment.
Just to check, which protobufs are we exposing to Coprocessor implementations, is it the shaded or unshaded module? Do we have an ErrorProne check that we always do the right thing?
There was a problem hiding this comment.
Or -- do we ever? Browsing through the other coprocessor interfaces and base classes, I don't see any protobufs used. Instead, it's all pure Java POJOs.
Should this be org.apache.hadoop.hbase.client.Action and that class exposed to IA.LimitedPrivate ?
There was a problem hiding this comment.
Ah, but this Quota stuff is all up in the RegionServer's business, where shaded protobufs are indeed used directly.
I think the question stands, of whether we want to expose the shaded protobufs to our coprocessor APIs, or if we need to keep this as a "pure" interface.
@apurtell@Apache9@busbey where do you stand on this question?
There was a problem hiding this comment.
According to the ref guide:
In HBase 3.0.0, the hbase-protocol module has been purged, the CPEP implementation should use the protos in hbase-protocol-shaded module, and also make use of the shaded protobuf in hbase-thirdparty.
I also think I remember Andrew saying that our protos are LimitedPrivate (but cant find that in the refguid). If you look at MultiRowMutationEndpoint in branch-3, it has access to org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos
So it seems like it should be ok to expose it in master/branch-3. I think in branch-2 it might be more tricky. MultiRowMutationEndpoint only has access to unshaded protobuf there, so it wouldn't translate into the server internals which use shaded.
One option here would be to not expose checkBatchQuota with ClientProtos.Action as an argument. The implementation of that method on the server side actually just iterates the batch to count the reads and writes, then calls a different checkBatchQuota that takes those two ints. We could expose something like checkBatchQuota(region, int reads, int writes).
There was a problem hiding this comment.
I'm okay with this approach, assuming that we expect downstream implementations to be okay with the lower fidelity summary of the actions list.
There was a problem hiding this comment.
I've made that change.
| this.services = services; | ||
| this.metricRegistry = | ||
| MetricsCoprocessor.createRegistryForRegionCoprocessor(impl.getClass().getName()); | ||
| // lets unit tests through |
There was a problem hiding this comment.
made this more clear
| } | ||
| public static class MyCoprocessor implements RegionCoprocessor { | ||
| private RegionCoprocessorEnvironment env; |
| @AfterClass | ||
| public static void tearDown() throws Exception { | ||
| CONN.close(); |
There was a problem hiding this comment.
docstring on HBaseTestingUtil#getConnection() says,
/**
* Get a shared Connection to the cluster. this method is thread safe.
* @return A Connection that can be shared. Don't close. Will be closed on shutdown of cluster.
*/
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e27117f to
2825e30CompareApache-HBase
commented
Jul 25, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 25, 2024
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com>
… (addendum) Add default implementations of the new methods so that a custom implementation of RegionCoprocessorEnvironment will not fail to compile after upgrade.
…ndum) (#6350) Add default implementations of the new methods so that a custom implementation of RegionCoprocessorEnvironment will not fail to compile after upgrade. Signed-off-by: Nihal Jain <nihaljain@apache.org>
… (addendum) (apache#6350) Add default implementations of the new methods so that a custom implementation of RegionCoprocessorEnvironment will not fail to compile after upgrade. Signed-off-by: Nihal Jain <nihaljain@apache.org>
…ndum) (#6350) Add default implementations of the new methods so that a custom implementation of RegionCoprocessorEnvironment will not fail to compile after upgrade. Signed-off-by: Nihal Jain <nihaljain@apache.org>
This new wiring will not have any users in this PR. I want to use this in AggregateImplemention or something like it, but I will put that in a separate ticket because it might be a big change.