[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4770: HBASE-27238 Backport backup restore to 2.x

[GitBox <gi...@apache.org>]

bbeaudreault commented on code in PR #4770:
URL: https://github.com/apache/hbase/pull/4770#discussion_r1060973517


##########
hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat2.java:
##########
@@ -254,16 +255,19 @@ public void write(ImmutableBytesWritable row, V cell) throws IOException {
         byte[] tableNameBytes = null;
         if (writeMultipleTables) {
           tableNameBytes = MultiTableHFileOutputFormat.getTableName(row.get());
-          tableNameBytes = TableName.valueOf(tableNameBytes).toBytes();
+          tableNameBytes = TableName.valueOf(tableNameBytes).getNameWithNamespaceInclAsString()

Review Comment:
   I've been looking at this. I think the assumptions in HBASE-20530 are incorrect. The multi-table support in HFileOutputFormat2 was introduced separately from backups and already exists in branch-2. As such we can't break them like this. I think changing this path would fall under "File format compatibility" in our [compatibility rules](https://hbase.apache.org/book.html#hbase.versioning.post10) which requires a major version to change.
   
   It seems like the problem in HBASE-20530 was due to a mismatch between the format returned by `HBackupFileSystem#getTableBackupDir` and this code here. The decision in that jira was to change this code here, rather than getTableBackupDir. I think that was a reasonable decision for two reasons:
   1. it was a major version upgrade at the time.
   2. this code here in HFileOutputFormat2 is susceptible to a bug where multiple tables with the same name but in separate namespaces might collide.
   
   That bug seems problematic, but no one has reported it. We have two options:
   
   1. Leave this code alone here, and instead change `HBackupFileSystem#getTableBackupDir`.
   2. Add a config in HFileOutputFormat2 like `hbase.mapreduce.hfileoutputformat.multi.table.include.namespace`. We can have B&R always set this to true, and the config would be removed as part of 3.x upgrade. We'd use this to gate the logic above.
   
   I don't love adding more config params, but I might suggest choosing option 2. This way if anyone stumbles across this bug in their own use-case, they actually have a way to get around it.



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org