Uh oh!
There was an error while loading. Please reload this page.
HBASE-26157 Expose some IA.LimitedPrivate interface in TestingHBaseCl… - #3643
Conversation
Apache-HBase
commented
Aug 28, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 28, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 28, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 28, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 28, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 28, 2021
🎊 +1 overall
This message was automatically generated. |
gjacoby126
commented
Aug 30, 2021
Thanks for these, @Apache9 . These will be helpful for the cell tag tests I mentioned earlier. Another API sometimes needed when testing a coproc is access to the Coprocessor and/or CoprocessorEnvironment (or WALCoprocessor / WAL) to assert on some state. All of these classes are LP(COPROC), but to get to them from the minicluster even today requires using IA.Private methods (HRegion.getCoprocessorHost, not present on the Region interface, which returns the IA.Private RegionCoprocessorHost. Same for WAL which is IA.Private but required to get to WALCoprocessor / WALObserver. I'll keep looking to find other candidates for useful helper methods. |
Apache9
commented
Aug 31, 2021
Got the mission. I will also try to see how to expose these things and propose new PRs to add support for them. Mind giving me some pointer on what is your current usage? Maybe in Phoenix? And, if the approach here is OK, do you want to approve or I need to find others to get approval? Thanks. |
| @Override | ||
| public Optional<OnlineRegions> getOnlineRegionsInterface(ServerName serverName) { | ||
| return Optional.ofNullable(util.getMiniHBaseCluster().getRegionServer(serverName)); |
There was a problem hiding this comment.
getRegionServer returns an HRegionServer...don't we need to keep going to the HRegionServer's RS coprocessor host to get the OnlineRegions?
There was a problem hiding this comment.
The HRegionServers extends RegionServerServices, RegionServerServices extends MutableOnlineRegions, and finally MutableOnlineRegions extends OnlineRegions, so HRegionServer itself is 'OnlineRegions', we do not need to go to RS coprocessor to get it. The design here is not give CP implementations a way to get the Region interface, but do not want to give the CP implementations the ability to add or remove from the online regions directly as it could mess up the state.
| * And also you could use the returned interface to get all regions on this region server, etc. | ||
| */ | ||
| @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.COPROC) | ||
| Optional<OnlineRegions> getOnlineRegionsInterface(ServerName serverName); |
There was a problem hiding this comment.
nit: does the "Interface" in the method name add any information? Seems like it could be "getOnlineRegions" to return an OnlineRegions. That's just like above, and in general throughout HBase, we have "getRegion" not "getRegionInterface" when we return a Region.
There was a problem hiding this comment.
Why I added the 'Interface' suffix is because I assumed the name 'getOnlineRegions ' is a bit confusing. It could also be explained as get the online regions, where we should return a list of Regions.
WDYT? I'm not a native English speaker so I'm fine with changing the name, if you think 'getOnlineRegions' is enough.
There was a problem hiding this comment.
I think that because the return type is Optional(OnlineRegions), the method name getOnlineRegions is clear without the Interface, and we usually don't append Interface. But it's not a big deal either way.
| try (RegionLocator locator = CONN.getRegionLocator(NAME)) { | ||
| loc = locator.getRegionLocation(info.getStartKey()).getServerName(); | ||
| } | ||
| OnlineRegions onlineRegionsInterface = CLUSTER.getOnlineRegionsInterface(loc).get(); |
There was a problem hiding this comment.
I'm confused why this isn't a ClassCastException going between HRegionServer and OnlineRegions but I see the test is passing in at least some of the test runs. Is there something interesting going on that I'm not understanding?
There was a problem hiding this comment.
As said above, the HRegionServer just extends the OnlineRegions interface, so there is no problem. So actually in the CP implementation, when you get an OnlineRegions instance, it is actually a HRegionServer :)
gjacoby126
commented
Aug 31, 2021
@Apache9 - No objection to limiting the review on this PR to just the methods you already added. I'll approve this PR when the review on those are done, and we can do additional methods in later JIRAs / PRs. |
Apache9
commented
Sep 1, 2021
Thank you @gjacoby126 . I've replied all your comments. Waiting for your response. Thanks. |
| * And also you could use the returned interface to get all regions on this region server, etc. | ||
| */ | ||
| @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.COPROC) | ||
| Optional<OnlineRegions> getOnlineRegionsInterface(ServerName serverName); |
There was a problem hiding this comment.
I think that because the return type is Optional(OnlineRegions), the method name getOnlineRegions is clear without the Interface, and we usually don't append Interface. But it's not a big deal either way.
Apache9
commented
Sep 2, 2021
Thanks @gjacoby126 ! Let me merge. |
…uster (#3643) Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org>
…uster