-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
HBASE-27201 Clean up error-prone findings in hbase-backup #4643
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Left a few comments.
Thanks @apurtell for the hard work!
@@ -1639,7 +1644,7 @@ public String[] getListOfBackupIdsFromDeleteOperation() throws IOException { | |||
if (val.length == 0) { | |||
return null; | |||
} | |||
return new String(val).split(","); | |||
return new String(val, StandardCharsets.UTF_8).split(","); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitter.on?
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Outdated
Show resolved
Hide resolved
String[] parts = rowStr.split(BLK_LD_DELIM); | ||
return parts[1]; | ||
// format is bulk : namespace : table : region : file | ||
Iterator<String> i = Splitter.onPattern(BLK_LD_DELIM).split(rowStr).iterator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Iterators.get(Splitter.on(BLK_LD_DELIM).split(rowStr).iterator(), 1);
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Outdated
Show resolved
Hide resolved
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java
Outdated
Show resolved
Hide resolved
@@ -594,8 +598,10 @@ public int compare(BackupInfo o1, BackupInfo o2) { | |||
} | |||
|
|||
private long getTimestamp(String backupId) { | |||
String[] split = backupId.split("_"); | |||
return Long.parseLong(split[1]); | |||
Iterator<String> i = Splitter.on('_').split(backupId).iterator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Iterators.get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this as recurring feedback, got it. Will apply this pattern to these things. There are other cases in other modules where the same change is appropriate.
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java
Outdated
Show resolved
Hide resolved
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupSystemTable.java
Outdated
Show resolved
Hide resolved
e.printStackTrace(); | ||
} | ||
// XXX Why do we need this sleep? | ||
Thread.sleep(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is 10 ms, not 100 ms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10ms is really short dont' you think? But it's fair that this patch shouldn't try to do more, will change it back.
Rebase. Address review feedback. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Outdated
Show resolved
Hide resolved
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Outdated
Show resolved
Hide resolved
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java
Outdated
Show resolved
Hide resolved
Rebase. More review feedback. Fix test. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@Apache9 Planning to merge this tomorrow unless further comment. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
No description provided.