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 2021/06/05 16:27:36 UTC
[GitHub] [hbase] rda3mon opened a new pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
rda3mon opened a new pull request #3359:
URL: https://github.com/apache/hbase/pull/3359
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache9 commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-917648062
Will merge tomorrow if still no other responses and objections.
Thank you for your patient @rda3mon .
--
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
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<TableName, HashMap<String, Long>> incrTimestampMap) {
+ public void setIncrTimestampMap(Map<TableName, Map<String, Long>> incrTimestampMap) {
this.backupImage.setIncrTimeRanges(incrTimestampMap);
}
- public Map<TableName, HashMap<String, Long>> getIncrTimestampMap() {
+ public Map<TableName, Map<String, Long>> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
saintstack commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-899718573
Do you want to push a new PR here @rda3mon ?
--
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
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-899940780
@saintstack I dont have anything for this one. Unless there are some suggestions. Can we merge if no comments? @Apache9 ?
--
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
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-878731850
@Apache9 Gentle reminder.
--
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
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-917640139
@Apache9 Since @saintstack comment was more on direction than on this particular PR. I have create ticket to track it --> https://issues.apache.org/jira/browse/HBASE-26279. Should we merge these changes as @saintstack has already approved the PR? I have follow up changes on parallel backup for which I can open a 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-882229011
@Apache9 This is a much needed enhancement before going with 3.0 release. Can you kindly spare some time for this?
--
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
[GitHub] [hbase] Apache9 merged pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache9 merged pull request #3359:
URL: https://github.com/apache/hbase/pull/3359
--
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
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<String> 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<TableName,
- HashMap<String, Long>> newTableSetTimestampMap) {
- this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map<TableName,
+ Map<String, Long>> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-863812182
@Apache9 Can you review this?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-855268678
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 2s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 21s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 14s | master passed |
| +1 :green_heart: | compile | 1m 11s | master passed |
| +1 :green_heart: | shadedjars | 9m 2s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 30s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 5s | the patch passed |
| +1 :green_heart: | compile | 1m 11s | the patch passed |
| +1 :green_heart: | javac | 1m 11s | the patch passed |
| +1 :green_heart: | shadedjars | 9m 3s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 28s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 48s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 12m 58s | hbase-backup in the patch passed. |
| | | 46m 28s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux b9434606e73b 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de06e20e0a |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/testReport/ |
| Max. process+thread count | 3090 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<WALItem> 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<String> getIncrBackupLogFileList() throws IOException {
- List<String> logList;
- HashMap<String, Long> newTimestamps;
- HashMap<String, Long> previousTimestampMins;
-
- String savedStartCode = readBackupStartCode();
-
- // key: tableName
- // value: <RegionServer,PreviousTimeStamp>
- HashMap<TableName, HashMap<String, Long>> 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<WALItem> logFromSystemTable =
- getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps, getBackupInfo()
- .getBackupRootDir());
-
- logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
- backupInfo.setIncrBackupFileList(logList);
-
- return logList;
- }
-
- private List<String> excludeAlreadyBackedUpAndProcV2WALs(List<String> logList,
- List<WALItem> logFromSystemTable) {
- Set<String> walFileNameSet = convertToSet(logFromSystemTable);
-
+ private List<String> excludeProcV2WALs(List<String> logList) {
List<String> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677913764
##########
File path: hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
##########
@@ -351,19 +355,19 @@ public void setIncrBackupFileList(List<String> 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<TableName,
- HashMap<String, Long>> newTableSetTimestampMap) {
- this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map<TableName,
+ Map<String, Long>> prevTableSetTimestampMap) {
+ this.incrTimestampMap = prevTableSetTimestampMap;
Review comment:
Is this a bug in the old code? We do not change the name of the method but change the implementation?
##########
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<WALItem> 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<String> getIncrBackupLogFileList() throws IOException {
- List<String> logList;
- HashMap<String, Long> newTimestamps;
- HashMap<String, Long> previousTimestampMins;
-
- String savedStartCode = readBackupStartCode();
-
- // key: tableName
- // value: <RegionServer,PreviousTimeStamp>
- HashMap<TableName, HashMap<String, Long>> 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<WALItem> logFromSystemTable =
- getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps, getBackupInfo()
- .getBackupRootDir());
-
- logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
- backupInfo.setIncrBackupFileList(logList);
-
- return logList;
- }
-
- private List<String> excludeAlreadyBackedUpAndProcV2WALs(List<String> logList,
- List<WALItem> logFromSystemTable) {
- Set<String> walFileNameSet = convertToSet(logFromSystemTable);
-
+ private List<String> excludeProcV2WALs(List<String> logList) {
List<String> 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:
On master branch we do not have WALProcedure any more. Now we use RegionProcedureStore, it has its own WAL. can file another issue to review this.
##########
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:
nits: Open another PR to change the style, it should be 'static final' instead of 'final static'. This could reduce the checkstyle warnings.
--
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
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-856830076
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 28s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 34s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 50s | master passed |
| +1 :green_heart: | compile | 1m 11s | master passed |
| +1 :green_heart: | shadedjars | 8m 30s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 35s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 53s | the patch passed |
| +1 :green_heart: | compile | 1m 16s | the patch passed |
| +1 :green_heart: | javac | 1m 16s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 33s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 33s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 49s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 9m 34s | hbase-backup in the patch passed. |
| | | 41m 31s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux f5cabbda3f1b 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / be14605898 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/3/testReport/ |
| Max. process+thread count | 5070 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/3/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-855328228
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 58s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 32s | Maven dependency ordering for branch |
| -1 :x: | mvninstall | 5m 20s | root in master failed. |
| +1 :green_heart: | compile | 2m 28s | master passed |
| +1 :green_heart: | checkstyle | 0m 25s | master passed |
| +1 :green_heart: | spotbugs | 5m 20s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 21s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 46s | the patch passed |
| +1 :green_heart: | compile | 2m 23s | the patch passed |
| +1 :green_heart: | cc | 2m 23s | the patch passed |
| +1 :green_heart: | javac | 1m 40s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | javac | 0m 43s | hbase-backup generated 0 new + 73 unchanged - 6 fixed = 73 total (was 79) |
| +1 :green_heart: | checkstyle | 0m 27s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 24m 41s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | hbaseprotoc | 1m 29s | the patch passed |
| +1 :green_heart: | spotbugs | 5m 14s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
| | | 65m 57s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool |
| uname | Linux 787395b70867 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de06e20e0a |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/artifact/yetus-general-check/output/branch-mvninstall-root.txt |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-855272036
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 8m 43s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 28s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 54s | master passed |
| +1 :green_heart: | compile | 2m 26s | master passed |
| +1 :green_heart: | checkstyle | 0m 29s | master passed |
| +1 :green_heart: | spotbugs | 5m 10s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 52s | the patch passed |
| +1 :green_heart: | compile | 2m 18s | the patch passed |
| +1 :green_heart: | cc | 2m 18s | the patch passed |
| -0 :warning: | javac | 0m 39s | hbase-backup generated 1 new + 73 unchanged - 6 fixed = 74 total (was 79) |
| +1 :green_heart: | checkstyle | 0m 26s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 24m 20s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | hbaseprotoc | 1m 40s | the patch passed |
| +1 :green_heart: | spotbugs | 5m 37s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 23s | The patch does not generate ASF License warnings. |
| | | 72m 21s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool |
| uname | Linux 8edc9748b32e 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de06e20e0a |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/artifact/yetus-general-check/output/diff-compile-javac-hbase-backup.txt |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<String> 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<TableName,
- HashMap<String, Long>> newTableSetTimestampMap) {
- this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map<TableName,
+ Map<String, Long>> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon removed a comment on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon removed a comment on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-882229011
@Apache9 This is a much needed enhancement before going with 3.0 release. Can you kindly spare some time for this?
--
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
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-869186011
> Will try to take a look tomorrow. It is a bit late in China, sorry...
No problem. Have a look when you find some time. Thanks
--
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
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<String> 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<TableName,
- HashMap<String, Long>> newTableSetTimestampMap) {
- this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map<TableName,
+ Map<String, Long>> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<WALItem> 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<String> getIncrBackupLogFileList() throws IOException {
- List<String> logList;
- HashMap<String, Long> newTimestamps;
- HashMap<String, Long> previousTimestampMins;
-
- String savedStartCode = readBackupStartCode();
-
- // key: tableName
- // value: <RegionServer,PreviousTimeStamp>
- HashMap<TableName, HashMap<String, Long>> 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<WALItem> logFromSystemTable =
- getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps, getBackupInfo()
- .getBackupRootDir());
-
- logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
- backupInfo.setIncrBackupFileList(logList);
-
- return logList;
- }
-
- private List<String> excludeAlreadyBackedUpAndProcV2WALs(List<String> logList,
- List<WALItem> logFromSystemTable) {
- Set<String> walFileNameSet = convertToSet(logFromSystemTable);
-
+ private List<String> excludeProcV2WALs(List<String> logList) {
List<String> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-855268153
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 27s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 23s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 28s | master passed |
| +1 :green_heart: | compile | 1m 27s | master passed |
| +1 :green_heart: | shadedjars | 8m 11s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 19s | the patch passed |
| +1 :green_heart: | compile | 1m 26s | the patch passed |
| +1 :green_heart: | javac | 1m 26s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 16s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 35s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 1s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 9m 34s | hbase-backup in the patch passed. |
| | | 42m 28s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 8950f4daeaa1 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de06e20e0a |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/testReport/ |
| Max. process+thread count | 5025 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-907790305
Thanks @Apache9. Will wait for @saintstack to comment.
--
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
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<TableName> 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<String> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r689755547
##########
File path: hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java
##########
@@ -514,24 +513,6 @@ public void addIncrementalBackupTableSet(Set<TableName> 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<String> files) throws IOException {
- systemTable.addWALFiles(files, backupInfo.getBackupId(), backupInfo.getBackupRootDir());
Review comment:
Did you figure anything @rda3mon ?
--
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
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<TableName> 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<String> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-856841479
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 4s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 32s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 4s | master passed |
| +1 :green_heart: | compile | 2m 7s | master passed |
| +1 :green_heart: | checkstyle | 0m 23s | master passed |
| +1 :green_heart: | spotbugs | 4m 35s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 0s | the patch passed |
| +1 :green_heart: | compile | 2m 5s | the patch passed |
| +1 :green_heart: | cc | 2m 5s | the patch passed |
| +1 :green_heart: | javac | 1m 29s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | javac | 0m 36s | hbase-backup generated 0 new + 73 unchanged - 6 fixed = 73 total (was 79) |
| +1 :green_heart: | checkstyle | 0m 21s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 19m 52s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | hbaseprotoc | 1m 26s | the patch passed |
| +1 :green_heart: | spotbugs | 5m 1s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 20s | The patch does not generate ASF License warnings. |
| | | 54m 46s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool |
| uname | Linux 4d15f602f54b 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / be14605898 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/3/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache9 commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-869175193
Will try to take a look tomorrow. It is a bit late in China, sorry...
--
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
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-856841154
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 27s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 42s | master passed |
| +1 :green_heart: | compile | 1m 55s | master passed |
| +1 :green_heart: | shadedjars | 11m 50s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 42s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 6m 1s | the patch passed |
| +1 :green_heart: | compile | 1m 46s | the patch passed |
| +1 :green_heart: | javac | 1m 46s | the patch passed |
| +1 :green_heart: | shadedjars | 11m 35s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 39s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 18s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 10m 2s | hbase-backup in the patch passed. |
| | | 54m 25s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 83112d3860fd 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / be14605898 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/3/testReport/ |
| Max. process+thread count | 5059 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/3/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache9 commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-907789062
I'm not very familiar with the code so for me I would like to trust your judgement @rda3mon , I have no more questions here. There is still a pending question from @saintstack about whether it is OK to remove the 'system' table, let's wait for his answer first.
Thanks.
--
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
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-912999917
@saintstack Gentle Reminder.
--
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
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-855326553
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 28s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 23s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 6s | master passed |
| +1 :green_heart: | compile | 1m 14s | master passed |
| +1 :green_heart: | shadedjars | 8m 54s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 33s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 39s | the patch passed |
| +1 :green_heart: | compile | 1m 10s | the patch passed |
| +1 :green_heart: | javac | 1m 10s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 46s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 32s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 48s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 9m 56s | hbase-backup in the patch passed. |
| | | 42m 14s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux a105f4796664 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de06e20e0a |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/testReport/ |
| Max. process+thread count | 5051 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-855326582
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 28s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 27s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 33s | master passed |
| +1 :green_heart: | compile | 1m 27s | master passed |
| +1 :green_heart: | shadedjars | 8m 15s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 15s | the patch passed |
| +1 :green_heart: | compile | 1m 25s | the patch passed |
| +1 :green_heart: | javac | 1m 25s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 11s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 0s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 9m 48s | hbase-backup in the patch passed. |
| | | 42m 49s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3359 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux eff342b1ac18 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de06e20e0a |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/testReport/ |
| Max. process+thread count | 4829 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-backup U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3359/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
rda3mon commented on pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#issuecomment-907787890
@Apache9 @saintstack If there are no suggestions, can we merge the change? I have followup patch ready to raise PR --> https://issues.apache.org/jira/browse/HBASE-25784
--
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
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<TableName> 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<String> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<TableName> 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<String> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677780945
##########
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<TableName, HashMap<String, Long>> incrTimestampMap) {
+ public void setIncrTimestampMap(Map<TableName, Map<String, Long>> incrTimestampMap) {
this.backupImage.setIncrTimeRanges(incrTimestampMap);
}
- public Map<TableName, HashMap<String, Long>> getIncrTimestampMap() {
+ public Map<TableName, Map<String, Long>> getIncrTimestampMap() {
Review comment:
Might have been better to do these converstions -- HashMap to Map -- as an issue of their own because they distract from meat of this PR (They are good, just off-the-point I'd say). Next time.
##########
File path: hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
##########
@@ -1011,148 +1006,6 @@ public void deleteIncrementalBackupTableSet(String backupRoot) throws IOExceptio
}
}
- /**
- * Register WAL files as eligible for deletion
- * @param files files
- * @param backupId backup id
- * @param backupRoot root directory path to backup destination
- * @throws IOException exception
- */
- public void addWALFiles(List<String> files, String backupId, String backupRoot)
- throws IOException {
- if (LOG.isTraceEnabled()) {
- LOG.trace("add WAL files to backup system table: " + backupId + " " + backupRoot + " files ["
- + StringUtils.join(files, ",") + "]");
- }
- if (LOG.isDebugEnabled()) {
- files.forEach(file -> LOG.debug("add :" + file));
- }
- try (Table table = connection.getTable(tableName)) {
- List<Put> puts = createPutsForAddWALFiles(files, backupId, backupRoot);
- table.put(puts);
- }
- }
-
- /**
- * Register WAL files as eligible for deletion
- * @param backupRoot root directory path to backup
- * @throws IOException exception
- */
- public Iterator<WALItem> getWALFilesIterator(String backupRoot) throws IOException {
- LOG.trace("get WAL files from backup system table");
-
- final Table table = connection.getTable(tableName);
- Scan scan = createScanForGetWALs(backupRoot);
- final ResultScanner scanner = table.getScanner(scan);
- final Iterator<Result> it = scanner.iterator();
- return new Iterator<WALItem>() {
-
- @Override
- public boolean hasNext() {
- boolean next = it.hasNext();
- if (!next) {
- // close all
- try {
- scanner.close();
- table.close();
- } catch (IOException e) {
- LOG.error("Close WAL Iterator", e);
- }
- }
- return next;
- }
-
- @Override
- public WALItem next() {
- Result next = it.next();
- List<Cell> cells = next.listCells();
- byte[] buf = cells.get(0).getValueArray();
- int len = cells.get(0).getValueLength();
- int offset = cells.get(0).getValueOffset();
- String backupId = new String(buf, offset, len);
- buf = cells.get(1).getValueArray();
- len = cells.get(1).getValueLength();
- offset = cells.get(1).getValueOffset();
- String walFile = new String(buf, offset, len);
- buf = cells.get(2).getValueArray();
- len = cells.get(2).getValueLength();
- offset = cells.get(2).getValueOffset();
- String backupRoot = new String(buf, offset, len);
- return new WALItem(backupId, walFile, backupRoot);
- }
-
- @Override
- public void remove() {
- // not implemented
- throw new RuntimeException("remove is not supported");
- }
- };
- }
-
- /**
- * Check if WAL file is eligible for deletion Future: to support all backup destinations
- * @param file name of a file to check
- * @return true, if deletable, false otherwise.
- * @throws IOException exception
- */
- // TODO: multiple backup destination support
- public boolean isWALFileDeletable(String file) throws IOException {
- if (LOG.isTraceEnabled()) {
- LOG.trace("Check if WAL file has been already backed up in backup system table " + file);
- }
- try (Table table = connection.getTable(tableName)) {
- Get get = createGetForCheckWALFile(file);
- Result res = table.get(get);
- return (!res.isEmpty());
- }
- }
-
- /**
- * Check if WAL file is eligible for deletion using multi-get
- * @param files names of a file to check
- * @return map of results (key: FileStatus object. value: true if the file is deletable, false
- * otherwise)
- * @throws IOException exception
- */
- public Map<FileStatus, Boolean> areWALFilesDeletable(Iterable<FileStatus> files)
- throws IOException {
- final int BUF_SIZE = 100;
-
- Map<FileStatus, Boolean> ret = new HashMap<>();
- try (Table table = connection.getTable(tableName)) {
- List<Get> getBuffer = new ArrayList<>();
- List<FileStatus> fileStatuses = new ArrayList<>();
-
- for (FileStatus file : files) {
- String fn = file.getPath().getName();
- if (fn.startsWith(WALProcedureStore.LOG_PREFIX)) {
- ret.put(file, true);
- continue;
- }
- String wal = file.getPath().toString();
- Get get = createGetForCheckWALFile(wal);
- getBuffer.add(get);
- fileStatuses.add(file);
- if (getBuffer.size() >= BUF_SIZE) {
- Result[] results = table.get(getBuffer);
- for (int i = 0; i < results.length; i++) {
- ret.put(fileStatuses.get(i), !results[i].isEmpty());
- }
- getBuffer.clear();
- fileStatuses.clear();
- }
- }
-
- if (!getBuffer.isEmpty()) {
- Result[] results = table.get(getBuffer);
- for (int i = 0; i < results.length; i++) {
- ret.put(fileStatuses.get(i), !results[i].isEmpty());
- }
- }
- }
- return ret;
- }
-
Review comment:
These removals of accounting from system table look good.
##########
File path: hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java
##########
@@ -514,24 +513,6 @@ public void addIncrementalBackupTableSet(Set<TableName> 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<String> files) throws IOException {
- systemTable.addWALFiles(files, backupInfo.getBackupId(), backupInfo.getBackupRootDir());
Review comment:
Can we get rid of the 'system' table altogether?
##########
File path: hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
##########
@@ -1653,7 +1506,7 @@ public static void restoreFromSnapshot(Connection conn) throws IOException {
}
}
- protected static boolean snapshotExists(Admin admin, String snapshotName) throws IOException {
+ private static boolean snapshotExists(Admin admin, String snapshotName) throws IOException {
Review comment:
Good.
##########
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");
final static byte[] BL_COMMIT = Bytes.toBytes("D");
- private final static String WALS_PREFIX = "wals:";
private final static String SET_KEY_PREFIX = "backupset:";
// separator between BULK_LOAD_PREFIX and ordinals
- protected final static String BLK_LD_DELIM = ":";
+ private final static String BLK_LD_DELIM = ":";
Review comment:
Good.
--
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
[GitHub] [hbase] rda3mon commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup
Posted by GitBox <gi...@apache.org>.
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<String> 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<TableName,
- HashMap<String, Long>> newTableSetTimestampMap) {
- this.tableSetTimestampMap = newTableSetTimestampMap;
+ public void setIncrTimestampMap(Map<TableName,
+ Map<String, Long>> 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org