Skip to content

HBASE-25368 Filter out more invalid encoded name in isEncodedRegionNa… #2753

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
huaxiangsun merged 1 commit into from
Dec 16, 2020
Merged

HBASE-25368 Filter out more invalid encoded name in isEncodedRegionNa… #2753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 46 additions & 41 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2388,51 +2388,56 @@ CompletableFuture<HRegionLocation> getRegionLocation(byte[] regionNameOrEncodedR
if (regionNameOrEncodedRegionName == null) {
return failedFuture(new IllegalArgumentException("Passed region name can't be null"));
}
try {
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
future = connection.registry.getMetaRegionLocations()
.thenApply(locs -> Stream.of(locs.getRegionLocations())
.filter(loc -> loc.getRegion().getEncodedName().equals(encodedName)).findFirst());
} else {
future = ClientMetaTableAccessor.getRegionLocationWithEncodedName(metaTable,
regionNameOrEncodedRegionName);
}

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
future = connection.registry.getMetaRegionLocations()
.thenApply(locs -> Stream.of(locs.getRegionLocations())
.filter(loc -> loc.getRegion().getEncodedName().equals(encodedName)).findFirst());
} else {
RegionInfo regionInfo =
CatalogFamilyFormat.parseRegionInfoFromRegionName(regionNameOrEncodedRegionName);
if (regionInfo.isMetaRegion()) {
future = connection.registry.getMetaRegionLocations()
.thenApply(locs -> Stream.of(locs.getRegionLocations())
.filter(loc -> loc.getRegion().getReplicaId() == regionInfo.getReplicaId())
.findFirst());
} else {
future =
ClientMetaTableAccessor.getRegionLocation(metaTable, regionNameOrEncodedRegionName);
}
future = ClientMetaTableAccessor.getRegionLocationWithEncodedName(metaTable,
regionNameOrEncodedRegionName);
}
} else {
// Not all regionNameOrEncodedRegionName here is going to be a valid region name,
// it needs to throw out IllegalArgumentException in case tableName is passed in.
RegionInfo regionInfo;
try {
regionInfo = CatalogFamilyFormat.parseRegionInfoFromRegionName(
regionNameOrEncodedRegionName);
} catch (IOException ioe) {
throw new IllegalArgumentException(ioe.getMessage());
}

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)));
} else {
returnedFuture.complete(location.get());
}
});
return returnedFuture;
} catch (IOException e) {
return failedFuture(e);
if (regionInfo.isMetaRegion()) {
future = connection.registry.getMetaRegionLocations()
.thenApply(locs -> Stream.of(locs.getRegionLocations())
.filter(loc -> loc.getRegion().getReplicaId() == regionInfo.getReplicaId())
.findFirst());
} else {
future =
ClientMetaTableAccessor.getRegionLocation(metaTable, regionNameOrEncodedRegionName);
}
}

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)));
} else {
returnedFuture.complete(location.get());
}
});
return returnedFuture;
}

/**
Expand Down
18 changes: 17 additions & 1 deletion hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,23 @@ static byte[] getStartKey(final byte[] regionName) throws IOException {
@InterfaceAudience.Private // For use by internals only.
public static boolean isEncodedRegionName(byte[] regionName) {
// If not parseable as region name, presume encoded. TODO: add stringency; e.g. if hex.
return parseRegionNameOrReturnNull(regionName) == null && regionName.length <= MD5_HEX_LENGTH;
if (parseRegionNameOrReturnNull(regionName) == null) {
if (regionName.length > MD5_HEX_LENGTH) {
return false;
} else if (regionName.length == MD5_HEX_LENGTH) {
return true;
Copy link
Contributor

@saintstack saintstack Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we protecting against? Could we be passed a tablename? If so, why can't we have a tablename that is an MD5? Or 32 bytes in size? Should we check it is all hex at least? I suppose if someone passes a tablename that is an md5, then they are asking for trouble?

Copy link
Contributor Author

@huaxiangsun huaxiangsun Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @saintstack. Two issues are addressed.
For compact/flush/split with the tableName,
` # Requests a table or region or column family major compaction
def major_compact(table_or_region_name, family = nil, type = 'NORMAL')
family_bytes = nil
family_bytes = family.to_java_bytes unless family.nil?
compact_type = nil
if type == 'NORMAL'
compact_type = org.apache.hadoop.hbase.client.CompactType::NORMAL
elsif type == 'MOB'
compact_type = org.apache.hadoop.hbase.client.CompactType::MOB
else
raise ArgumentError, 'only NORMAL or MOB accepted for type!'
end

  begin
    if family_bytes.nil?
      @admin.majorCompactRegion(table_or_region_name.to_java_bytes)
    else
      @admin.majorCompactRegion(table_or_region_name.to_java_bytes, family_bytes)
    end
  rescue java.lang.IllegalArgumentException, org.apache.hadoop.hbase.UnknownRegionException
    if family_bytes.nil?
      @admin.majorCompact(TableName.valueOf(table_or_region_name), compact_type)
    else
      @admin.majorCompact(TableName.valueOf(table_or_region_name), family_bytes, compact_type)
    end
  end
end

`
it first calls majorCompactRegion() with tableName as input. It expects an IllegalArgumentException or UnknownRegionException to call majorCompact().

This normally involves a registry query to get this UnknownRegionException, this is an expensive path.
If it knows that the input string is not an encodedRegionName or regionName, it can throw out IllegalArgumentException without registry query.

For the case that a md5 hex being used as tableName, it has to go through expensive path to find out that this is not an encodedRegionName, it will still work, just optimization wont be applied in this case.

It also fixes a bug for a table name length over 32 bytes, currently, compact/flush/split this tableName from shell will fail.

Copy link
Contributor

@saintstack saintstack Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THanks for explanation. Shove this up in the JIRA description? Its good. Thanks.

} else {
String encodedName = Bytes.toString(regionName);
try {
Integer.parseInt(encodedName);
// If this is a valid integer, it could be hbase:meta's encoded region name.
return true;
Copy link
Contributor

@saintstack saintstack Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know the hbase:meta int. It does not change. Compare it? When meta splits, it will have md5 for new regions.

Copy link
Contributor Author

@huaxiangsun huaxiangsun Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to have meta regionname to compare (another query). This will defeat the purpose of optimization, so instead, it just check if it is an integer. If it is an integer, then this is a possible encoded region name.

} catch(NumberFormatException er) {
return false;
}
}
}
return false;
}

/**
Expand Down
19 changes: 19 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,25 @@ public void testSplitFlushCompactUnknownTable() throws InterruptedException {
assertTrue(exception instanceof TableNotFoundException);
}

@Test
public void testCompactATableWithSuperLongTableName() throws Exception {
TableName tableName = TableName.valueOf(name.getMethodName());
TableDescriptor htd = TableDescriptorBuilder.newBuilder(tableName)
.setColumnFamily(ColumnFamilyDescriptorBuilder.of("fam1")).build();
try {
ADMIN.createTable(htd);
try {
ADMIN.majorCompactRegion(tableName.getName());
ADMIN.majorCompactRegion(Bytes.toBytes("abcd"));
} catch (IllegalArgumentException iae) {
LOG.info("This is expected");
}
} finally {
ADMIN.disableTable(tableName);
ADMIN.deleteTable(tableName);
}
}

@Test
public void testCompactionTimestamps() throws Exception {
TableName tableName = TableName.valueOf(name.getMethodName());
Expand Down