You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2022/07/23 12:37:06 UTC
[GitHub] [hbase] Apache9 commented on a diff in pull request #4643: HBASE-27201 Clean up error-prone findings in hbase-backup
Apache9 commented on code in PR #4643:
URL: https://github.com/apache/hbase/pull/4643#discussion_r928118558
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -1737,20 +1742,31 @@ static Scan createScanForOrigBulkLoadedFiles(TableName table) {
}
static String getTableNameFromOrigBulkLoadRow(String rowStr) {
- 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();
+ i.next(); // Skip the first entry
+ return i.next();
}
static String getRegionNameFromOrigBulkLoadRow(String rowStr) {
// format is bulk : namespace : table : region : file
- String[] parts = rowStr.split(BLK_LD_DELIM);
- int idx = 3;
- if (parts.length == 4) {
- // the table is in default namespace
- idx = 2;
- }
- LOG.debug("bulk row string " + rowStr + " region " + parts[idx]);
- return parts[idx];
+ List<String> parts = Splitter.onPattern(BLK_LD_DELIM).splitToList(rowStr);
+ Iterator<String> i = parts.iterator();
+ String region;
+ if (parts.size() == 4) {
+ // the table is in default namespace, get it from position 3
+ i.next(); // Skip the first entry
Review Comment:
Use Iterators.get(iter, idx);
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -1720,7 +1725,7 @@ public String[] getListOfBackupIdsFromMergeOperation() throws IOException {
if (val.length == 0) {
return null;
}
- return new String(val).split(",");
+ return new String(val, StandardCharsets.UTF_8).split(",");
Review Comment:
Ditto.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java:
##########
@@ -734,7 +740,10 @@ public static BulkLoadHFiles createLoader(Configuration config) {
public static String findMostRecentBackupId(String[] backupIds) {
long recentTimestamp = Long.MIN_VALUE;
for (String backupId : backupIds) {
- long ts = Long.parseLong(backupId.split("_")[1]);
+ Iterator<String> i = Splitter.on('_').split(backupId).iterator();
Review Comment:
Ditto.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java:
##########
@@ -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();
Review Comment:
Use Iterators.get
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupSystemTable.java:
##########
@@ -112,7 +112,7 @@ public void testWriteReadBackupStartCode() throws IOException {
Long code = 100L;
Review Comment:
Why not just change this to long?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -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(",");
Review Comment:
Splitter.on?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java:
##########
@@ -63,12 +63,14 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import org.apache.hbase.thirdparty.com.google.common.base.Splitter;
+
/**
* A collection for methods used by multiple classes to backup HBase tables.
*/
@InterfaceAudience.Private
public final class BackupUtils {
- protected static final Logger LOG = LoggerFactory.getLogger(BackupUtils.class);
+ static final Logger LOG = LoggerFactory.getLogger(BackupUtils.class);
Review Comment:
Can not be private?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -1737,20 +1742,31 @@ static Scan createScanForOrigBulkLoadedFiles(TableName table) {
}
static String getTableNameFromOrigBulkLoadRow(String rowStr) {
- 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();
Review Comment:
`return Iterators.get(Splitter.on(BLK_LD_DELIM).split(rowStr).iterator(), 1);`
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupSystemTable.java:
##########
@@ -485,15 +485,12 @@ private BackupInfo createBackupInfo() {
return ctxt;
}
- private List<BackupInfo> createBackupInfoList(int size) {
+ private List<BackupInfo> createBackupInfoList(int size) throws InterruptedException {
List<BackupInfo> list = new ArrayList<>();
for (int i = 0; i < size; i++) {
list.add(createBackupInfo());
- try {
- Thread.sleep(10);
- } catch (InterruptedException e) {
- e.printStackTrace();
- }
+ // XXX Why do we need this sleep?
+ Thread.sleep(100);
Review Comment:
It is 10 ms, not 100 ms?
--
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