Apache9 commented on code in PR #5037:
URL: https://github.com/apache/hbase/pull/5037#discussion_r1110951016
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonMetaRegionLocator.java:
##########
@@ -217,6 +217,223 @@ private boolean tryComplete(LocateRequest req,
CompletableFuture<RegionLocations
}
}
+ /**
+ * Wrapper around ConcurrentSkipListMap ensuring proper access to cached
items. Updates are
+ * synchronized, but reads are not.
+ */
+ private static final class RegionLocationCache {
+
+ private final ConcurrentNavigableMap<byte[], RegionLocations> cache =
+ new ConcurrentSkipListMap<>(BYTES_COMPARATOR);
+
+ /**
+ * Add the given locations to the cache, merging with existing if
necessary. Also cleans out any
+ * previously cached locations which may have been superseded by this one
(i.e. in case of
+ * merged regions). See {@link #cleanOverlappingRegions(RegionLocations)}
+ * @param locs the locations to cache
+ * @return the final location (possibly merged) that was added to the cache
+ */
+ public synchronized RegionLocations add(RegionLocations locs) {
+ byte[] startKey = locs.getRegionLocation().getRegion().getStartKey();
+ RegionLocations oldLocs = cache.putIfAbsent(startKey, locs);
+ if (oldLocs == null) {
+ cleanOverlappingRegions(locs);
+ return locs;
+ }
+
+ // check whether the regions are the same, this usually happens when
table is split/merged,
+ // or deleted and recreated again.
+ RegionInfo region = locs.getRegionLocation().getRegion();
+ RegionInfo oldRegion = oldLocs.getRegionLocation().getRegion();
+ if (region.getEncodedName().equals(oldRegion.getEncodedName())) {
+ RegionLocations mergedLocs = oldLocs.mergeLocations(locs);
+ if (isEqual(mergedLocs, oldLocs)) {
+ // the merged one is the same with the old one, give up
+ LOG.trace("Will not add {} to cache because the old value {} "
+ + " is newer than us or has the same server name."
+ + " Maybe it is updated before we replace it", locs, oldLocs);
+ return oldLocs;
+ }
+ locs = mergedLocs;
+ } else {
+ // the region is different, here we trust the one we fetched. This
maybe wrong but finally
+ // the upper layer can detect this and trigger removal of the wrong
locations
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("The newly fetch region {} is different from the old one
{} for row '{}',"
+ + " try replaying the old one...", region, oldRegion,
Bytes.toStringBinary(startKey));
+ }
+ }
+
+ cache.put(startKey, locs);
+ cleanOverlappingRegions(locs);
+ return locs;
+ }
+
+ /**
+ * When caching a location, the region may have been the result of a
merge. Check to see if the
+ * region's boundaries overlap any other cached locations. Those would
have been merge parents
+ * which no longer exist. We need to proactively clear them out to avoid a
case where a merged
+ * region which receives no requests never gets cleared. This causes
requests to other merged
+ * regions after it to see the wrong cached location. See HBASE-27650
+ * @param locations the new location that was just cached
+ */
+ private void cleanOverlappingRegions(RegionLocations locations) {
+ RegionInfo region = locations.getRegionLocation().getRegion();
+
+ boolean isLast = isEmptyStopRow(region.getEndKey());
+
+ while (true) {
Review Comment:
I think the implementation can still be improved? Now it will stop when we
hit the location itself, but it is still possible that an region whose startKey
is less than this location but the endKey is greater than the startKey of this
location.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
