[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r706840974
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java
##
@@ -514,24 +513,6 @@ public void addIncrementalBackupTableSet(Set
tables) throws IOExcepti
systemTable.addIncrementalBackupTableSet(tables,
backupInfo.getBackupRootDir());
}
- /**
- * Saves list of WAL files after incremental backup operation. These files
will be stored until
- * TTL expiration and are used by Backup Log Cleaner plug-in to determine
which WAL files can be
- * safely purged.
- */
- public void recordWALFiles(List files) throws IOException {
-systemTable.addWALFiles(files, backupInfo.getBackupId(),
backupInfo.getBackupRootDir());
Review comment:
Created a ticket on this -->
https://issues.apache.org/jira/browse/HBASE-26279
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r689997830
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
##
@@ -178,11 +174,10 @@ public String toString() {
final static byte[] BL_PREPARE = Bytes.toBytes("R");
Review comment:
Created issue here -->
https://issues.apache.org/jira/browse/HBASE-26203. Will raise PR in this week.
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r689993418
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java
##
@@ -99,68 +95,19 @@ public IncrementalBackupManager(Connection conn,
Configuration conf) throws IOEx
newTimestamps = readRegionServerLastLogRollResult();
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps,
conf, savedStartCode);
-List logFromSystemTable =
-getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps,
getBackupInfo()
-.getBackupRootDir());
-logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
+logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
return newTimestamps;
}
- /**
- * Get list of WAL files eligible for incremental backup.
- *
- * @return list of WAL files
- * @throws IOException if getting the list of WAL files fails
- */
- public List getIncrBackupLogFileList() throws IOException {
-List logList;
-HashMap newTimestamps;
-HashMap previousTimestampMins;
-
-String savedStartCode = readBackupStartCode();
-
-// key: tableName
-// value:
-HashMap> previousTimestampMap =
readLogTimestampMap();
-
-previousTimestampMins =
BackupUtils.getRSLogTimestampMins(previousTimestampMap);
-
-if (LOG.isDebugEnabled()) {
- LOG.debug("StartCode " + savedStartCode + "for backupID " +
backupInfo.getBackupId());
-}
-// get all new log files from .logs and .oldlogs after last TS and before
new timestamp
-if (savedStartCode == null || previousTimestampMins == null
-|| previousTimestampMins.isEmpty()) {
- throw new IOException(
- "Cannot read any previous back up timestamps from backup system
table. "
- + "In order to create an incremental backup, at least one full
backup is needed.");
-}
-
-newTimestamps = readRegionServerLastLogRollResult();
-
-logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps,
conf, savedStartCode);
-List logFromSystemTable =
-getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps,
getBackupInfo()
-.getBackupRootDir());
-
-logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
-backupInfo.setIncrBackupFileList(logList);
-
-return logList;
- }
-
- private List excludeAlreadyBackedUpAndProcV2WALs(List
logList,
- List logFromSystemTable) {
-Set walFileNameSet = convertToSet(logFromSystemTable);
-
+ private List excludeProcV2WALs(List logList) {
List list = new ArrayList<>();
for (int i=0; i < logList.size(); i++) {
Path p = new Path(logList.get(i));
String name = p.getName();
- if (walFileNameSet.contains(name) ||
name.startsWith(WALProcedureStore.LOG_PREFIX)) {
+ if (name.startsWith(WALProcedureStore.LOG_PREFIX)) {
Review comment:
Create issue here --> https://issues.apache.org/jira/browse/HBASE-26202.
Will raise PR in this week
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r689984154
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java
##
@@ -514,24 +513,6 @@ public void addIncrementalBackupTableSet(Set
tables) throws IOExcepti
systemTable.addIncrementalBackupTableSet(tables,
backupInfo.getBackupRootDir());
}
- /**
- * Saves list of WAL files after incremental backup operation. These files
will be stored until
- * TTL expiration and are used by Backup Log Cleaner plug-in to determine
which WAL files can be
- * safely purged.
- */
- public void recordWALFiles(List files) throws IOException {
-systemTable.addWALFiles(files, backupInfo.getBackupId(),
backupInfo.getBackupRootDir());
Review comment:
@saintstack Sorry, I missed to reply on this one.
There is more cleanup possible, but not everything can be removed. Do you
think It can be merged to `hbase:meta`? Or is there a recommendation on what
should be in `hbase:meta` and what shouldn't?
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r689984154
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java
##
@@ -514,24 +513,6 @@ public void addIncrementalBackupTableSet(Set
tables) throws IOExcepti
systemTable.addIncrementalBackupTableSet(tables,
backupInfo.getBackupRootDir());
}
- /**
- * Saves list of WAL files after incremental backup operation. These files
will be stored until
- * TTL expiration and are used by Backup Log Cleaner plug-in to determine
which WAL files can be
- * safely purged.
- */
- public void recordWALFiles(List files) throws IOException {
-systemTable.addWALFiles(files, backupInfo.getBackupId(),
backupInfo.getBackupRootDir());
Review comment:
@saintstack Sorry, I missed to reply on this one.
There is more cleanup possible, but not everything can be removed. Do you
think It can be merged to `hbase:meta`?
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677947714
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
##
@@ -351,19 +355,19 @@ public void setIncrBackupFileList(List
incrBackupFileList) {
/**
* Set the new region server log timestamps after distributed log roll
- * @param newTableSetTimestampMap table timestamp map
+ * @param prevTableSetTimestampMap table timestamp map
*/
- public void setIncrTimestampMap(HashMap> newTableSetTimestampMap) {
-this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map> prevTableSetTimestampMap) {
+this.incrTimestampMap = prevTableSetTimestampMap;
Review comment:
It looks like. What do you suggest to do here? Since backup is not part
of any release, I took the liberty to correct this. But open for suggestions.
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677948243
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
##
@@ -351,19 +355,19 @@ public void setIncrBackupFileList(List
incrBackupFileList) {
/**
* Set the new region server log timestamps after distributed log roll
- * @param newTableSetTimestampMap table timestamp map
+ * @param prevTableSetTimestampMap table timestamp map
*/
- public void setIncrTimestampMap(HashMap> newTableSetTimestampMap) {
-this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map> prevTableSetTimestampMap) {
+this.incrTimestampMap = prevTableSetTimestampMap;
Review comment:
`incrTimestampMap` is used by incremental backup to check when was
previous backup logroll happened so that it can take backup since then. and
stored as part of backup manifest.
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677948243
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
##
@@ -351,19 +355,19 @@ public void setIncrBackupFileList(List
incrBackupFileList) {
/**
* Set the new region server log timestamps after distributed log roll
- * @param newTableSetTimestampMap table timestamp map
+ * @param prevTableSetTimestampMap table timestamp map
*/
- public void setIncrTimestampMap(HashMap> newTableSetTimestampMap) {
-this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map> prevTableSetTimestampMap) {
+this.incrTimestampMap = prevTableSetTimestampMap;
Review comment:
`incrTimestampMap` is used by incremental backup to check when was
previous backup logroll happened so that it can take backup since then.
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677947714
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
##
@@ -351,19 +355,19 @@ public void setIncrBackupFileList(List
incrBackupFileList) {
/**
* Set the new region server log timestamps after distributed log roll
- * @param newTableSetTimestampMap table timestamp map
+ * @param prevTableSetTimestampMap table timestamp map
*/
- public void setIncrTimestampMap(HashMap> newTableSetTimestampMap) {
-this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map> prevTableSetTimestampMap) {
+this.incrTimestampMap = prevTableSetTimestampMap;
Review comment:
It looks like. What do you suggest to do here?
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677946644
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java
##
@@ -99,68 +95,19 @@ public IncrementalBackupManager(Connection conn,
Configuration conf) throws IOEx
newTimestamps = readRegionServerLastLogRollResult();
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps,
conf, savedStartCode);
-List logFromSystemTable =
-getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps,
getBackupInfo()
-.getBackupRootDir());
-logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
+logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
return newTimestamps;
}
- /**
- * Get list of WAL files eligible for incremental backup.
- *
- * @return list of WAL files
- * @throws IOException if getting the list of WAL files fails
- */
- public List getIncrBackupLogFileList() throws IOException {
-List logList;
-HashMap newTimestamps;
-HashMap previousTimestampMins;
-
-String savedStartCode = readBackupStartCode();
-
-// key: tableName
-// value:
-HashMap> previousTimestampMap =
readLogTimestampMap();
-
-previousTimestampMins =
BackupUtils.getRSLogTimestampMins(previousTimestampMap);
-
-if (LOG.isDebugEnabled()) {
- LOG.debug("StartCode " + savedStartCode + "for backupID " +
backupInfo.getBackupId());
-}
-// get all new log files from .logs and .oldlogs after last TS and before
new timestamp
-if (savedStartCode == null || previousTimestampMins == null
-|| previousTimestampMins.isEmpty()) {
- throw new IOException(
- "Cannot read any previous back up timestamps from backup system
table. "
- + "In order to create an incremental backup, at least one full
backup is needed.");
-}
-
-newTimestamps = readRegionServerLastLogRollResult();
-
-logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps,
conf, savedStartCode);
-List logFromSystemTable =
-getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps,
getBackupInfo()
-.getBackupRootDir());
-
-logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
-backupInfo.setIncrBackupFileList(logList);
-
-return logList;
- }
-
- private List excludeAlreadyBackedUpAndProcV2WALs(List
logList,
- List logFromSystemTable) {
-Set walFileNameSet = convertToSet(logFromSystemTable);
-
+ private List excludeProcV2WALs(List logList) {
List list = new ArrayList<>();
for (int i=0; i < logList.size(); i++) {
Path p = new Path(logList.get(i));
String name = p.getName();
- if (walFileNameSet.contains(name) ||
name.startsWith(WALProcedureStore.LOG_PREFIX)) {
+ if (name.startsWith(WALProcedureStore.LOG_PREFIX)) {
Review comment:
Ack. Will raise 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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677946401
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
##
@@ -178,11 +174,10 @@ public String toString() {
final static byte[] BL_PREPARE = Bytes.toBytes("R");
Review comment:
Ack. will do that.
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677931600
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java
##
@@ -512,11 +512,11 @@ public void addDependentImage(BackupImage image) {
* Set the incremental timestamp map directly.
* @param incrTimestampMap timestamp map
*/
- public void setIncrTimestampMap(HashMap>
incrTimestampMap) {
+ public void setIncrTimestampMap(Map>
incrTimestampMap) {
this.backupImage.setIncrTimeRanges(incrTimestampMap);
}
- public Map> getIncrTimestampMap() {
+ public Map> getIncrTimestampMap() {
Review comment:
Sure. Will keep this on mind.
For this particular thing, it was related. For BackupLogCleaner
`getIncrTimestampMap` was to be stored as part of BackupInfo and restored.
Proto deser was resulting into Type mismatch between Map and HashMap. So, I had
to club this as part of same PR.
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677930266
##
File path:
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java
##
@@ -514,24 +513,6 @@ public void addIncrementalBackupTableSet(Set
tables) throws IOExcepti
systemTable.addIncrementalBackupTableSet(tables,
backupInfo.getBackupRootDir());
}
- /**
- * Saves list of WAL files after incremental backup operation. These files
will be stored until
- * TTL expiration and are used by Backup Log Cleaner plug-in to determine
which WAL files can be
- * safely purged.
- */
- public void recordWALFiles(List files) throws IOException {
-systemTable.addWALFiles(files, backupInfo.getBackupId(),
backupInfo.getBackupRootDir());
Review comment:
Let me review and come back on the feasibility aspect.
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
