HBASE-30161 Add paginated, single-RPC RegionLocator.getRegionLocations(startKey, limit) API for bulk meta-cache warmup#8236
Conversation
…s(startKey, limit) API for bulk meta-cache warmup
| public List<HRegionLocation> getRegionLocations(byte[] startKey, int limit) throws IOException { | ||
| // No need to page as region locations are already in-memory. | ||
| return getAllRegionLocations(); | ||
| } |
There was a problem hiding this comment.
This is actually breaking the contract established by the javadoc by always returning all regions.
There was a problem hiding this comment.
Yeah, I was also wondering same. Thanks for bringing this up. Instead of implementing paging here, shall I throw an exception saying use getAllRegionLocations as I don't see advantage of paging in here.
There was a problem hiding this comment.
Sure, that works too, as long as you call out the fact that all implementation may not support it and that they should fallback to getAllRegionLocations.
| * more regions exist | ||
| * @throws IOException if a remote or network exception occurs | ||
| */ | ||
| List<HRegionLocation> getRegionLocations(byte[] startKey, int limit) throws IOException; |
There was a problem hiding this comment.
Since this is changing a public interface on a stable branch, this will break external code implementing the interface, why not have a default implementation perhaps throw unsupported or return a blank list?
| * Ordering: regions are returned in ascending region start-key order (the natural order of | ||
| * {@code hbase:meta} rows for a single table). Within each region, replicas are returned in | ||
| * ascending replica-id order (replica 0, then 1, then 2, ...). Split parents and offline regions | ||
| * are filtered out, which may cause a page to contain fewer than {@code limit} regions but never |
There was a problem hiding this comment.
Where is this filtering happening? I didn't see any test coverage either. The existing methods don't do any such filtering correct, so is this even needed?
There was a problem hiding this comment.
Filtering is already implemented and happens in inside MetaTableAccessor.DefaultVisitorBase#visit(). Call chain: HRegionLocator#getRegionLocations() -> MetaTableAccessor.TableVisitorBase#visit() -> MetaTableAccessor.DefaultVisitorBase#visit().
There was a problem hiding this comment.
I am reusing the filtering logic so, no test coverage needed.
| * more regions exist | ||
| * @throws IOException if a remote or network exception occurs | ||
| */ | ||
| List<HRegionLocation> getRegionLocations(byte[] startKey, int limit) throws IOException; |
There was a problem hiding this comment.
Also, this is overloading an existing methods that takes a data row to find its locations, which is a totally different semantic and so reusing that name for this can be very confusing. Perhaps just call this getLocations or may be you can come up with a better name.
… and add default-throws Review comments on apache#8236 by haridsv: - Adding an abstract method to RegionLocator / AsyncTableRegionLocator breaks external implementers. Convert the new method to a default that throws UnsupportedOperationException, with javadoc instructing callers to fall back to getAllRegionLocations(). - Reusing the getRegionLocations name overloads a method with completely different semantic (row -> all replicas of containing region). Rename to getRegionLocationsPage(byte[] startKey, int limit) to make the pagination/range intent explicit. - SnapshotRegionLocator no longer needs an override; it inherits the default-throws and callers fall back to getAllRegionLocations(). Tests renamed to call getRegionLocationsPage; semantic and assertions unchanged.
2 participants
JIRA: HBASE-30161
Generated-by: Claude Opus 4.7