You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "hgromer (via GitHub)" <gi...@apache.org> on 2025/12/29 13:09:20 UTC
[PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
hgromer opened a new pull request, #7582:
URL: https://github.com/apache/hbase/pull/7582
cc @DieterDP-ng @krconv
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2682159689
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
It depends
1. In isolation, without the other changes to this PR, it means we'll never include the WAL files for a host that was removed for the cluster. Keep in mind, currentLogFile is an oldLog. So backup happens, new wals get created and moved to oldWAL dir, server goes offline, the boundary for that server is never updated, those WAL files will never be included in future backups and we lose data
2. In the context of this PR, we update the boundary for each host based on the latest WAL that was included in the backup for the host, so this logic actually completely irrelevant b/c newTimestamp will never be null
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2677182275
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
> I'm not sure I necessarily agree with this logic. B/c the RS not being rolled this go around doesn't mean we've backed up all the files from the RS we need to backup. It just means the host doesn't exist on the cluster at the moment.
>
> 1. Server A is backed up
>
> 2. Server A generate more WAL files
>
> 3. Server A is removed from cluster
>
> 4. New backup occurs, but we don't get a roll time for Server A so we ignore its files
>
>
> We need to backup the files that were generated between the last backup, and this backup
Not sure we're on the same page here.
For the example you gave (quoted above), with the code block I suggested, the following would happen:
- when the logs are rolled, `rollTimestamps` would not have been updated, since the RS A was offline
- so the `rollTimestamps` value for A would be `< rollStartTs`, and A would not be present as a key in `newTimestamps`
- in the updated `getLogFilesForNewBackup`, we'd fall in case `server found in only previous: a region server that has gone offline, all logs will be older than rollStartTs and should be included`, so all logs of A would be included.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7582:
URL: https://github.com/apache/hbase/pull/7582#issuecomment-3699729101
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck |
|||| _ Prechecks _ |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 39s | | master passed |
| +1 :green_heart: | compile | 0m 19s | | master passed |
| +1 :green_heart: | javadoc | 0m 16s | | master passed |
| +1 :green_heart: | shadedjars | 6m 21s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 11s | | the patch passed |
| +1 :green_heart: | compile | 0m 19s | | the patch passed |
| +1 :green_heart: | javac | 0m 19s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 13s | | the patch passed |
| +1 :green_heart: | shadedjars | 6m 13s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 11m 58s | | hbase-backup in the patch passed. |
| | | 34m 1s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/2/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7582 |
| JIRA Issue | HBASE-29776 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux c8fccd520408 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 25d095b942b66ab4b9d5e77492b8a8d3f86767da |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/2/testReport/ |
| Max. process+thread count | 3869 (vs. ulimit of 30000) |
| modules | C: hbase-backup U: hbase-backup |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/2/console |
| versions | git=2.34.1 maven=3.9.8 |
| Powered by | Apache Yetus 0.15.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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2676956513
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
> I don't think so, this would exclude all WAL files between last backup (previousTimestamps) and the current log roll (newTimestamp). Unless I'm misunderstanding
A WAL log from between last backup and current log roll would have a `currentLogTS <= newTimestamp`, no? If `currentLogTS > newTimestamp` it means it was created after the log roll that just happened, and should be excluded from the backup (so it should be in `newestLogs`)
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2716998408
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
My company mostly runs on cloud infrastructure, for which I assumed time skew is a solved issue. (Less my area of expertise though.)
- https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/set-time.html
- https://docs.cloud.google.com/compute/docs/instances/time-synchronization
- https://docs.azure.cn/en-us/virtual-machines/linux/time-sync
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2682167982
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -181,6 +189,50 @@ public void execute() throws IOException {
// set overall backup status: complete. Here we make sure to complete the backup.
// After this checkpoint, even if entering cancel process, will let the backup finished
backupInfo.setState(BackupState.COMPLETE);
+
+ // Scan oldlogs for dead/decommissioned hosts and add their max WAL timestamps
+ // to newTimestamps. This ensures subsequent incremental backups won't try to back up
+ // WALs that are already covered by this full backup's snapshot.
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
+ String host = BackupUtils.parseHostNameFromLogFile(hostLogDir.getPath());
+ if (host == null) {
+ continue;
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(hostLogDir.getPath())));
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(oldLogDir)));
Review Comment:
> Question: when exactly (and by what) are WALs moved to the oldWals folder (oldLogDir)?
They are moved asynchronously, after all edits to that WAL file have been closed and all edits have been replicated (if a peer is online)
This happens in `AbstractFSWAL#cleanOldLogs`
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7582:
URL: https://github.com/apache/hbase/pull/7582#issuecomment-3710635275
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 13s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | hbaseanti | 0m 0s | | Patch does not have any anti-patterns. |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 59s | | master passed |
| +1 :green_heart: | compile | 0m 36s | | master passed |
| +1 :green_heart: | checkstyle | 0m 13s | | master passed |
| +1 :green_heart: | spotbugs | 0m 34s | | master passed |
| +1 :green_heart: | spotless | 0m 50s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 58s | | the patch passed |
| +1 :green_heart: | compile | 0m 31s | | the patch passed |
| -0 :warning: | javac | 0m 31s | [/results-compile-javac-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/3/artifact/yetus-general-check/output/results-compile-javac-hbase-backup.txt) | hbase-backup generated 1 new + 127 unchanged - 0 fixed = 128 total (was 127) |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 12s | | the patch passed |
| +1 :green_heart: | spotbugs | 0m 36s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 12m 16s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| +1 :green_heart: | spotless | 0m 43s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 11s | | The patch does not generate ASF License warnings. |
| | | 30m 28s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7582 |
| JIRA Issue | HBASE-29776 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux de11d54be049 6.8.0-1024-aws #26~22.04.1-Ubuntu SMP Wed Feb 19 06:54:57 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / d45cb7bdec8135441ecdb596b9427dc165f7a705 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 83 (vs. ulimit of 30000) |
| modules | C: hbase-backup U: hbase-backup |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/3/console |
| versions | git=2.34.1 maven=3.9.8 spotbugs=4.7.3 |
| Powered by | Apache Yetus 0.15.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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7582:
URL: https://github.com/apache/hbase/pull/7582#issuecomment-3710650389
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck |
|||| _ Prechecks _ |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 42s | | master passed |
| +1 :green_heart: | compile | 0m 19s | | master passed |
| +1 :green_heart: | javadoc | 0m 16s | | master passed |
| +1 :green_heart: | shadedjars | 6m 23s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 11s | | the patch passed |
| +1 :green_heart: | compile | 0m 19s | | the patch passed |
| +1 :green_heart: | javac | 0m 19s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 14s | | the patch passed |
| +1 :green_heart: | shadedjars | 6m 17s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 12m 13s | | hbase-backup in the patch passed. |
| | | 34m 33s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/3/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7582 |
| JIRA Issue | HBASE-29776 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux fbc84f21bee9 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / d45cb7bdec8135441ecdb596b9427dc165f7a705 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/3/testReport/ |
| Max. process+thread count | 3797 (vs. ulimit of 30000) |
| modules | C: hbase-backup U: hbase-backup |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/3/console |
| versions | git=2.34.1 maven=3.9.8 |
| Powered by | Apache Yetus 0.15.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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2676876057
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -144,21 +152,21 @@ public void execute() throws IOException {
// logs while we do the backup.
backupManager.writeBackupStartCode(0L);
}
- // We roll log here before we do the snapshot. It is possible there is duplicate data
- // in the log that is already in the snapshot. But if we do it after the snapshot, we
- // could have data loss.
- // A better approach is to do the roll log on each RS in the same global procedure as
- // the snapshot.
- LOG.info("Execute roll log procedure for full backup ...");
// Gather the bulk loads being tracked by the system, which can be deleted (since their data
// will be part of the snapshot being taken). We gather this list before taking the actual
// snapshots for the same reason as the log rolls.
List<BulkLoad> bulkLoadsToDelete = backupManager.readBulkloadRows(tableList);
+ Map<String, Long> previousLogRollsByHost = backupManager.readRegionServerLastLogRollResult();
+ // We roll log here before we do the snapshot. It is possible there is duplicate data
Review Comment:
Nit: this comment block can be improved a bit.
- Won't there always be data duplication between the rolled log and the snapshot? I don't see a reason to mention this.
- I would mention that we roll the logs to create a boundary for WALs that need to be tracked for upcoming incremental backups.
- `A better approach is` -> `...would be` (to avoid confusion that you're talking about what is actually happen
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2847878275
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
Review Comment:
I'm happy to address these test cases
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2746804991
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
Ah, I assumed you wanted to remove this check because you thought it has no effect. Now I understand that you're suggesting to change the existing behavior to include all WALs available at that point in time. (Correct me if I'm wrong here.)
I think that in most general cases, it's fine to include all WALs. However, there might be some corner cases, where the backup storage is acting slow, as mentioned in my previous comment. In a hypothetical case like that, it could be possible that there's a significant difference in the everything-up-to-this-point-is-backed-up between different tables. If the user does some kind of cross-table operation, there's a higher change of data being out of sync.
This is all very hypothetical, but keeping this check does avoid that scenario. Hence why I'm in favor of keeping the check in the code.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7582:
URL: https://github.com/apache/hbase/pull/7582#issuecomment-3696579299
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 1m 15s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck |
|||| _ Prechecks _ |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 54s | | master passed |
| +1 :green_heart: | compile | 0m 16s | | master passed |
| +1 :green_heart: | javadoc | 0m 13s | | master passed |
| +1 :green_heart: | shadedjars | 4m 40s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 22s | | the patch passed |
| +1 :green_heart: | compile | 0m 16s | | the patch passed |
| +1 :green_heart: | javac | 0m 16s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 11s | | the patch passed |
| +1 :green_heart: | shadedjars | 4m 41s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 12m 18s | | hbase-backup in the patch passed. |
| | | 29m 55s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7582 |
| JIRA Issue | HBASE-29776 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux 9b0e7fafe2f6 6.8.0-1024-aws #26~22.04.1-Ubuntu SMP Wed Feb 19 06:54:57 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 7ad60d2d009559ce9bf39ee24b85187591acc540 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/1/testReport/ |
| Max. process+thread count | 3878 (vs. ulimit of 30000) |
| modules | C: hbase-backup U: hbase-backup |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/1/console |
| versions | git=2.34.1 maven=3.9.8 |
| Powered by | Apache Yetus 0.15.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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2712358485
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
> Just out of curiosity: did you actually have evidence of clock skew causing issues? I thought clock-sync methods were sufficient to avoid such issues.
We didn't notice any ourselves, but are wary that it could happen. Which clock-sync methods were you considering 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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2817333240
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -83,12 +82,48 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = readRegionServerLastLogRollResult();
+
+ Map<String, Long> latestLogRollByHost = readRegionServerLastLogRollResult();
+ for (Map.Entry<String, Long> entry : latestLogRollByHost.entrySet()) {
+ String host = entry.getKey();
+ long latestLogRoll = entry.getValue();
+ Long earliestTimestampToIncludeInBackup = previousTimestampMins.get(host);
+
+ boolean isInactive = earliestTimestampToIncludeInBackup != null
+ && earliestTimestampToIncludeInBackup > latestLogRoll;
+
+ long latestTimestampToIncludeInBackup;
+ if (isInactive) {
+ LOG.debug("Avoided resetting latest timestamp boundary for {} from {} to {}", host,
+ earliestTimestampToIncludeInBackup, latestLogRoll);
+ latestTimestampToIncludeInBackup = earliestTimestampToIncludeInBackup;
+ } else {
+ latestTimestampToIncludeInBackup = latestLogRoll;
+ }
+ newTimestamps.put(host, latestTimestampToIncludeInBackup);
+ }
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
+ // Update boundaries based on WALs that will be backed up
+ for (String logFile : logList) {
+ Path logPath = new Path(logFile);
+ String logHost = BackupUtils.parseHostFromOldLog(logPath);
+ if (logHost == null) {
+ logHost = BackupUtils.parseHostNameFromLogFile(logPath.getParent());
Review Comment:
You're right, we can merge the two paths here. I'm actually duplicating work that `parseHostNameFromLogFile` which already handles the difference between the oldWALs and WAL file directory structure
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2665925215
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
Review Comment:
Figured it out, it's to ensure the `newTimestamps` for no longer active region servers are updated.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2676601419
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -181,6 +189,50 @@ public void execute() throws IOException {
// set overall backup status: complete. Here we make sure to complete the backup.
// After this checkpoint, even if entering cancel process, will let the backup finished
backupInfo.setState(BackupState.COMPLETE);
+
+ // Scan oldlogs for dead/decommissioned hosts and add their max WAL timestamps
+ // to newTimestamps. This ensures subsequent incremental backups won't try to back up
+ // WALs that are already covered by this full backup's snapshot.
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
+ String host = BackupUtils.parseHostNameFromLogFile(hostLogDir.getPath());
+ if (host == null) {
+ continue;
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(hostLogDir.getPath())));
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(oldLogDir)));
Review Comment:
There's apparently a config key `hbase.separate.oldlogdir.by.regionserver` that adds the server address as a subfolder, so this line will not work as expected when that's set to `true`.
Question: when exactly (and by what) are WALs moved to the oldWals folder (oldLogDir)?
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2677161083
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
One advantage of not basing `newTimestamps` on the logs, but on the roll timestamps would be that the same logic could be used for full and incremental backups.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2727803013
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
I think this check is now superfluous, and we simply include the WAL file in the current backup in the case you mention.
Check out [this](https://github.com/apache/hbase/blob/d45cb7bdec8135441ecdb596b9427dc165f7a705/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java#L256) logic.
If we have a record of the hose the log file belongs to in our previous TS, we can use that to compare against the log file. Otherwise, by default we add it _unless_ the log file dates back to before savedStartCode.
The final piece is that we then update our boundaries based on _all_ the log files we backup. So the new timestamp boundary for that host would be the TS for L2.
Happy to continue discussing, let me know if you still disagree
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2668925536
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
I think the relevant filtering out for very old log files happens [here](https://github.com/apache/hbase/blob/d45cb7bdec8135441ecdb596b9427dc165f7a705/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java#L263)
So this logic is safe to remove imo
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2668916419
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
```java
if (newTimestamp != null && currentLogTS > newTimestamp) {
newestLogs.add(currentLogFile);
}
```
I don't think so, this would exclude all WAL files between last backup (previousTimestamps) and the current log roll (newTimestamp). Unless I'm misunderstanding
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2677314263
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
Review Comment:
We should also have a testcases for:
- a RS goes offline after a full backup was taken, before an incremental is taken
- a RS comes online & goes back offline before a full backup
- a RS comes online & goes back offline after a full backup was taken, before an incremental backup
At the same time, I realize the test code for each of these is quite elaborate. So I'm thinking it might be better to write a unit-type test for `IncrementalBackupManager` where the logs are abstracted away in an interface. Test code could then easily define which logs are present in a given scenario, and production code uses a file-based implementation of the logs.
If you want, I could give a shot at refactoring the code 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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2708626185
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
> 1. In isolation, without the other changes to this PR, it means we'll never include the WAL files for a host that was removed for the cluster. Keep in mind, currentLogFile is an oldLog. So backup happens, new wals get created and moved to oldWAL dir, server goes offline, the boundary for that server is never updated, those WAL files will never be included in future backups and we lose data
Agreed. This is the case where `newTimestamp == null`, and I agree this case resulted in data loss in the original code, and should be fixed by not using `if (newTimestamp == null || currentLogTS > newTimestamp)` as-is.
> 2. In the context of this PR, we update the boundary for each host based on the latest WAL that was included in the backup for the host, so this logic actually completely irrelevant b/c newTimestamp will never be null
Again, agreed, for the case where `newTimestamp == null`. Conclusion: `if (newTimestamp == null || currentLogTS > newTimestamp)` is incorrect as-is.
However, my remark is about the case where `newTimestamp != null && currentLogTS > newTimestamp`.
It's probably an extremely unlikely scenario, but imagine the following in a system where some HDFS operations take longer than normal (in the context of this PR):
- A incremental backup is triggered, a log roll is triggered as a result, RegionServer X creates logfile L1.
- Right after the log roll was triggered, but before `getLogFilesForNewBackup` lists all the oldLogs, RegionServer X receives some data and goes offline again, causing an oldLog L2 for X to be written to storage.
- `newTimestamps` will contain an entry `X: timestamp(L1)`
- When `getLogFilesForNewBackup` lists the oldLogs, it will see L2, which is not added to `newestLogs` (because this PR removes the entire code block), hence L2 will be included in the backup.
My remark is that if, instead of deleting the check, we change it to
```
if (newTimestamp != null && currentLogTS > newTimestamp) {
newestLogs.add(currentLogFile);
}
```
then L2 would not be included in the current backup (as I would expect), but it would end up in the following one.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2676601419
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -181,6 +189,50 @@ public void execute() throws IOException {
// set overall backup status: complete. Here we make sure to complete the backup.
// After this checkpoint, even if entering cancel process, will let the backup finished
backupInfo.setState(BackupState.COMPLETE);
+
+ // Scan oldlogs for dead/decommissioned hosts and add their max WAL timestamps
+ // to newTimestamps. This ensures subsequent incremental backups won't try to back up
+ // WALs that are already covered by this full backup's snapshot.
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
+ String host = BackupUtils.parseHostNameFromLogFile(hostLogDir.getPath());
+ if (host == null) {
+ continue;
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(hostLogDir.getPath())));
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(oldLogDir)));
Review Comment:
There's apparently a config key `hbase.separate.oldlogdir.by.regionserver` that adds the server address as a subfolder, so this line will not work as expected when that's set to `true`. (Solution is to also check files that are one folder deeper.)
Question: when exactly (and by what) are WALs moved to the oldWals folder (oldLogDir)?
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2676850468
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
> No worries, appreciate you taking the time to look here. We've found a slew of bugs in the WAL retention system and I think it's important to get this right, so happy to iterate on feedback.
>
> > Since newTimestamps never is pruned, the entry in the backup table will keep growing over time.
>
> Agree with this. It's something we should take a look at.
>
> To your point about WAL retention and boundaries, conceptually, I've been trying to think about it from the perspective of "which WAL files have been backed up". Otherwise you run into issues when a host goes offline.
>
> For example, in the case where
>
> We have a Server A with row X
>
> 1. An incremental backup is taken, A is rolled
>
> 2. A writes more WAL files
>
> 3. Row X is deleted
>
> 4. A major compaction happens
>
> 5. A full backup is taken, WALs are rolled, but we don't update the timestamp for A. Row X is _not_ included in the full backup
>
> 6. An incremental backup is taken, we still have a very old roll time for this host. Row X is backed up again, and shows up in the backup even though we had previously deleted (but the tombstone no longer exists).
>
>
> So we've resurfaced dead data that shouldn't be included. It's problematic to back up WALs that are very old. So this is the main culprit for the added complexity here
I agree with your example, and agree that the change to `FullTableBackupClient` would fix this. It also shrinks the `newTimestamps`, which nice.
> Additionally, I'm weary of comparing timestamps across hosts, which is why I was wary of doing something like generating a boundary timestamp in the backup process, which happens client side and opted to compare WAL timestamps which are generated by the same host.
I see, I originally thought it might be less code to generate a client pre-roll timestamp, but it doesn't really simplify things. For the `FullTableBackupClient` at least, the current code is simple enough. I would suggest to split off a dedicated `calculateNewTimestamps` method with some proper javadoc. (Still thinking about the incremental case.)
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2668873178
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
No worries, appreciate you taking the time to look here. We've found a slew of bugs in the WAL retention system and I think it's important to get this right, so happy to iterate on feedback.
> Since newTimestamps never is pruned, the entry in the backup table will keep growing over time.
Agree with this, at my company we've added a 30d TTL to the table. This isn't perfect, but it works for us. It's possible a more comprehensive solution is needed.
To your point about WAL retention and boundaries, conceptually, I've been trying to think about it from the perspective of "which WAL files have been backed up". Otherwise you run into issues when a host goes offline.
Additionally, I'm weary of comparing timestamps across hosts, which is why I was wary of doing something like generating a boundary timestamp in the backup process, which happens client side and opted to compare WAL timestamps which are generated by the same host.
For example, in the case where
We have a Server A with row X
1. An incremental backup is taken, A is rolled
2. A writes more WAL files
3. Row X is deleted
4. A major compaction happens
5. A full backup is taken, WALs are rolled, but we don't update the timestamp for A. Row X is _not_ included in the full backup
6. An incremental backup is taken, we still have a very old roll time for this host. Row X is backed up again, and shows up in the backup even though we had previously deleted (but the tombstone no longer exists).
So we've resurfaced dead data that shouldn't be included. It's problematic to back up WALs that are very old. So this is the main culprit for the added complexity here
> server found in both previous and newTimestamps: a region server that is unchanged, include logs older than previous and newer than newTimestamps
> server found in only previous: a region server that has gone offline, all logs will be older than rollStartTs and should be included
If I understand correctly, run into this issue
> server found in only newTimestamps: a new region server, include all logs that are older than the corresponding newTimestamp
> server found in neither: a region server that was started and went back offline in between the previous and current backup, all logs will be older than rollStartTs and should be included
Agree here on the first backup this happens, but we never update the host TS and so we'll continue to backup the WAL files and run into the issue mentioned above.
I'd be more than happy to find a simpler solution though, I really don't love how complex this WAL retention boundary logic is; but struggled to do so and also avoid corrupting the data
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7582:
URL: https://github.com/apache/hbase/pull/7582#issuecomment-3699722587
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | hbaseanti | 0m 0s | | Patch does not have any anti-patterns. |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 39s | | master passed |
| +1 :green_heart: | compile | 0m 31s | | master passed |
| +1 :green_heart: | checkstyle | 0m 13s | | master passed |
| +1 :green_heart: | spotbugs | 0m 31s | | master passed |
| +1 :green_heart: | spotless | 0m 51s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 4s | | the patch passed |
| +1 :green_heart: | compile | 0m 29s | | the patch passed |
| -0 :warning: | javac | 0m 29s | [/results-compile-javac-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/2/artifact/yetus-general-check/output/results-compile-javac-hbase-backup.txt) | hbase-backup generated 1 new + 127 unchanged - 0 fixed = 128 total (was 127) |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 11s | | the patch passed |
| +1 :green_heart: | spotbugs | 0m 35s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 12m 13s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| +1 :green_heart: | spotless | 0m 45s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 11s | | The patch does not generate ASF License warnings. |
| | | 31m 23s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7582 |
| JIRA Issue | HBASE-29776 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 27ce6787059f 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 25d095b942b66ab4b9d5e77492b8a8d3f86767da |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 82 (vs. ulimit of 30000) |
| modules | C: hbase-backup U: hbase-backup |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/2/console |
| versions | git=2.34.1 maven=3.9.8 spotbugs=4.7.3 |
| Powered by | Apache Yetus 0.15.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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2677182275
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
> I'm not sure I necessarily agree with this logic. B/c the RS not being rolled this go around doesn't mean we've backed up all the files from the RS we need to backup. It just means the host doesn't exist on the cluster at the moment.
>
> 1. Server A is backed up
>
> 2. Server A generate more WAL files
>
> 3. Server A is removed from cluster
>
> 4. New backup occurs, but we don't get a roll time for Server A so we ignore its files
>
>
> We need to backup the files that were generated between the last backup, and this backup
Not sure we're on the same page here. (Did you miss that I mentioned that `getLogFilesForNewBackup` needs some adjustments?)
For the example you gave (quoted above), with the code block I suggested, the following would happen:
- when the logs are rolled, `rollTimestamps` would not have been updated, since the RS A was offline
- so the `rollTimestamps` value for A would be `< rollStartTs`, and A would not be present as a key in `newTimestamps`
- in the updated `getLogFilesForNewBackup`, we'd fall in case `server found in only previous: a region server that has gone offline, all logs will be older than rollStartTs and should be included`, so all logs of A would be included.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2682104819
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
> One advantage of not basing newTimestamps on the logs, but on the roll timestamps would be that the same logic could be used for full and incremental backups.
It's tricky to use `rollTimestamps` b/c we're comparing timestamps across different machines, and so we can run into clock skew issues. If a WAL file for a host is generated roughly at the time we set `rollTimestamps`; and then the host goes offline before it can be rolled.
Using WAL timestamps allows us to always compare timestamp values generated by the same machine. We avoid any weird clock skew issues
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7582:
URL: https://github.com/apache/hbase/pull/7582#issuecomment-3696578318
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | hbaseanti | 0m 0s | | Patch does not have any anti-patterns. |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 25s | | master passed |
| +1 :green_heart: | compile | 0m 32s | | master passed |
| +1 :green_heart: | checkstyle | 0m 13s | | master passed |
| +1 :green_heart: | spotbugs | 0m 31s | | master passed |
| +1 :green_heart: | spotless | 0m 50s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 56s | | the patch passed |
| +1 :green_heart: | compile | 0m 30s | | the patch passed |
| -0 :warning: | javac | 0m 30s | [/results-compile-javac-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/1/artifact/yetus-general-check/output/results-compile-javac-hbase-backup.txt) | hbase-backup generated 2 new + 127 unchanged - 0 fixed = 129 total (was 127) |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 12s | | the patch passed |
| -1 :x: | spotbugs | 0m 38s | [/new-spotbugs-hbase-backup.html](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/1/artifact/yetus-general-check/output/new-spotbugs-hbase-backup.html) | hbase-backup generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) |
| +1 :green_heart: | hadoopcheck | 11m 23s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| +1 :green_heart: | spotless | 0m 45s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 11s | | The patch does not generate ASF License warnings. |
| | | 29m 54s | | |
| Reason | Tests |
|-------:|:------|
| SpotBugs | module:hbase-backup |
| | Dead store to newTimestamps in org.apache.hadoop.hbase.backup.impl.IncrementalBackupManager.getIncrBackupLogFileMap() At IncrementalBackupManager.java:org.apache.hadoop.hbase.backup.impl.IncrementalBackupManager.getIncrBackupLogFileMap() At IncrementalBackupManager.java:[line 63] |
| | org.apache.hadoop.hbase.backup.impl.IncrementalBackupManager.getIncrBackupLogFileMap() makes inefficient use of keySet iterator instead of entrySet iterator At IncrementalBackupManager.java:keySet iterator instead of entrySet iterator At IncrementalBackupManager.java:[line 92] |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7582 |
| JIRA Issue | HBASE-29776 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 03a6ae72fd23 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 7ad60d2d009559ce9bf39ee24b85187591acc540 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 83 (vs. ulimit of 30000) |
| modules | C: hbase-backup U: hbase-backup |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7582/1/console |
| versions | git=2.34.1 maven=3.9.8 spotbugs=4.7.3 |
| Powered by | Apache Yetus 0.15.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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2668873178
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
No worries, appreciate you taking the time to look here. We've found a slew of bugs in the WAL retention system and I think it's important to get this right, so happy to iterate on feedback.
> Since newTimestamps never is pruned, the entry in the backup table will keep growing over time.
Agree with this. It's something we should take a look at.
To your point about WAL retention and boundaries, conceptually, I've been trying to think about it from the perspective of "which WAL files have been backed up". Otherwise you run into issues when a host goes offline.
For example, in the case where
We have a Server A with row X
1. An incremental backup is taken, A is rolled
2. A writes more WAL files
3. Row X is deleted
4. A major compaction happens
5. A full backup is taken, WALs are rolled, but we don't update the timestamp for A. Row X is _not_ included in the full backup
6. An incremental backup is taken, we still have a very old roll time for this host. Row X is backed up again, and shows up in the backup even though we had previously deleted (but the tombstone no longer exists).
So we've resurfaced dead data that shouldn't be included. It's problematic to back up WALs that are very old. So this is the main culprit for the added complexity here
Additionally, I'm weary of comparing timestamps across hosts, which is why I was wary of doing something like generating a boundary timestamp in the backup process, which happens client side and opted to compare WAL timestamps which are generated by the same host.
> server found in both previous and newTimestamps: a region server that is unchanged, include logs older than previous and newer than newTimestamps
> server found in only previous: a region server that has gone offline, all logs will be older than rollStartTs and should be included
If I understand correctly, run into this issue
> server found in only newTimestamps: a new region server, include all logs that are older than the corresponding newTimestamp
> server found in neither: a region server that was started and went back offline in between the previous and current backup, all logs will be older than rollStartTs and should be included
Agree here on the first backup this happens, but we never update the host TS and so we'll continue to backup the WAL files and run into the issue mentioned above.
I'd be more than happy to find a simpler solution though, I really don't love how complex this WAL retention boundary logic is; but struggled to do so and also avoid corrupting the data
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2834656669
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
Imagine the following scenario, where the underlying storage of HBASE is slow
- An incremental backup is running
- `IncrementalBackupManager#getIncrBackupLogFileMap` executes the log roll at timestamp `T`: `BackupUtils.logRoll(...)`. At this point the logs that are written to storage represent a (not perfect) snapshot of the state of the different tables.
- `IncrementalBackupManager#getIncrBackupLogFileMap` executes the log retrieval: `getLogFilesForNewBackup(...)`
- Now, because we assume the storage is slow, it can take multiple minutes before all files have been added to `resultLogFiles`. Let's say this completes at `T+10`.
- If, during this time, additional data was ingested in HBASE that caused more wals to be written, it could be that some of these are picked up by `getLogFilesForNewBackup`, while others aren't. Since we're assuming the check has been removed, there's nothing that excludes the extra wals from being included in the backup. But this could mean that for some region servers we have logs up to timestamp `T`, for others `T+5` and for some `T+10`.
So, removing the check causes more variability as to the contents of the data included in the backup.
But to reiterate: I don't think this is a likely realistic case, but it showcases the conceptual idea about the mechanism that decides which logs to include.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer closed pull request #7582: HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss
URL: https://github.com/apache/hbase/pull/7582
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2658268536
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
Hi @hgromer, I agree HBASE-29776 is an issue (sorry for not responding sooner there, I was on vacation the past weeks), but I'm not yet convinced this is the right approach to fix it. It feels very complex to reason about, so I wonder if there isn't a simpler approach. Already wanted to give some intermediate feedback while I think a bit more about it.
- Since newTimestamps never is pruned, the entry in the backup table will keep growing over time.
- `newTimestamps` will end up being written in `BackupSystemTable#createPutForWriteRegionServerLogTimestamp`, but these change no longer match the javadoc of that method.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -83,12 +82,48 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = readRegionServerLastLogRollResult();
Review Comment:
This method does an unnecessary scan, since you override all entries in the code you add below.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
My suggestion is to simplify all changes in this file to (the fix for the excluded log files is also needed):
```
LOG.info("Execute roll log procedure for incremental backup ...");
long rollStartTs = EnvironmentEdgeManager.currentTime();
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
Map<String, Long> rollTimestamps = readRegionServerLastLogRollResult();
Map<String, Long> newTimestamps =
rollTimestamps.entrySet().stream()
// Region servers that are offline since the last backup will have old roll timestamps,
// prune their information here, as it is not relevant to be stored or used for finding
// the relevant logs.
.filter(entry -> entry.getValue() > rollStartTs)
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
// This method needs to be adjusted to use "rollStartTs" if an entry is not found in newTimestamps.
// Or alternatively: getLogFilesForNewBackup(previousTimestampMins,
// DefaultedMap.defaultedMap(newTimestamps, rollStartTs), conf, savedStartCode);
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
```
Then when finding which logs to include, these are the options:
- server found in both previous and newTimestamps: a region server that is unchanged, include logs older than previous and newer than newTimestamps
- server found in only previous: a region server that has gone offline, all logs will be older than rollStartTs and should be included
- server found in only newTimestamps: a new region server, include all logs that are older than the corresponding newTimestamp
- server found in neither: a region server that was started and went back offline in between the previous and current backup, all logs will be older than rollStartTs and should be included
This approach will keep `newTimestamps` limited to the relevant entries. We could consider cleaning up the entries for `readRegionServerLastLogRollResult` as well, but left that out of scope for now.
Similar code suffices in the `FullTableBackupClient`.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
I think removing this block entirely is wrong. I believe the semantics of `newestTimestamps` is "ensure we have everything backed up to this timestamp". So if `currentLogTS > newTimestamp` is true, we should indeed skip this file.
So I think this block should be kept, but adjusted to:
```
if (newTimestamp != null && currentLogTS > newTimestamp) {
newestLogs.add(currentLogFile);
}
```
I also think a similar issue is present for the `.logs` in this method.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -83,12 +82,48 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = readRegionServerLastLogRollResult();
+
+ Map<String, Long> latestLogRollByHost = readRegionServerLastLogRollResult();
+ for (Map.Entry<String, Long> entry : latestLogRollByHost.entrySet()) {
+ String host = entry.getKey();
+ long latestLogRoll = entry.getValue();
+ Long earliestTimestampToIncludeInBackup = previousTimestampMins.get(host);
+
+ boolean isInactive = earliestTimestampToIncludeInBackup != null
+ && earliestTimestampToIncludeInBackup > latestLogRoll;
+
+ long latestTimestampToIncludeInBackup;
+ if (isInactive) {
+ LOG.debug("Avoided resetting latest timestamp boundary for {} from {} to {}", host,
+ earliestTimestampToIncludeInBackup, latestLogRoll);
+ latestTimestampToIncludeInBackup = earliestTimestampToIncludeInBackup;
+ } else {
+ latestTimestampToIncludeInBackup = latestLogRoll;
+ }
+ newTimestamps.put(host, latestTimestampToIncludeInBackup);
+ }
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
+ // Update boundaries based on WALs that will be backed up
Review Comment:
For my understanding, is this code block an optimization, or a necessary fix for a specific case of appearing/disappearing region servers?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -83,12 +82,48 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = readRegionServerLastLogRollResult();
+
+ Map<String, Long> latestLogRollByHost = readRegionServerLastLogRollResult();
+ for (Map.Entry<String, Long> entry : latestLogRollByHost.entrySet()) {
+ String host = entry.getKey();
+ long latestLogRoll = entry.getValue();
+ Long earliestTimestampToIncludeInBackup = previousTimestampMins.get(host);
+
+ boolean isInactive = earliestTimestampToIncludeInBackup != null
+ && earliestTimestampToIncludeInBackup > latestLogRoll;
+
+ long latestTimestampToIncludeInBackup;
+ if (isInactive) {
+ LOG.debug("Avoided resetting latest timestamp boundary for {} from {} to {}", host,
+ earliestTimestampToIncludeInBackup, latestLogRoll);
+ latestTimestampToIncludeInBackup = earliestTimestampToIncludeInBackup;
+ } else {
+ latestTimestampToIncludeInBackup = latestLogRoll;
+ }
+ newTimestamps.put(host, latestTimestampToIncludeInBackup);
+ }
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
+ // Update boundaries based on WALs that will be backed up
+ for (String logFile : logList) {
+ Path logPath = new Path(logFile);
+ String logHost = BackupUtils.parseHostFromOldLog(logPath);
+ if (logHost == null) {
+ logHost = BackupUtils.parseHostNameFromLogFile(logPath.getParent());
Review Comment:
This method seems to support parsing old log names as well, is it possible to merge with the parsing 2 lines above? Though I am confused as to why the former uses `logPath` and the latter `logPath.getParent()`
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -83,12 +82,48 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = readRegionServerLastLogRollResult();
+
+ Map<String, Long> latestLogRollByHost = readRegionServerLastLogRollResult();
+ for (Map.Entry<String, Long> entry : latestLogRollByHost.entrySet()) {
+ String host = entry.getKey();
+ long latestLogRoll = entry.getValue();
+ Long earliestTimestampToIncludeInBackup = previousTimestampMins.get(host);
+
+ boolean isInactive = earliestTimestampToIncludeInBackup != null
+ && earliestTimestampToIncludeInBackup > latestLogRoll;
+
+ long latestTimestampToIncludeInBackup;
+ if (isInactive) {
+ LOG.debug("Avoided resetting latest timestamp boundary for {} from {} to {}", host,
+ earliestTimestampToIncludeInBackup, latestLogRoll);
+ latestTimestampToIncludeInBackup = earliestTimestampToIncludeInBackup;
+ } else {
+ latestTimestampToIncludeInBackup = latestLogRoll;
+ }
+ newTimestamps.put(host, latestTimestampToIncludeInBackup);
+ }
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
+ // Update boundaries based on WALs that will be backed up
+ for (String logFile : logList) {
+ Path logPath = new Path(logFile);
+ String logHost = BackupUtils.parseHostFromOldLog(logPath);
+ if (logHost == null) {
+ logHost = BackupUtils.parseHostNameFromLogFile(logPath.getParent());
+ }
+ if (logHost != null) {
+ long logTs = BackupUtils.getCreationTime(logPath);
+ Long latestTimestampToIncludeInBackup = newTimestamps.get(logHost);
+ if (latestTimestampToIncludeInBackup == null || logTs > latestTimestampToIncludeInBackup) {
+ LOG.info("Updating backup boundary for inactive host {}: timestamp={}", logHost, logTs);
Review Comment:
Is this log line correct? Are we dealing with an inactive host if `latestTimestampToIncludeInBackup != null`?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
Review Comment:
Haven't gotten around to processing the changes in this file, but can you sketch why they are needed? Since your original ticket only discusses an issue with incremental backups.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
From your comment in HBASE-29776:
> newTimestamp represents the last time a backup rolled the WAL on the RS. If the RegionServer isn't running and therefore isn't able to roll the WAL again, then this timestamp will be in the past, and we end up filtering out all WAL files that were updated since then. Why would we filter out oldWALs that have been created since then? That seems wrong as well
Your comment is correct, but I think the better fix is to ensure the newTimestamps are correctly updated (as you do in your other changes). Removing this block would result in too many logs being included in the backup.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -83,12 +82,48 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = readRegionServerLastLogRollResult();
+
+ Map<String, Long> latestLogRollByHost = readRegionServerLastLogRollResult();
+ for (Map.Entry<String, Long> entry : latestLogRollByHost.entrySet()) {
+ String host = entry.getKey();
+ long latestLogRoll = entry.getValue();
+ Long earliestTimestampToIncludeInBackup = previousTimestampMins.get(host);
+
+ boolean isInactive = earliestTimestampToIncludeInBackup != null
+ && earliestTimestampToIncludeInBackup > latestLogRoll;
+
+ long latestTimestampToIncludeInBackup;
+ if (isInactive) {
+ LOG.debug("Avoided resetting latest timestamp boundary for {} from {} to {}", host,
+ earliestTimestampToIncludeInBackup, latestLogRoll);
+ latestTimestampToIncludeInBackup = earliestTimestampToIncludeInBackup;
+ } else {
+ latestTimestampToIncludeInBackup = latestLogRoll;
+ }
+ newTimestamps.put(host, latestTimestampToIncludeInBackup);
+ }
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
+ // Update boundaries based on WALs that will be backed up
Review Comment:
Figured it out, it is to update the newTimestamps entries for regionservers that have since gone offline, but for which the logs are now backed up.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2668902625
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
```java
Map<String, Long> newTimestamps =
rollTimestamps.entrySet().stream()
// Region servers that are offline since the last backup will have old roll timestamps,
// prune their information here, as it is not relevant to be stored or used for finding
// the relevant logs.
.filter(entry -> entry.getValue() > rollStartTs)
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
```
I'm not sure I necessarily agree with this logic. B/c the RS not being rolled this go around doesn't mean we've backed up all the files from the RS we need to backup. It just means the host doesn't exist on the cluster at the moment.
1. Server A is backed up
2. Server A generate more WAL files
3. Server A is removed from cluster
4. New backup occurs, but we don't get a roll time for Server A so we ignore its files
We need to backup the files that were generated between the last backup, and this backup
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2668902625
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
```java
Map<String, Long> newTimestamps =
rollTimestamps.entrySet().stream()
// Region servers that are offline since the last backup will have old roll timestamps,
// prune their information here, as it is not relevant to be stored or used for finding
// the relevant logs.
.filter(entry -> entry.getValue() > rollStartTs)
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
```
I'm not sure I necessarily agree with this logic. B/c the RS not being rolled this go around doesn't mean we've backed up all the files from the RS we need to backup. It just means the host doesn't exist on the cluster at the moment.
1. Server A is backed up
2. Server A generate more WAL files
3. Server A is removed from cluster
4. New backup occurs, but we don't get a roll time for Server A so we ignore its files
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2668873178
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
No worries, appreciate you taking the time to look here. We've found a slew of bugs in the WAL retention system and I think it's important to get this right, so happy to iterate on feedback.
> Since newTimestamps never is pruned, the entry in the backup table will keep growing over time.
Agree with this, at my company we've added a 30d TTL to the table. This isn't perfect, but it works for us. It's possible a more comprehensive solution is needed.
To your point about WAL retention and boundaries, conceptually, I've been trying to think about it from the perspective of "which WAL files have been backed up". Otherwise you run into issues when a host goes offline.
For example, in the case where
We have a Server A with row X
1. An incremental backup is taken, A is rolled
2. A writes more WAL files
3. Row X is deleted
4. A major compaction happens
5. A full backup is taken, WALs are rolled, but we don't update the timestamp for A. Row X is _not_ included in the full backup
6. An incremental backup is taken, we still have a very old roll time for this host. Row X is backed up again, and shows up in the backup even though we had previously deleted (but the tombstone no longer exists).
So we've resurfaced dead data that shouldn't be included. It's problematic to back up WALs that are very old. So this is the main culprit for the added complexity here
Additionally, I'm weary of comparing timestamps across hosts, which is why I was wary of doing something like generating a boundary timestamp in the backup process, which happens client side and opted to compare WAL timestamps which are generated by the same host.
> server found in both previous and newTimestamps: a region server that is unchanged, include logs older than previous and newer than newTimestamps
> server found in only previous: a region server that has gone offline, all logs will be older than rollStartTs and should be included
If I understand correctly, run into this issue
> server found in only newTimestamps: a new region server, include all logs that are older than the corresponding newTimestamp
> server found in neither: a region server that was started and went back offline in between the previous and current backup, all logs will be older than rollStartTs and should be included
Agree here on the first backup this happens, but we never update the host TS and so we'll continue to backup the WAL files and run into the issue mentioned above.
I'd be more than happy to find a simpler solution though, I really don't love how complex this WAL retention boundary logic is; but struggled to do so and also avoid corrupting the data
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2709210795
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
So the scenario would be that a RS host X would (just before receiving the log roll command) start a shutdown, causing it so there's no updated entry for that host in `readRegionServerLastLogRollResult`. Then, there's at least one RS Y that completes it's roll so quick, while having some time skew, that it would appear that X went offline after the the log roll of Y, causing it to not be included.
It's a bit of a far-fetched scenario, but given the complexity of having 2 possible logroll procedures, I can't easily say whether it's possible or not. But I admit that looking at the WAL files instead avoids this complexity of reasoning. So, I guess I'm convinced that the suggested WAL-file approach is fine. I would appreciate if the code is condensed & structured a bit extra though.
Just out of curiosity: did you actually have evidence of clock skew causing issues? I thought clock-sync methods were sufficient to avoid such issues.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2817341872
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
> In a hypothetical case like that, it could be possible that there's a significant difference in the everything-up-to-this-point-is-backed-up between different tables. If the user does some kind of cross-table operation, there's a higher change of data being out of sync.
Hmm do you mind elaborating on this a bit, I'm not sure I quite understand. If the boundaries are based on the the log files that were included in the backup, I'm not sure how things might be out of sync in a way that causes issues.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDePaepe (via GitHub)" <gi...@apache.org>.
DieterDePaepe commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2906666835
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -181,6 +189,50 @@ public void execute() throws IOException {
// set overall backup status: complete. Here we make sure to complete the backup.
// After this checkpoint, even if entering cancel process, will let the backup finished
backupInfo.setState(BackupState.COMPLETE);
+
+ // Scan oldlogs for dead/decommissioned hosts and add their max WAL timestamps
+ // to newTimestamps. This ensures subsequent incremental backups won't try to back up
+ // WALs that are already covered by this full backup's snapshot.
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
+ String host = BackupUtils.parseHostNameFromLogFile(hostLogDir.getPath());
+ if (host == null) {
Review Comment:
This check also occurs in the code below, on both the the new and old logs. So you can just drop this check here.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
@@ -0,0 +1,324 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+
+/**
+ * Tests that WAL files from offline/inactive RegionServers are handled correctly during backup.
+ * Specifically verifies that WALs from an offline RS are:
+ * <ol>
+ * <li>Backed up once in the first backup after the RS goes offline</li>
+ * <li>NOT re-backed up in subsequent backups</li>
+ * </ol>
+ */
+@Category(LargeTests.class)
+public class TestBackupOfflineRS extends TestBackupBase {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestBackupOfflineRS.class);
+
+ private static final Logger LOG = LoggerFactory.getLogger(TestBackupOfflineRS.class);
+
+ @BeforeClass
+ public static void setUp() throws Exception {
+ TEST_UTIL = new HBaseTestingUtil();
+ conf1 = TEST_UTIL.getConfiguration();
+ conf1.setInt("hbase.regionserver.info.port", -1);
+ autoRestoreOnFailure = true;
+ useSecondCluster = false;
+ setUpHelper();
+ // Start an additional RS so we have at least 2
+ TEST_UTIL.getMiniHBaseCluster().startRegionServer();
+ TEST_UTIL.waitTableAvailable(table1);
+ }
+
+ @Before
+ public void cleanupBetweenTests() throws Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ while (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+ TEST_UTIL.waitTableAvailable(table1);
+ try (BackupSystemTable sysTable = new BackupSystemTable(TEST_UTIL.getConnection())) {
+ try {
+ sysTable.finishBackupExclusiveOperation();
+ } catch (Exception ignored) {
+ }
+ for (BackupInfo info : sysTable.getBackupHistory()) {
+ try {
+ sysTable.deleteBackupInfo(info.getBackupId());
+ } catch (Exception ignored) {
+ }
+ }
+ try {
+ sysTable.deleteIncrementalBackupTableSet(BACKUP_ROOT_DIR);
+ } catch (Exception ignored) {
+ }
+ }
+ }
+
+ /**
+ * Tests that when a full backup is taken while an RS is offline (with WALs in oldlogs), the
+ * offline host's timestamps are recorded so subsequent incremental backups don't re-include those
+ * WALs.
+ */
+ @Test
+ public void testBackupWithOfflineRS() throws Exception {
+ LOG.info("Starting testFullBackupWithOfflineRS");
+
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ LOG.info("Inserting data to generate WAL entries");
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 2, 100);
+ }
+
+ int rsToStop = 0;
+ HRegionServer rsBeforeStop = cluster.getRegionServer(rsToStop);
+ String offlineHost =
+ rsBeforeStop.getServerName().getHostname() + ":" + rsBeforeStop.getServerName().getPort();
+ LOG.info("Stopping RS: {}", offlineHost);
+
+ cluster.stopRegionServer(rsToStop);
+ // Wait for WALs to be moved to oldlogs
+ Thread.sleep(5000);
+
+ LOG.info("Taking full backup (with offline RS WALs in oldlogs)");
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue("Full backup should succeed", checkSucceeded(fullBackupId));
+
+ try (BackupSystemTable sysTable = new BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ LOG.info("RS timestamps after full backup: {}", rsTimestamps);
+
+ Long tsAfterFullBackup = rsTimestamps.get(offlineHost);
+ assertNotNull("Offline host should have timestamp recorded in trslm after full backup",
+ tsAfterFullBackup);
+
+ LOG.info("Taking incremental backup (should NOT include offline RS WALs)");
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue("Incremental backup should succeed", checkSucceeded(incrBackupId));
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ rsTimestamps = timestamps.get(table1);
+ assertFalse("Offline host should not have a boundary ",
+ rsTimestamps.containsKey(offlineHost));
+ }
+ }
+
+ /**
+ * Tests that WALs written to an RS after a full backup are correctly included in the subsequent
+ * incremental backup, even if that RS has gone offline before the incremental runs.
+ */
+ @Test
+ public void testRSGoesOfflineAfterFullBackupBeforeIncremental() throws Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 3, 50);
+ }
+
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue("Full backup should succeed", checkSucceeded(fullBackupId));
+
+ // Insert more data after the full backup to generate WALs that belong in the incremental
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 4, 50);
+ }
+
+ // Pick a live RS, force a roll to ensure WAL files exist after the full backup's boundary,
+ // then stop it before the incremental runs
+ HRegionServer rsBeforeStop = cluster.getLiveRegionServerThreads().get(0).getRegionServer();
+ rsBeforeStop.getWalRoller().requestRollAll();
+ rsBeforeStop.getWalRoller().waitUntilWalRollFinished();
+ String offlineHost =
+ rsBeforeStop.getServerName().getHostname() + ":" + rsBeforeStop.getServerName().getPort();
+
+ cluster.stopRegionServer(rsBeforeStop.getServerName());
+ Thread.sleep(5000);
+
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue("Incremental backup should succeed", checkSucceeded(incrBackupId));
+
+ try (BackupSystemTable sysTable = new BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ assertNotNull(
+ "Offline RS should have a timestamp boundary after the incremental that backed up its WALs",
+ rsTimestamps.get(offlineHost));
+
+ String incrBackupId2 = incrementalTableBackup(tables);
+ assertTrue("Second incremental backup should succeed", checkSucceeded(incrBackupId2));
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ rsTimestamps = timestamps.get(table1);
+ assertFalse("Offline RS should not have a boundary after all its WALs have been backed up",
+ rsTimestamps.containsKey(offlineHost));
+ }
+ }
+
+ /**
+ * Tests that a brand-new RS that comes online and goes offline before any backup correctly has
+ * its WALs covered by the full backup. This prevents subsequent incremental backups from
+ * re-including those WALs.
+ */
+ @Test
+ public void testTransientRSBeforeFullBackup() throws Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ // Start a new RS that will be online briefly and then go offline before the full backup
+ HRegionServer transientRS = cluster.startRegionServerAndWait(10000).getRegionServer();
+ try (Admin admin = TEST_UTIL.getConnection().getAdmin()) {
+ List<RegionInfo> regions = admin.getRegions(table1);
+ if (!regions.isEmpty()) {
Review Comment:
I think this check cannot be empty - a default region gets created when a table is created. Having this check here hints that it would be a valid option to have no regions - but in that case there would be no guarantee of an active region being present on transientRS.
I suggest to replace this with an `assertFalse`.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
@@ -0,0 +1,324 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+
+/**
+ * Tests that WAL files from offline/inactive RegionServers are handled correctly during backup.
+ * Specifically verifies that WALs from an offline RS are:
+ * <ol>
+ * <li>Backed up once in the first backup after the RS goes offline</li>
+ * <li>NOT re-backed up in subsequent backups</li>
+ * </ol>
+ */
+@Category(LargeTests.class)
+public class TestBackupOfflineRS extends TestBackupBase {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestBackupOfflineRS.class);
+
+ private static final Logger LOG = LoggerFactory.getLogger(TestBackupOfflineRS.class);
+
+ @BeforeClass
+ public static void setUp() throws Exception {
+ TEST_UTIL = new HBaseTestingUtil();
+ conf1 = TEST_UTIL.getConfiguration();
+ conf1.setInt("hbase.regionserver.info.port", -1);
+ autoRestoreOnFailure = true;
+ useSecondCluster = false;
+ setUpHelper();
+ // Start an additional RS so we have at least 2
+ TEST_UTIL.getMiniHBaseCluster().startRegionServer();
+ TEST_UTIL.waitTableAvailable(table1);
+ }
+
+ @Before
+ public void cleanupBetweenTests() throws Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ while (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+ TEST_UTIL.waitTableAvailable(table1);
+ try (BackupSystemTable sysTable = new BackupSystemTable(TEST_UTIL.getConnection())) {
+ try {
+ sysTable.finishBackupExclusiveOperation();
+ } catch (Exception ignored) {
+ }
+ for (BackupInfo info : sysTable.getBackupHistory()) {
+ try {
+ sysTable.deleteBackupInfo(info.getBackupId());
+ } catch (Exception ignored) {
+ }
+ }
+ try {
+ sysTable.deleteIncrementalBackupTableSet(BACKUP_ROOT_DIR);
+ } catch (Exception ignored) {
+ }
+ }
+ }
+
+ /**
+ * Tests that when a full backup is taken while an RS is offline (with WALs in oldlogs), the
+ * offline host's timestamps are recorded so subsequent incremental backups don't re-include those
+ * WALs.
+ */
+ @Test
+ public void testBackupWithOfflineRS() throws Exception {
+ LOG.info("Starting testFullBackupWithOfflineRS");
+
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ LOG.info("Inserting data to generate WAL entries");
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 2, 100);
+ }
+
+ int rsToStop = 0;
+ HRegionServer rsBeforeStop = cluster.getRegionServer(rsToStop);
+ String offlineHost =
+ rsBeforeStop.getServerName().getHostname() + ":" + rsBeforeStop.getServerName().getPort();
+ LOG.info("Stopping RS: {}", offlineHost);
+
+ cluster.stopRegionServer(rsToStop);
+ // Wait for WALs to be moved to oldlogs
+ Thread.sleep(5000);
+
+ LOG.info("Taking full backup (with offline RS WALs in oldlogs)");
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue("Full backup should succeed", checkSucceeded(fullBackupId));
+
+ try (BackupSystemTable sysTable = new BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ LOG.info("RS timestamps after full backup: {}", rsTimestamps);
+
+ Long tsAfterFullBackup = rsTimestamps.get(offlineHost);
+ assertNotNull("Offline host should have timestamp recorded in trslm after full backup",
+ tsAfterFullBackup);
+
+ LOG.info("Taking incremental backup (should NOT include offline RS WALs)");
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue("Incremental backup should succeed", checkSucceeded(incrBackupId));
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ rsTimestamps = timestamps.get(table1);
+ assertFalse("Offline host should not have a boundary ",
+ rsTimestamps.containsKey(offlineHost));
+ }
+ }
+
+ /**
+ * Tests that WALs written to an RS after a full backup are correctly included in the subsequent
+ * incremental backup, even if that RS has gone offline before the incremental runs.
+ */
+ @Test
+ public void testRSGoesOfflineAfterFullBackupBeforeIncremental() throws Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 3, 50);
+ }
+
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue("Full backup should succeed", checkSucceeded(fullBackupId));
+
+ // Insert more data after the full backup to generate WALs that belong in the incremental
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 4, 50);
+ }
+
+ // Pick a live RS, force a roll to ensure WAL files exist after the full backup's boundary,
+ // then stop it before the incremental runs
+ HRegionServer rsBeforeStop = cluster.getLiveRegionServerThreads().get(0).getRegionServer();
+ rsBeforeStop.getWalRoller().requestRollAll();
+ rsBeforeStop.getWalRoller().waitUntilWalRollFinished();
+ String offlineHost =
+ rsBeforeStop.getServerName().getHostname() + ":" + rsBeforeStop.getServerName().getPort();
+
+ cluster.stopRegionServer(rsBeforeStop.getServerName());
+ Thread.sleep(5000);
+
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue("Incremental backup should succeed", checkSucceeded(incrBackupId));
+
+ try (BackupSystemTable sysTable = new BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ assertNotNull(
+ "Offline RS should have a timestamp boundary after the incremental that backed up its WALs",
+ rsTimestamps.get(offlineHost));
+
+ String incrBackupId2 = incrementalTableBackup(tables);
+ assertTrue("Second incremental backup should succeed", checkSucceeded(incrBackupId2));
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ rsTimestamps = timestamps.get(table1);
+ assertFalse("Offline RS should not have a boundary after all its WALs have been backed up",
+ rsTimestamps.containsKey(offlineHost));
+ }
+ }
+
+ /**
+ * Tests that a brand-new RS that comes online and goes offline before any backup correctly has
+ * its WALs covered by the full backup. This prevents subsequent incremental backups from
+ * re-including those WALs.
+ */
+ @Test
+ public void testTransientRSBeforeFullBackup() throws Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ // Start a new RS that will be online briefly and then go offline before the full backup
+ HRegionServer transientRS = cluster.startRegionServerAndWait(10000).getRegionServer();
+ try (Admin admin = TEST_UTIL.getConnection().getAdmin()) {
+ List<RegionInfo> regions = admin.getRegions(table1);
+ if (!regions.isEmpty()) {
+ admin.move(regions.get(0).getEncodedNameAsBytes(), transientRS.getServerName());
+ TEST_UTIL.waitUntilAllRegionsAssigned(table1);
+ }
+ }
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 5, 50);
+ }
+ transientRS.getWalRoller().requestRollAll();
+ transientRS.getWalRoller().waitUntilWalRollFinished();
+ String transientHost =
+ transientRS.getServerName().getHostname() + ":" + transientRS.getServerName().getPort();
+
+ cluster.stopRegionServer(transientRS.getServerName());
+ Thread.sleep(5000);
+
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue("Full backup should succeed", checkSucceeded(fullBackupId));
+
+ try (BackupSystemTable sysTable = new BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ assertNotNull(
+ "Transient RS that went offline before full backup should have its WAL boundary recorded",
+ rsTimestamps.get(transientHost));
+
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue("Incremental backup after transient RS went offline should succeed",
+ checkSucceeded(incrBackupId));
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ rsTimestamps = timestamps.get(table1);
+ assertFalse(
+ "Transient RS should not have a boundary after the full backup covered all its WALs",
+ rsTimestamps.containsKey(transientHost));
+ }
+ }
+
+ /**
+ * Tests that WALs from an RS that comes online and goes offline between a full backup and an
+ * incremental backup are correctly included in the incremental backup and not re-included in
+ * subsequent incremental backups.
+ */
+ @Test
+ public void testTransientRSAfterFullBackupBeforeIncremental() throws Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 6, 50);
+ }
+
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue("Full backup should succeed", checkSucceeded(fullBackupId));
+
+ // Start a new RS after the full backup, move a region to it, let it accumulate WALs, then stop
+ HRegionServer transientRS = cluster.startRegionServerAndWait(10000).getRegionServer();
+ try (Admin admin = TEST_UTIL.getConnection().getAdmin()) {
+ List<RegionInfo> regions = admin.getRegions(table1);
+ if (!regions.isEmpty()) {
Review Comment:
Same remark as line 230.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -181,6 +189,50 @@ public void execute() throws IOException {
// set overall backup status: complete. Here we make sure to complete the backup.
// After this checkpoint, even if entering cancel process, will let the backup finished
backupInfo.setState(BackupState.COMPLETE);
+
+ // Scan oldlogs for dead/decommissioned hosts and add their max WAL timestamps
+ // to newTimestamps. This ensures subsequent incremental backups won't try to back up
+ // WALs that are already covered by this full backup's snapshot.
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
+ String host = BackupUtils.parseHostNameFromLogFile(hostLogDir.getPath());
+ if (host == null) {
+ continue;
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(hostLogDir.getPath())));
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(oldLogDir)));
Review Comment:
The first part of my comment still needs to be addressed.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDePaepe (via GitHub)" <gi...@apache.org>.
DieterDePaepe commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2913219950
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
This needs an update due to your changes.
Suggestion:
> Gather all log files that either: 1) are newer than the older timestamps, but not newer than the newest timestamps. 2) archived logs whose host name does not occur in newest timestamps.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -181,6 +189,50 @@ public void execute() throws IOException {
// set overall backup status: complete. Here we make sure to complete the backup.
// After this checkpoint, even if entering cancel process, will let the backup finished
backupInfo.setState(BackupState.COMPLETE);
+
+ // Scan oldlogs for dead/decommissioned hosts and add their max WAL timestamps
+ // to newTimestamps. This ensures subsequent incremental backups won't try to back up
+ // WALs that are already covered by this full backup's snapshot.
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
+ String host = BackupUtils.parseHostNameFromLogFile(hostLogDir.getPath());
+ if (host == null) {
+ continue;
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(hostLogDir.getPath())));
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(oldLogDir)));
+
+ newTimestamps = new HashMap<>();
+
+ for (FileStatus log : allLogs) {
+ if (AbstractFSWALProvider.isMetaFile(log.getPath())) {
+ continue;
+ }
+ String host = BackupUtils.parseHostNameFromLogFile(log.getPath());
+ if (host == null) {
+ continue;
+ }
+ long timestamp = BackupUtils.getCreationTime(log.getPath());
+ Long previousLogRoll = previousLogRollsByHost.get(host);
+ Long latestLogRoll = latestLogRollsByHost.get(host);
+ boolean isInactive = latestLogRoll == null || latestLogRoll.equals(previousLogRoll);
+
+ if (isInactive) {
+ long currentTs = newTimestamps.getOrDefault(host, 0L);
+ if (timestamp > currentTs) {
+ newTimestamps.put(host, timestamp);
Review Comment:
Can you add a summarizing javadoc here?
Suggestion, for inactive path:
```
// For a host that was never part of a log roll procedure, or that was offline during the last log roll:
// newTimestamps takes the maximum WAL file creation timestamp found for that host in the filesystem
```
for the active path:
```
// For a host that took part of the log roll (and had one or more logs on the filesystem):
// newTimestamps holds the value of the log roll
```
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
Revisiting this comment, and after some discussion with Claude AI, I can make my remark a bit more concrete:
I think that the `isInactive` check should be updated to use `>=` instead of `>`:
```
boolean isInactive = earliestTimestampToIncludeInBackup != null
&& earliestTimestampToIncludeInBackup >= latestLogRoll;
```
(This also ensures that a host that was active during last backup, but died shortly after, is marked as inactive, which makes conceptually sense to be classified as inactive.)
And that the removed block should be replaced by:
```
if (newTimestamp != null && currentLogTS > newTimestamp) {
newestLogs.add(currentLogFile);
}
```
This covers the scenario where the host is alive during the roll, but dies during the filesystem scan.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -83,12 +83,41 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = new HashMap<>();
+ Map<String, Long> latestLogRollByHost = readRegionServerLastLogRollResult();
+ for (Map.Entry<String, Long> entry : latestLogRollByHost.entrySet()) {
+ String host = entry.getKey();
+ long latestLogRoll = entry.getValue();
+ Long earliestTimestampToIncludeInBackup = previousTimestampMins.get(host);
+
+ boolean isInactive = earliestTimestampToIncludeInBackup != null
+ && earliestTimestampToIncludeInBackup > latestLogRoll;
+
+ if (isInactive) {
+ LOG.debug("Skipping inactive host {} from newTimestamps (boundary={} > latestLogRoll={})",
+ host, earliestTimestampToIncludeInBackup, latestLogRoll);
+ } else {
+ newTimestamps.put(host, latestLogRoll);
+ }
+ }
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
+ // Update boundaries based on WALs that will be backed up
+ for (String logFile : logList) {
+ Path logPath = new Path(logFile);
+ String logHost = BackupUtils.parseHostNameFromLogFile(logPath);
+ if (logHost != null) {
Review Comment:
I would drop this check, since it adds noise to the reading of the code. It will never trigger since it's guaranteed that `getLogFilesForNewBackup` will only return logfiles where the host can be parsed.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -83,12 +83,41 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = new HashMap<>();
+ Map<String, Long> latestLogRollByHost = readRegionServerLastLogRollResult();
+ for (Map.Entry<String, Long> entry : latestLogRollByHost.entrySet()) {
+ String host = entry.getKey();
+ long latestLogRoll = entry.getValue();
+ Long earliestTimestampToIncludeInBackup = previousTimestampMins.get(host);
+
+ boolean isInactive = earliestTimestampToIncludeInBackup != null
+ && earliestTimestampToIncludeInBackup > latestLogRoll;
+
+ if (isInactive) {
+ LOG.debug("Skipping inactive host {} from newTimestamps (boundary={} > latestLogRoll={})",
+ host, earliestTimestampToIncludeInBackup, latestLogRoll);
+ } else {
+ newTimestamps.put(host, latestLogRoll);
+ }
+ }
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
+ // Update boundaries based on WALs that will be backed up
+ for (String logFile : logList) {
+ Path logPath = new Path(logFile);
+ String logHost = BackupUtils.parseHostNameFromLogFile(logPath);
+ if (logHost != null) {
+ long logTs = BackupUtils.getCreationTime(logPath);
+ Long latestTimestampToIncludeInBackup = newTimestamps.get(logHost);
+ if (latestTimestampToIncludeInBackup == null || logTs > latestTimestampToIncludeInBackup) {
+ LOG.debug("Updating backup boundary for inactive host {}: timestamp={}", logHost, logTs);
+ newTimestamps.put(logHost, logTs);
+ }
+ }
+ }
return newTimestamps;
Review Comment:
Similar to my suggestion for FullTableBackupClient, a comment summarizing what ends up in `newTimestamps` would be useful for future code readers.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -181,6 +189,50 @@ public void execute() throws IOException {
// set overall backup status: complete. Here we make sure to complete the backup.
// After this checkpoint, even if entering cancel process, will let the backup finished
backupInfo.setState(BackupState.COMPLETE);
+
+ // Scan oldlogs for dead/decommissioned hosts and add their max WAL timestamps
+ // to newTimestamps. This ensures subsequent incremental backups won't try to back up
+ // WALs that are already covered by this full backup's snapshot.
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
+ String host = BackupUtils.parseHostNameFromLogFile(hostLogDir.getPath());
+ if (host == null) {
+ continue;
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(hostLogDir.getPath())));
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(oldLogDir)));
Review Comment:
Noticed that this also affects existing code in IncrementalBackupManager, so I just logged https://issues.apache.org/jira/browse/HBASE-29984 - no need to fix it in this PR.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
Review Comment:
Tests look 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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2727744503
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
ah admittedly, this is also not my area of expertise. my company also runs exclusively on cloud infrastructure, though that is changing in the future.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2682167982
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -181,6 +189,50 @@ public void execute() throws IOException {
// set overall backup status: complete. Here we make sure to complete the backup.
// After this checkpoint, even if entering cancel process, will let the backup finished
backupInfo.setState(BackupState.COMPLETE);
+
+ // Scan oldlogs for dead/decommissioned hosts and add their max WAL timestamps
+ // to newTimestamps. This ensures subsequent incremental backups won't try to back up
+ // WALs that are already covered by this full backup's snapshot.
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
+ String host = BackupUtils.parseHostNameFromLogFile(hostLogDir.getPath());
+ if (host == null) {
+ continue;
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(hostLogDir.getPath())));
+ }
+ allLogs.addAll(Arrays.asList(fs.listStatus(oldLogDir)));
Review Comment:
> Question: when exactly (and by what) are WALs moved to the oldWals folder (oldLogDir)?
They are moved asynchronously, after all edits to that WAL file have been closed and all edits have been replicated (if a peer is online)
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7582:
URL: https://github.com/apache/hbase/pull/7582#discussion_r2708626185
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -228,15 +263,6 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps,
} else if (currentLogTS > oldTimeStamp) {
resultLogFiles.add(currentLogFile);
}
-
- // It is possible that a host in .oldlogs is an obsolete region server
Review Comment:
> 1. In isolation, without the other changes to this PR, it means we'll never include the WAL files for a host that was removed for the cluster. Keep in mind, currentLogFile is an oldLog. So backup happens, new wals get created and moved to oldWAL dir, server goes offline, the boundary for that server is never updated, those WAL files will never be included in future backups and we lose data
Agreed. This is the case where `newTimestamp == null`, and I agree this case resulted in data loss in the original code, and should be fixed by not using `if (newTimestamp == null || currentLogTS > newTimestamp)` as-is.
> 2. In the context of this PR, we update the boundary for each host based on the latest WAL that was included in the backup for the host, so this logic actually completely irrelevant b/c newTimestamp will never be null
Again, agreed, for the case where `newTimestamp == null`. Conclusion: `if (newTimestamp == null || currentLogTS > newTimestamp)` is incorrect as-is.
However, my remark is about the case where `newTimestamp != null && currentLogTS > newTimestamp`.
It's probably an extremely unlikely scenario, but imagine the following in a system where some HDFS operations take longer than normal (in the context of this PR):
- A incremental backup is triggered, a log roll is triggered as a result, RegionServer X creates logfile L1.
- Right after the log roll was triggered, but before `getLogFilesForNewBackup` lists all the oldLogs, RegionServer X receives some data and goes offline again, causing an oldLog L2 for X to be written to storage.
- `newTimestamps` will contain an entry `X: timestamp(L1)`
- When `getLogFilesForNewBackup` lists the oldLogs, it will see L2, which is not added to `newestLogs` (because this PR removes the entire code block), hence L2 will be included in the backup.
My remark is that if, instead of deleting the check, we change it to
```
if (newTimestamp != null && currentLogTS > newTimestamp) {
newestLogs.add(currentLogFile);
}
```
then L2 would not be included in the current backup, but it would end up in the following one.
Which is the result that I would have expected.
--
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
Re: [PR] HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on PR #7582:
URL: https://github.com/apache/hbase/pull/7582#issuecomment-5783727221
This has become very stale, I'm going to close this out, and put up a new 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