saintstack commented on a change in pull request #2445:
URL: https://github.com/apache/hbase/pull/2445#discussion_r503638276
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonRootRegionLocator.java
##########
@@ -174,10 +184,14 @@ private boolean tryComplete(LocateRequest req,
CompletableFuture<RegionLocations
// startKey < req.row and endKey >= req.row. Here we split it to
endKey == req.row ||
// (endKey > req.row && startKey < req.row). The two conditions are
equal since startKey <
// endKey.
+ KeyValue.KVComparator comparator =
getComparator(loc.getRegion().getTable());
byte[] endKey = loc.getRegion().getEndKey();
- int c = Bytes.compareTo(endKey, req.row);
+ int c = comparator.compareRows(endKey, 0, endKey.length,
+ req.row,0, req.row.length);
Review comment:
This works even if the Cell is ByteBuffer backed?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonRootRegionLocator.java
##########
@@ -68,9 +70,9 @@
* The asynchronous locator for regions other than meta.
*/
@InterfaceAudience.Private
-class AsyncNonMetaRegionLocator {
+class AsyncNonRootRegionLocator {
Review comment:
This locates user-space and hbase:meta Regions?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##########
@@ -2391,42 +2406,91 @@ public void run(Timeout timeout) throws Exception {
return failedFuture(new IllegalArgumentException("Passed region name
can't be null"));
}
try {
+ TableName parentTable;
CompletableFuture<Optional<HRegionLocation>> future;
if (RegionInfo.isEncodedRegionName(regionNameOrEncodedRegionName)) {
String encodedName = Bytes.toString(regionNameOrEncodedRegionName);
- if (encodedName.length() < RegionInfo.MD5_HEX_LENGTH) {
- // old format encodedName, should be meta region
+
+ //TODO francis do we really need to support encoded name for root?
Review comment:
Dunno. I think the region id used to be 0. Could it be the encoded name
too?
The encoded name is usually md5 hash but old style Regions like hbase:meta
used the original simple hash encoded number.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MutableRegionInfo.java
##########
@@ -206,16 +209,20 @@ public TableName getTable() {
*/
@Override
public boolean containsRange(byte[] rangeStartKey, byte[] rangeEndKey) {
- if (Bytes.compareTo(rangeStartKey, rangeEndKey) > 0) {
+ CellComparator comparator = CellComparator.getComparator(tableName);
Review comment:
Yeah, this getComparator is a dupe method? Can we have just one?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##########
@@ -2391,42 +2406,91 @@ public void run(Timeout timeout) throws Exception {
return failedFuture(new IllegalArgumentException("Passed region name
can't be null"));
}
try {
+ TableName parentTable;
CompletableFuture<Optional<HRegionLocation>> future;
if (RegionInfo.isEncodedRegionName(regionNameOrEncodedRegionName)) {
String encodedName = Bytes.toString(regionNameOrEncodedRegionName);
- if (encodedName.length() < RegionInfo.MD5_HEX_LENGTH) {
- // old format encodedName, should be meta region
+
+ //TODO francis do we really need to support encoded name for root?
+ boolean isRoot = false;
Review comment:
isRoot is name of the method that checks a boolean, not the name of
variable.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonRootRegionLocator.java
##########
@@ -667,4 +689,14 @@ int getNumberOfCachedRegionLocations(TableName tableName) {
}
return
tableCache.cache.values().stream().mapToInt(RegionLocations::numNonNullElements).sum();
}
+
+ private static KeyValue.KVComparator getComparator(TableName tableName) {
Review comment:
Is this the right place for this method?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MutableRegionInfo.java
##########
@@ -126,7 +127,9 @@ private static int checkReplicaId(int regionId) {
this.replicaId = checkReplicaId(replicaId);
this.offLine = offLine;
this.regionName = RegionInfo.createRegionName(this.tableName,
this.startKey, this.regionId,
- this.replicaId, !this.tableName.equals(TableName.META_TABLE_NAME));
+ this.replicaId,
+ //1 is region id of FIRST_META_REGION_INFO
+ !(this.tableName.equals(TableName.META_TABLE_NAME) && regionId == 1));
Review comment:
What is happening here? '1' is the id of the hardcoded single region
hbase:meta table but is it going to be the region id when we split meta?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##########
@@ -1130,22 +1143,24 @@ public void run(PRESP resp) {
* List all region locations for the specific table.
*/
private CompletableFuture<List<HRegionLocation>>
getTableHRegionLocations(TableName tableName) {
- if (TableName.META_TABLE_NAME.equals(tableName)) {
+ if (ROOT_TABLE_NAME.equals(tableName)) {
CompletableFuture<List<HRegionLocation>> future = new
CompletableFuture<>();
- addListener(connection.registry.getMetaRegionLocations(), (metaRegions,
err) -> {
+ addListener(connection.registry.getMetaRegionLocations(), (rootRegions,
err) -> {
if (err != null) {
future.completeExceptionally(err);
- } else if (metaRegions == null || metaRegions.isEmpty() ||
- metaRegions.getDefaultRegionLocation() == null) {
+ } else if (rootRegions == null || rootRegions.isEmpty() ||
+ rootRegions.getDefaultRegionLocation() == null) {
future.completeExceptionally(new IOException("meta region does not
found"));
} else {
-
future.complete(Collections.singletonList(metaRegions.getDefaultRegionLocation()));
+
future.complete(Collections.singletonList(rootRegions.getDefaultRegionLocation()));
}
});
return future;
} else {
- // For non-meta table, we fetch all locations by scanning hbase:meta
table
- return ClientMetaTableAccessor.getTableHRegionLocations(metaTable,
tableName);
+ // For non-meta table, we fetch all locations by scanning catalog table
Review comment:
Is comment right? Seems like we can get users-space tables and regions
in meta too?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##########
@@ -2391,42 +2406,91 @@ public void run(Timeout timeout) throws Exception {
return failedFuture(new IllegalArgumentException("Passed region name
can't be null"));
}
try {
+ TableName parentTable;
CompletableFuture<Optional<HRegionLocation>> future;
if (RegionInfo.isEncodedRegionName(regionNameOrEncodedRegionName)) {
String encodedName = Bytes.toString(regionNameOrEncodedRegionName);
- if (encodedName.length() < RegionInfo.MD5_HEX_LENGTH) {
- // old format encodedName, should be meta region
+
+ //TODO francis do we really need to support encoded name for root?
+ boolean isRoot = false;
+ for (int i = 0; i< numRootReplicas; i++) {
+ RegionInfo info =
+
RegionReplicaUtil.getRegionInfoForReplica(RegionInfoBuilder.ROOT_REGIONINFO, i);
+ if (Bytes.equals(info.getRegionName(),
regionNameOrEncodedRegionName)) {
+ isRoot = true;
+ break;
+ }
+ }
+ if (isRoot) {
future = connection.registry.getMetaRegionLocations()
.thenApply(locs -> Stream.of(locs.getRegionLocations())
.filter(loc ->
loc.getRegion().getEncodedName().equals(encodedName)).findFirst());
+ parentTable = null;
+ } else if (encodedName.length() < RegionInfo.MD5_HEX_LENGTH) {
+ future =
ClientMetaTableAccessor.getRegionLocationWithEncodedName(rootTable,
+ regionNameOrEncodedRegionName);
+ parentTable = ROOT_TABLE_NAME;
} else {
future =
ClientMetaTableAccessor.getRegionLocationWithEncodedName(metaTable,
regionNameOrEncodedRegionName);
+ parentTable = META_TABLE_NAME;
}
} else {
RegionInfo regionInfo =
CatalogFamilyFormat.parseRegionInfoFromRegionName(regionNameOrEncodedRegionName);
- if (regionInfo.isMetaRegion()) {
+ if (regionInfo.isRootRegion()) {
future = connection.registry.getMetaRegionLocations()
.thenApply(locs -> Stream.of(locs.getRegionLocations())
.filter(loc -> loc.getRegion().getReplicaId() ==
regionInfo.getReplicaId())
.findFirst());
+ parentTable = null;
+ //TODO francis it won't reach here once meta is split
+ } else if (regionInfo.isMetaRegion()) {
+ parentTable = ROOT_TABLE_NAME;
+ future =
+ ClientMetaTableAccessor.getRegionLocation(rootTable,
regionNameOrEncodedRegionName);
} else {
+ parentTable = META_TABLE_NAME;
future =
ClientMetaTableAccessor.getRegionLocation(metaTable,
regionNameOrEncodedRegionName);
}
}
+ final TableName finalParentTable = parentTable;
CompletableFuture<HRegionLocation> returnedFuture = new
CompletableFuture<>();
addListener(future, (location, err) -> {
if (err != null) {
returnedFuture.completeExceptionally(err);
return;
}
if (!location.isPresent() || location.get().getRegion() == null) {
- returnedFuture.completeExceptionally(
- new UnknownRegionException("Invalid region name or encoded region
name: " +
- Bytes.toStringBinary(regionNameOrEncodedRegionName)));
+ if (META_TABLE_NAME.equals(finalParentTable)) {
+ if (LOG.isDebugEnabled()) {
Review comment:
No need of if LOG.isDebugEnabled... do paramaterized logging....
"... root for region : {}"
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncNonRootRegionLocator.java
##########
@@ -174,10 +184,14 @@ private boolean tryComplete(LocateRequest req,
CompletableFuture<RegionLocations
// startKey < req.row and endKey >= req.row. Here we split it to
endKey == req.row ||
// (endKey > req.row && startKey < req.row). The two conditions are
equal since startKey <
// endKey.
+ KeyValue.KVComparator comparator =
getComparator(loc.getRegion().getTable());
byte[] endKey = loc.getRegion().getEndKey();
- int c = Bytes.compareTo(endKey, req.row);
+ int c = comparator.compareRows(endKey, 0, endKey.length,
+ req.row,0, req.row.length);
Review comment:
Will it never be ByteBuffer backed because it is endkey from RegionInfo?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##########
@@ -2391,42 +2406,91 @@ public void run(Timeout timeout) throws Exception {
return failedFuture(new IllegalArgumentException("Passed region name
can't be null"));
}
try {
+ TableName parentTable;
CompletableFuture<Optional<HRegionLocation>> future;
if (RegionInfo.isEncodedRegionName(regionNameOrEncodedRegionName)) {
String encodedName = Bytes.toString(regionNameOrEncodedRegionName);
- if (encodedName.length() < RegionInfo.MD5_HEX_LENGTH) {
- // old format encodedName, should be meta region
+
+ //TODO francis do we really need to support encoded name for root?
+ boolean isRoot = false;
+ for (int i = 0; i< numRootReplicas; i++) {
+ RegionInfo info =
+
RegionReplicaUtil.getRegionInfoForReplica(RegionInfoBuilder.ROOT_REGIONINFO, i);
Review comment:
So, you support replicas in root?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
