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/05 17:46:28 UTC
[PR] HBASE-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
hgromer opened a new pull request, #7523:
URL: https://github.com/apache/hbase/pull/7523
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3627860402
: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 24s | | master passed |
| +1 :green_heart: | compile | 0m 34s | | master passed |
| +1 :green_heart: | checkstyle | 0m 14s | | master passed |
| +1 :green_heart: | spotbugs | 0m 31s | | master passed |
| +1 :green_heart: | spotless | 0m 52s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 18s | | the patch passed |
| +1 :green_heart: | compile | 0m 34s | | the patch passed |
| +1 :green_heart: | javac | 0m 34s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 0m 12s | [/results-checkstyle-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/7/artifact/yetus-general-check/output/results-checkstyle-hbase-backup.txt) | hbase-backup: The patch generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0) |
| +1 :green_heart: | spotbugs | 0m 42s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 12m 49s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| +1 :green_heart: | spotless | 0m 51s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 9s | | The patch does not generate ASF License warnings. |
| | | 32m 15s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/7/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 319c42e559c3 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 / ae69b33f416198c83734615913546f8eb2bab528 |
| 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-7523/7/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3631989201
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 31s | | 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 32s | | master passed |
| +1 :green_heart: | compile | 0m 31s | | master passed |
| +1 :green_heart: | checkstyle | 0m 12s | | master passed |
| +1 :green_heart: | spotbugs | 0m 33s | | master passed |
| +1 :green_heart: | spotless | 0m 51s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 1s | | the patch passed |
| +1 :green_heart: | compile | 0m 30s | | the patch passed |
| +1 :green_heart: | javac | 0m 30s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 0m 11s | [/results-checkstyle-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/8/artifact/yetus-general-check/output/results-checkstyle-hbase-backup.txt) | hbase-backup: The patch generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0) |
| +1 :green_heart: | spotbugs | 0m 35s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 12m 6s | | 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 10s | | The patch does not generate ASF License warnings. |
| | | 31m 7s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/8/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 8dadfb441c87 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 / 9ecf82ba8961fd3892a3a41dfc55e22dc77967e2 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 84 (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-7523/8/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3631831403
@DieterDP-ng Thanks for the eyes here, I think your comments should 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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618589868
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 12s | | 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 | 5m 47s | | master passed |
| +1 :green_heart: | compile | 0m 51s | | master passed |
| +1 :green_heart: | checkstyle | 0m 22s | | master passed |
| +1 :green_heart: | spotbugs | 0m 49s | | master passed |
| +1 :green_heart: | spotless | 1m 15s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 11s | | the patch passed |
| +1 :green_heart: | compile | 0m 48s | | the patch passed |
| -0 :warning: | javac | 0m 48s | [/results-compile-javac-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/4/artifact/yetus-general-check/output/results-compile-javac-hbase-backup.txt) | hbase-backup generated 1 new + 129 unchanged - 0 fixed = 130 total (was 129) |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 22s | | the patch passed |
| +1 :green_heart: | spotbugs | 1m 0s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 16m 25s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| +1 :green_heart: | spotless | 1m 11s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 19s | | The patch does not generate ASF License warnings. |
| | | 44m 46s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 6dacfebb2c46 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 / d1ea8bb0a21ee0572229c4ac8d22addaa02f37a3 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 111 (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-7523/4/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2599777144
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.master.BackupLogCleaner;
+import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.wal.AbstractFSWALProvider;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class BackupBoundaries {
+ private static final Logger LOG = LoggerFactory.getLogger(BackupBoundaries.class);
+ private static final BackupBoundaries EMPTY_BOUNDARIES =
+ new BackupBoundaries(new HashMap<>(), Long.MAX_VALUE);
+
+ // This map tracks, for every RegionServer, the least recent (= oldest / lowest timestamp)
+ // inclusion in any backup. In other words, it is the timestamp boundary up to which all backup
+ // roots have included the WAL in their backup.
+ private final Map<Address, Long> boundaries;
+ private final long oldestStartCode;
Review Comment:
This field deserves a description similar to above, since it's not just the oldest start code, it's the oldest start code of the most recent completed backup of each backup root.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
Review Comment:
I'm not opposed to splitting this logic in a separate class, but then it needs some more documentation to make clear it is meant for the backup log cleaner. Some suggestions:
- class javadoc
- `isDeletable(Path path)` -> `isDeletable(Path walLogPath)`
- `BackupBoundariesBuilder#add` -> `BackupBoundariesBuilder#addBackupTimestamps`
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.master.BackupLogCleaner;
+import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.wal.AbstractFSWALProvider;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class BackupBoundaries {
+ private static final Logger LOG = LoggerFactory.getLogger(BackupBoundaries.class);
+ private static final BackupBoundaries EMPTY_BOUNDARIES =
+ new BackupBoundaries(new HashMap<>(), Long.MAX_VALUE);
Review Comment:
You can reuse an existing empty collection object here: `Collections.emptyMap()`.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.master.BackupLogCleaner;
+import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.wal.AbstractFSWALProvider;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class BackupBoundaries {
+ private static final Logger LOG = LoggerFactory.getLogger(BackupBoundaries.class);
+ private static final BackupBoundaries EMPTY_BOUNDARIES =
+ new BackupBoundaries(new HashMap<>(), Long.MAX_VALUE);
+
+ // This map tracks, for every RegionServer, the least recent (= oldest / lowest timestamp)
+ // inclusion in any backup. In other words, it is the timestamp boundary up to which all backup
+ // roots have included the WAL in their backup.
+ private final Map<Address, Long> boundaries;
+ private final long oldestStartCode;
+
+ private BackupBoundaries(Map<Address, Long> boundaries, long oldestStartCode) {
+ this.boundaries = boundaries;
+ this.oldestStartCode = oldestStartCode;
+ }
+
+ public boolean isDeletable(Path path) {
+ try {
+ String hostname = BackupUtils.parseHostNameFromLogFile(path);
Review Comment:
The original code had a null check here. In this case, the NullPointer would be caught, but it's probably cleaner to actually check for nullness here already.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.master.BackupLogCleaner;
+import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.wal.AbstractFSWALProvider;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class BackupBoundaries {
+ private static final Logger LOG = LoggerFactory.getLogger(BackupBoundaries.class);
+ private static final BackupBoundaries EMPTY_BOUNDARIES =
+ new BackupBoundaries(new HashMap<>(), Long.MAX_VALUE);
+
+ // This map tracks, for every RegionServer, the least recent (= oldest / lowest timestamp)
+ // inclusion in any backup. In other words, it is the timestamp boundary up to which all backup
+ // roots have included the WAL in their backup.
+ private final Map<Address, Long> boundaries;
+ private final long oldestStartCode;
+
+ private BackupBoundaries(Map<Address, Long> boundaries, long oldestStartCode) {
+ this.boundaries = boundaries;
+ this.oldestStartCode = oldestStartCode;
+ }
+
+ public boolean isDeletable(Path path) {
+ try {
+ String hostname = BackupUtils.parseHostNameFromLogFile(path);
+ Address address = Address.fromString(hostname);
+ long ts = AbstractFSWALProvider.getTimestamp(path.getName());
+
+ if (!boundaries.containsKey(address)) {
+ boolean isDeletable = ts <= oldestStartCode;
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Boundary for {} not found. isDeletable = {} based on oldestStartCode = {} and WAL ts of {}",
+ path, isDeletable, oldestStartCode, ts);
+ }
+ return isDeletable;
+ }
+
+ long rollTs = boundaries.get(address);
+ if (ts <= rollTs) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "WAL cleanup time-boundary found for server {}: {}. Ok to delete older file: {}",
+ address.getHostName(), ts, path);
+ }
+ return true;
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("WAL cleanup time-boundary found for server {}: {}. Keeping younger file: {}",
+ address.getHostName(), rollTs, path);
+ }
+
+ return false;
+ } catch (Exception e) {
+ LOG.warn("Error occurred while filtering file: {}. Ignoring cleanup of this log", path, e);
+ return false;
+ }
+ }
+
+ public Map<Address, Long> getBoundaries() {
+ return boundaries;
+ }
+
+ public long getOldestStartCode() {
+ return oldestStartCode;
+ }
+
+ public static BackupBoundariesBuilder builder(Configuration conf) {
+ return new BackupBoundariesBuilder(conf);
+ }
+
+ public static class BackupBoundariesBuilder {
+ private final Map<Address, Long> boundaries = new HashMap<>();
+ private final Configuration conf;
+
+ private long oldestStartCode = 0L;
+
+ private BackupBoundariesBuilder(Configuration conf) {
+ this.conf = conf;
+ }
+
+ public BackupBoundariesBuilder add(String host, long hostLogRollTs, long backupStartCode)
+ throws IOException {
+ Address address = Address.fromString(host);
+ Long storedTs = boundaries.get(address);
+ if (storedTs == null || hostLogRollTs < storedTs) {
+ boundaries.put(address, hostLogRollTs);
+ }
+
+ if (oldestStartCode == 0L || oldestStartCode > backupStartCode) {
+ oldestStartCode = backupStartCode;
+ }
+
+ return this;
+ }
+
+ public BackupBoundaries build() {
+ if (boundaries.isEmpty()) {
+ return EMPTY_BOUNDARIES;
+ }
+
+ long buffer =
+ conf.getLong(BackupLogCleaner.TS_BUFFER_KEY, BackupLogCleaner.TS_BUFFER_DEFAULT);
Review Comment:
I'd move this out of the builder, and simply take is as a constructor argument. That way, you keep the usage and definition of that config key in the BackupLogCleaner.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.master.BackupLogCleaner;
+import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.wal.AbstractFSWALProvider;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class BackupBoundaries {
+ private static final Logger LOG = LoggerFactory.getLogger(BackupBoundaries.class);
+ private static final BackupBoundaries EMPTY_BOUNDARIES =
+ new BackupBoundaries(new HashMap<>(), Long.MAX_VALUE);
+
+ // This map tracks, for every RegionServer, the least recent (= oldest / lowest timestamp)
+ // inclusion in any backup. In other words, it is the timestamp boundary up to which all backup
+ // roots have included the WAL in their backup.
+ private final Map<Address, Long> boundaries;
+ private final long oldestStartCode;
+
+ private BackupBoundaries(Map<Address, Long> boundaries, long oldestStartCode) {
+ this.boundaries = boundaries;
+ this.oldestStartCode = oldestStartCode;
+ }
+
+ public boolean isDeletable(Path path) {
+ try {
+ String hostname = BackupUtils.parseHostNameFromLogFile(path);
+ Address address = Address.fromString(hostname);
+ long ts = AbstractFSWALProvider.getTimestamp(path.getName());
+
+ if (!boundaries.containsKey(address)) {
+ boolean isDeletable = ts <= oldestStartCode;
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Boundary for {} not found. isDeletable = {} based on oldestStartCode = {} and WAL ts of {}",
+ path, isDeletable, oldestStartCode, ts);
+ }
+ return isDeletable;
+ }
+
+ long rollTs = boundaries.get(address);
+ if (ts <= rollTs) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "WAL cleanup time-boundary found for server {}: {}. Ok to delete older file: {}",
+ address.getHostName(), ts, path);
+ }
+ return true;
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("WAL cleanup time-boundary found for server {}: {}. Keeping younger file: {}",
+ address.getHostName(), rollTs, path);
+ }
+
+ return false;
+ } catch (Exception e) {
+ LOG.warn("Error occurred while filtering file: {}. Ignoring cleanup of this log", path, e);
+ return false;
+ }
+ }
+
+ public Map<Address, Long> getBoundaries() {
+ return boundaries;
+ }
+
+ public long getOldestStartCode() {
+ return oldestStartCode;
+ }
+
+ public static BackupBoundariesBuilder builder(Configuration conf) {
+ return new BackupBoundariesBuilder(conf);
+ }
+
+ public static class BackupBoundariesBuilder {
+ private final Map<Address, Long> boundaries = new HashMap<>();
+ private final Configuration conf;
+
+ private long oldestStartCode = 0L;
Review Comment:
If you change this to `Long.MAX_VALUE`, you can simplify the `add` method a bit without any other changes to the result. Since if `add` is never called, you return `EMPTY_BOUNDARIES`
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.master.BackupLogCleaner;
+import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.wal.AbstractFSWALProvider;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class BackupBoundaries {
+ private static final Logger LOG = LoggerFactory.getLogger(BackupBoundaries.class);
+ private static final BackupBoundaries EMPTY_BOUNDARIES =
+ new BackupBoundaries(new HashMap<>(), Long.MAX_VALUE);
+
+ // This map tracks, for every RegionServer, the least recent (= oldest / lowest timestamp)
+ // inclusion in any backup. In other words, it is the timestamp boundary up to which all backup
+ // roots have included the WAL in their backup.
+ private final Map<Address, Long> boundaries;
+ private final long oldestStartCode;
+
+ private BackupBoundaries(Map<Address, Long> boundaries, long oldestStartCode) {
+ this.boundaries = boundaries;
+ this.oldestStartCode = oldestStartCode;
+ }
+
+ public boolean isDeletable(Path path) {
+ try {
+ String hostname = BackupUtils.parseHostNameFromLogFile(path);
+ Address address = Address.fromString(hostname);
+ long ts = AbstractFSWALProvider.getTimestamp(path.getName());
+
+ if (!boundaries.containsKey(address)) {
+ boolean isDeletable = ts <= oldestStartCode;
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Boundary for {} not found. isDeletable = {} based on oldestStartCode = {} and WAL ts of {}",
+ path, isDeletable, oldestStartCode, ts);
+ }
+ return isDeletable;
+ }
+
+ long rollTs = boundaries.get(address);
+ if (ts <= rollTs) {
Review Comment:
Both names are ambiguous, suggestion: `pathTs`, `backupTs`
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618563539
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 27s | | 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 48s | | master passed |
| +1 :green_heart: | compile | 0m 24s | | master passed |
| +1 :green_heart: | javadoc | 0m 19s | | 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 19s | | the patch passed |
| +1 :green_heart: | compile | 0m 22s | | the patch passed |
| +1 :green_heart: | javac | 0m 22s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 16s | | the patch passed |
| +1 :green_heart: | shadedjars | 6m 10s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 11m 8s | | hbase-backup in the patch passed. |
| | | 33m 42s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/4/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux 0879ebf69cae 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 / d1ea8bb0a21ee0572229c4ac8d22addaa02f37a3 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/4/testReport/ |
| Max. process+thread count | 3590 (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-7523/4/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618431735
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 12s | | 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 | 5m 23s | | master passed |
| +1 :green_heart: | compile | 0m 45s | | master passed |
| +1 :green_heart: | checkstyle | 0m 20s | | master passed |
| +1 :green_heart: | spotbugs | 0m 45s | | master passed |
| +1 :green_heart: | spotless | 1m 11s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| -1 :x: | mvninstall | 5m 8s | [/patch-mvninstall-root.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-general-check/output/patch-mvninstall-root.txt) | root in the patch failed. |
| -1 :x: | compile | 0m 17s | [/patch-compile-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-general-check/output/patch-compile-hbase-backup.txt) | hbase-backup in the patch failed. |
| -0 :warning: | javac | 0m 17s | [/patch-compile-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-general-check/output/patch-compile-hbase-backup.txt) | hbase-backup in the patch failed. |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 0m 21s | [/results-checkstyle-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-general-check/output/results-checkstyle-hbase-backup.txt) | hbase-backup: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
| -1 :x: | spotbugs | 0m 17s | [/patch-spotbugs-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-general-check/output/patch-spotbugs-hbase-backup.txt) | hbase-backup in the patch failed. |
| -1 :x: | hadoopcheck | 3m 42s | | The patch causes 10 errors with Hadoop v3.3.6. |
| -1 :x: | hadoopcheck | 7m 24s | | The patch causes 10 errors with Hadoop v3.4.1. |
| +1 :green_heart: | spotless | 1m 6s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 13s | | The patch does not generate ASF License warnings. |
| | | 25m 20s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 65c79c990618 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 / 2df506edc1e495055f207cc245fa39d2d06825dd |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| hadoopcheck | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-general-check/output/patch-javac-3.3.6.txt |
| hadoopcheck | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-general-check/output/patch-javac-3.4.1.txt |
| Max. process+thread count | 111 (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-7523/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618315172
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 2m 7s | | 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 | 5m 58s | | master passed |
| +1 :green_heart: | compile | 0m 50s | | master passed |
| +1 :green_heart: | checkstyle | 0m 23s | | master passed |
| +1 :green_heart: | spotbugs | 0m 49s | | master passed |
| +1 :green_heart: | spotless | 1m 16s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 22s | | the patch passed |
| +1 :green_heart: | compile | 0m 46s | | the patch passed |
| -0 :warning: | javac | 0m 46s | [/results-compile-javac-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/2/artifact/yetus-general-check/output/results-compile-javac-hbase-backup.txt) | hbase-backup generated 1 new + 129 unchanged - 0 fixed = 130 total (was 129) |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 20s | | the patch passed |
| +1 :green_heart: | spotbugs | 0m 54s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 16m 26s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| -1 :x: | spotless | 1m 0s | | patch has 21 errors when running spotless:check, run spotless:apply to fix. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 17s | | The patch does not generate ASF License warnings. |
| | | 46m 54s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux b8665ddee03c 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 / 26aa33e0714572da44ce3a4edd68d4dcd08184fc |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| spotless | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/2/artifact/yetus-general-check/output/patch-spotless.txt |
| Max. process+thread count | 115 (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-7523/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "krconv (via GitHub)" <gi...@apache.org>.
krconv commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2593819731
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -193,35 +200,140 @@ public void testBackupLogCleaner() throws Exception {
// Taking the minimum timestamp (= 2), this means all WALs preceding B3 can be deleted.
deletable = cleaner.getDeletableFiles(walFilesAfterB5);
assertEquals(toSet(walFilesAfterB2), toSet(deletable));
+ } finally {
+ TEST_UTIL.truncateTable(BackupSystemTable.getTableName(TEST_UTIL.getConfiguration())).close();
}
}
- private Set<FileStatus> mergeAsSet(Collection<FileStatus> toCopy, Collection<FileStatus> toAdd) {
- Set<FileStatus> result = new LinkedHashSet<>(toCopy);
- result.addAll(toAdd);
- return result;
+ @Test
+ public void testDoesNotDeleteWALsFromNewServers() throws Exception {
Review Comment:
Wow that's an awesome test
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2594066946
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -153,19 +159,34 @@ public Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
return files;
}
- Map<Address, Long> serverToPreservationBoundaryTs;
+ long oldestStartCode = Long.MAX_VALUE;
try {
- try (BackupManager backupManager = new BackupManager(conn, getConf())) {
- serverToPreservationBoundaryTs =
- serverToPreservationBoundaryTs(backupManager.getBackupHistory(true));
+ try (BackupSystemTable sysTable = new BackupSystemTable(conn)) {
+ Set<String> roots = sysTable.getBackupHistory(true).stream()
+ .map(BackupInfo::getBackupRootDir).collect(Collectors.toSet());
+ if (roots.isEmpty()) {
+ LOG.info("No backups found, can delete all files");
Review Comment:
👍
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
Ah thank you good catch
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618128559
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 47s | | 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 | 5m 29s | | master passed |
| +1 :green_heart: | compile | 1m 19s | | master passed |
| +1 :green_heart: | checkstyle | 0m 33s | | master passed |
| +1 :green_heart: | spotbugs | 1m 4s | | master passed |
| +1 :green_heart: | spotless | 1m 33s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 54s | | the patch passed |
| +1 :green_heart: | compile | 0m 56s | | the patch passed |
| -0 :warning: | javac | 0m 56s | [/results-compile-javac-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/1/artifact/yetus-general-check/output/results-compile-javac-hbase-backup.txt) | hbase-backup generated 1 new + 129 unchanged - 0 fixed = 130 total (was 129) |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 31s | | the patch passed |
| +1 :green_heart: | spotbugs | 0m 59s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 20m 19s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| +1 :green_heart: | spotless | 0m 51s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 15s | | The patch does not generate ASF License warnings. |
| | | 50m 5s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux b1b9fb7b241e 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 / ac00ff6fc0d376b37f89b595a867e803e69969e6 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 133 (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-7523/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618871268
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 2m 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 | 6m 32s | | master passed |
| +1 :green_heart: | compile | 0m 46s | | master passed |
| +1 :green_heart: | checkstyle | 0m 22s | | master passed |
| +1 :green_heart: | spotbugs | 0m 46s | | master passed |
| +1 :green_heart: | spotless | 1m 11s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 1s | | the patch passed |
| +1 :green_heart: | compile | 0m 43s | | the patch passed |
| +1 :green_heart: | javac | 0m 43s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 21s | | the patch passed |
| +1 :green_heart: | spotbugs | 0m 55s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 14m 52s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| +1 :green_heart: | spotless | 1m 4s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 17s | | The patch does not generate ASF License warnings. |
| | | 44m 41s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/5/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux b98f2398dbe0 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 / 87342e942e48422a91313c5dc9a166ebd07ad5ae |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 111 (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-7523/5/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2594066167
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -248,6 +241,7 @@ protected static boolean canDeleteFile(Map<Address, Long> addressToBoundaryTs, P
private static boolean isHMasterWAL(Path path) {
String fn = path.getName();
return fn.startsWith(WALProcedureStore.LOG_PREFIX)
- || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX);
+ || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX)
+ || path.toString().contains(MasterRegionFactory.MASTER_STORE_DIR);
Review Comment:
There's a test that already exists `testCleansUpArchivedHMasterWal`. It started failing with the code change and I realized it was a happy coincidence of this code path
```java
if (!addressToBoundaryTs.containsKey(walServerAddress)) {
if (LOG.isDebugEnabled()) {
LOG.debug("No cleanup WAL time-boundary found for server: {}. Ok to delete file: {}",
walServerAddress.getHostName(), path);
}
return true;
}
```
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2597856808
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
My suggestion would be to change the following in the original code:
```
protected static boolean canDeleteFile(Map<Address, Long> addressToBoundaryTs, Path path) {
...
if (!addressToBoundaryTs.containsKey(walServerAddress)) {
if (LOG.isDebugEnabled()) {
LOG.debug("No cleanup WAL time-boundary found for server: {}. Ok to delete file: {}",
walServerAddress.getHostName(), path);
}
return true;
}
...
```
to:
```
// oldestStartCode calculated as suggested in this PR
protected static boolean canDeleteFile(Map<Address, Long> addressToBoundaryTs, long oldestStartCode, Path path) {
...
if (!addressToBoundaryTs.containsKey(walServerAddress)) {
long walTimestamp = AbstractFSWALProvider.getTimestamp(path.getName());
if (walTimestamp <= oldestStartCode) {
if (LOG.isDebugEnabled()) {
LOG.debug(...);
}
return true;
}
}
...
```
So this uses the specialized logic from the original solution for RS still in use, and your fix for cases where RS have since appeared/disappeared.
I agree complexity is a bad thing, but I feel it is manageable in the log cleaner, and brings relevant advantages. I won't make a hard stance about this. It's just that I would expect that at some point someone in the future would end up trying to re-introduce more specialized cleaning logic because they're seeing log files sticking around longer than required.
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "ndimiduk (via GitHub)" <gi...@apache.org>.
ndimiduk merged PR #7523:
URL: https://github.com/apache/hbase/pull/7523
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618419814
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 27s | | 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 15s | | master passed |
| +1 :green_heart: | compile | 0m 22s | | master passed |
| +1 :green_heart: | javadoc | 0m 16s | | master passed |
| +1 :green_heart: | shadedjars | 6m 15s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| -1 :x: | mvninstall | 3m 18s | [/patch-mvninstall-root.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-jdk17-hadoop3-check/output/patch-mvninstall-root.txt) | root in the patch failed. |
| -1 :x: | compile | 0m 14s | [/patch-compile-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-jdk17-hadoop3-check/output/patch-compile-hbase-backup.txt) | hbase-backup in the patch failed. |
| -0 :warning: | javac | 0m 13s | [/patch-compile-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-jdk17-hadoop3-check/output/patch-compile-hbase-backup.txt) | hbase-backup in the patch failed. |
| +1 :green_heart: | javadoc | 0m 16s | | the patch passed |
| +1 :green_heart: | shadedjars | 6m 12s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| -1 :x: | unit | 0m 13s | [/patch-unit-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-jdk17-hadoop3-check/output/patch-unit-hbase-backup.txt) | hbase-backup in the patch failed. |
| | | 21m 49s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux 12b668a16f7f 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 / 2df506edc1e495055f207cc245fa39d2d06825dd |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/3/testReport/ |
| Max. process+thread count | 133 (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-7523/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3631994266
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 27s | | 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 37s | | master passed |
| +1 :green_heart: | compile | 0m 19s | | master passed |
| +1 :green_heart: | javadoc | 0m 16s | | master passed |
| +1 :green_heart: | shadedjars | 6m 12s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 8s | | 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 11s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 11m 1s | | hbase-backup in the patch passed. |
| | | 32m 44s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/8/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux a3af5bca5bea 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 / 9ecf82ba8961fd3892a3a41dfc55e22dc77967e2 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/8/testReport/ |
| Max. process+thread count | 3524 (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-7523/8/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2593823522
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
Method `serverToPreservationBoundaryTs` has become unused, so it should be removed.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
You've simplified the logic to determine what can be deleted, but I can see this backfiring in the following case.
Imagine a cluster with 2 backup roots (though multi-root doesn't work properly yet, assume it does). Root A has a backup of a single table, and is effectively not used. Root B is actually used and gets a new backup every day.
Because the timestamp of A will remain unchanged, it will prohibit the deletion of regions that are only covered by B.
Hence, I'd prefer the old logic path (with a fix for the case of missing timestamps).
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -248,6 +241,7 @@ protected static boolean canDeleteFile(Map<Address, Long> addressToBoundaryTs, P
private static boolean isHMasterWAL(Path path) {
String fn = path.getName();
return fn.startsWith(WALProcedureStore.LOG_PREFIX)
- || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX);
+ || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX)
+ || path.toString().contains(MasterRegionFactory.MASTER_STORE_DIR);
Review Comment:
Do you know the format of the `Path`s entering here? Is there a chance that it contains a table name, in which case a table called "MyMasterData" would be classified as a master WAL.
Might be safer to add path boundaries: `.contains("/" + MasterRegionFactory.MASTER_STORE_DIR + "/")`
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -248,6 +241,7 @@ protected static boolean canDeleteFile(Map<Address, Long> addressToBoundaryTs, P
private static boolean isHMasterWAL(Path path) {
String fn = path.getName();
return fn.startsWith(WALProcedureStore.LOG_PREFIX)
- || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX);
+ || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX)
+ || path.toString().contains(MasterRegionFactory.MASTER_STORE_DIR);
Review Comment:
Add a test for this.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
Another interesting case is when I were to take a backup of a single table, this shouldn't affect the cleanup of regions of tables not covered by the backup.
Do we have a way to know whether a file offered to the cleaner is part of a table covered by backups? The format of the logs and the link to regions/tables is still spotty to me.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -153,19 +159,34 @@ public Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
return files;
}
- Map<Address, Long> serverToPreservationBoundaryTs;
+ long oldestStartCode = Long.MAX_VALUE;
try {
- try (BackupManager backupManager = new BackupManager(conn, getConf())) {
- serverToPreservationBoundaryTs =
- serverToPreservationBoundaryTs(backupManager.getBackupHistory(true));
+ try (BackupSystemTable sysTable = new BackupSystemTable(conn)) {
+ Set<String> roots = sysTable.getBackupHistory(true).stream()
+ .map(BackupInfo::getBackupRootDir).collect(Collectors.toSet());
+ if (roots.isEmpty()) {
+ LOG.info("No backups found, can delete all files");
Review Comment:
This will spam logs, suggest to change to debug logging.
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2602297829
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.master.BackupLogCleaner;
+import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.wal.AbstractFSWALProvider;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class BackupBoundaries {
+ private static final Logger LOG = LoggerFactory.getLogger(BackupBoundaries.class);
+ private static final BackupBoundaries EMPTY_BOUNDARIES =
+ new BackupBoundaries(new HashMap<>(), Long.MAX_VALUE);
+
+ // This map tracks, for every RegionServer, the least recent (= oldest / lowest timestamp)
+ // inclusion in any backup. In other words, it is the timestamp boundary up to which all backup
+ // roots have included the WAL in their backup.
+ private final Map<Address, Long> boundaries;
+ private final long oldestStartCode;
+
+ private BackupBoundaries(Map<Address, Long> boundaries, long oldestStartCode) {
+ this.boundaries = boundaries;
+ this.oldestStartCode = oldestStartCode;
+ }
+
+ public boolean isDeletable(Path path) {
+ try {
+ String hostname = BackupUtils.parseHostNameFromLogFile(path);
Review Comment:
Added the null check
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618847764
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 28s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | | 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 49s | | master passed |
| +1 :green_heart: | compile | 0m 25s | | master passed |
| +1 :green_heart: | javadoc | 0m 19s | | master passed |
| +1 :green_heart: | shadedjars | 6m 17s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 19s | | the patch passed |
| +1 :green_heart: | compile | 0m 22s | | the patch passed |
| +1 :green_heart: | javac | 0m 22s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 17s | | the patch passed |
| +1 :green_heart: | shadedjars | 6m 11s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 11m 22s | | hbase-backup in the patch passed. |
| | | 33m 54s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/5/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux b7654d2d2a70 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 / 87342e942e48422a91313c5dc9a166ebd07ad5ae |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/5/testReport/ |
| Max. process+thread count | 3634 (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-7523/5/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618070648
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 31s | | 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 46s | | master passed |
| +1 :green_heart: | compile | 0m 23s | | master passed |
| +1 :green_heart: | javadoc | 0m 19s | | master passed |
| +1 :green_heart: | shadedjars | 6m 16s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 21s | | the patch passed |
| +1 :green_heart: | compile | 0m 22s | | the patch passed |
| +1 :green_heart: | javac | 0m 22s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 16s | | the patch passed |
| +1 :green_heart: | shadedjars | 6m 15s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 10m 55s | | hbase-backup in the patch passed. |
| | | 35m 46s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux cc04022bfbfd 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 / ac00ff6fc0d376b37f89b595a867e803e69969e6 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/1/testReport/ |
| Max. process+thread count | 3658 (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-7523/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3618268639
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 27s | | 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 13s | | master passed |
| +1 :green_heart: | compile | 0m 22s | | master passed |
| +1 :green_heart: | javadoc | 0m 16s | | master passed |
| +1 :green_heart: | shadedjars | 6m 12s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 19s | | the patch passed |
| +1 :green_heart: | compile | 0m 21s | | the patch passed |
| +1 :green_heart: | javac | 0m 21s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 16s | | the patch passed |
| +1 :green_heart: | shadedjars | 6m 12s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 11m 2s | | hbase-backup in the patch passed. |
| | | 32m 44s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/2/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux 2f498e41c1bd 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 / 26aa33e0714572da44ce3a4edd68d4dcd08184fc |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/2/testReport/ |
| Max. process+thread count | 3534 (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-7523/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2598988239
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -248,6 +241,7 @@ protected static boolean canDeleteFile(Map<Address, Long> addressToBoundaryTs, P
private static boolean isHMasterWAL(Path path) {
String fn = path.getName();
return fn.startsWith(WALProcedureStore.LOG_PREFIX)
- || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX);
+ || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX)
+ || path.toString().contains(MasterRegionFactory.MASTER_STORE_DIR);
Review Comment:
You're correct here, we should be more defensive, thanks for the comment, fixed
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3627662838
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 17s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | | 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 | 5m 12s | | master passed |
| +1 :green_heart: | compile | 0m 27s | | master passed |
| +1 :green_heart: | javadoc | 0m 28s | | master passed |
| +1 :green_heart: | shadedjars | 9m 30s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 40s | | the patch passed |
| +1 :green_heart: | compile | 0m 34s | | the patch passed |
| +1 :green_heart: | javac | 0m 34s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 22s | | the patch passed |
| +1 :green_heart: | shadedjars | 9m 35s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 16m 36s | | hbase-backup in the patch passed. |
| | | 48m 59s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/6/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux 30811a571a6b 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 / 2c22144e482d82a990bc3208d40bff3f5be9649a |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/6/testReport/ |
| Max. process+thread count | 3603 (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-7523/6/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3617882129
cc @rmdmattingly @ndimiduk @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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2593766937
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -193,35 +200,131 @@ public void testBackupLogCleaner() throws Exception {
// Taking the minimum timestamp (= 2), this means all WALs preceding B3 can be deleted.
deletable = cleaner.getDeletableFiles(walFilesAfterB5);
assertEquals(toSet(walFilesAfterB2), toSet(deletable));
+ } finally {
+ TEST_UTIL.truncateTable(BackupSystemTable.getTableName(TEST_UTIL.getConfiguration())).close();
}
}
- private Set<FileStatus> mergeAsSet(Collection<FileStatus> toCopy, Collection<FileStatus> toAdd) {
- Set<FileStatus> result = new LinkedHashSet<>(toCopy);
- result.addAll(toAdd);
- return result;
+ @Test
+ public void testDoesNotDeleteWALsFromNewServers() throws Exception {
+ Path backupRoot1 = new Path(BACKUP_ROOT_DIR, "backup1");
+ List<TableName> tableSetFull = List.of(table1, table2, table3, table4);
+
+ try (BackupSystemTable systemTable = new BackupSystemTable(TEST_UTIL.getConnection())) {
+ LOG.info("Creating initial backup B1");
+ String backupIdB1 = backupTables(BackupType.FULL, tableSetFull, backupRoot1.toString());
+ assertTrue(checkSucceeded(backupIdB1));
+
+ List<FileStatus> walsAfterB1 = getListOfWALFiles(TEST_UTIL.getConfiguration());
+ LOG.info("WALs after B1: {}", walsAfterB1.size());
+
+ String startCodeStr = systemTable.readBackupStartCode(backupRoot1.toString());
+ long b1StartCode = Long.parseLong(startCodeStr);
+ LOG.info("B1 startCode: {}", b1StartCode);
+
+ // Add a new RegionServer to the cluster
+ LOG.info("Adding new RegionServer to cluster");
+ HRegionServer newRS = TEST_UTIL.getMiniHBaseCluster().startRegionServer().getRegionServer();
+ ServerName newServerName = newRS.getServerName();
+ LOG.info("New RegionServer started: {}", newServerName);
+
+ // Move a region to the new server to ensure it creates a WAL
+ List<RegionInfo> regions = TEST_UTIL.getAdmin().getRegions(table1);
+ RegionInfo regionToMove = regions.get(0);
+
+ LOG.info("Moving region {} to new server {}", regionToMove.getEncodedName(), newServerName);
+ TEST_UTIL.getAdmin().move(regionToMove.getEncodedNameAsBytes(), newServerName);
+
+ TEST_UTIL.waitFor(30000, () -> {
+ try {
+ HRegionLocation location = TEST_UTIL.getConnection().getRegionLocator(table1)
+ .getRegionLocation(regionToMove.getStartKey());
+ return location.getServerName().equals(newServerName);
+ } catch (IOException e) {
+ return false;
+ }
+ });
+
+ // Write some data to trigger WAL creation on the new server
+ try (Table t1 = TEST_UTIL.getConnection().getTable(table1)) {
+ for (int i = 0; i < 100; i++) {
+ Put p = new Put(Bytes.toBytes("newserver-row-" + i));
+ p.addColumn(famName, qualName, Bytes.toBytes("val" + i));
+ t1.put(p);
+ }
+ }
+ TEST_UTIL.getAdmin().flushRegion(regionToMove.getEncodedNameAsBytes());
+
+ List<FileStatus> walsAfterNewServer = getListOfWALFiles(TEST_UTIL.getConfiguration());
+ LOG.info("WALs after adding new server: {}", walsAfterNewServer.size());
+ assertTrue("Should have more WALs after new server",
+ walsAfterNewServer.size() > walsAfterB1.size());
+
+ List<FileStatus> newServerWALs = new ArrayList<>(walsAfterNewServer);
+ newServerWALs.removeAll(walsAfterB1);
+ assertFalse("Should have WALs from new server", newServerWALs.isEmpty());
+
+ BackupLogCleaner cleaner = new BackupLogCleaner();
+ cleaner.setConf(TEST_UTIL.getConfiguration());
+ cleaner.init(Map.of(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster()));
+
+ Set<FileStatus> deletable =
+ ImmutableSet.copyOf(cleaner.getDeletableFiles(walsAfterNewServer));
+ for (FileStatus newWAL : newServerWALs) {
+ assertFalse("WAL from new server should NOT be deletable: " + newWAL.getPath(),
Review Comment:
This assertion fails without the patch, which proves the data loss scenario
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2594061417
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
I'd like to push back here a bit, and reconsider the tradeoffs between the complexities and what we're risking by having a more complicated design for WAL retention.
I would prefer to have a solution that is easier to reason about, and store WAL files for longer than necessary rather than an implementation that is harder to debug and can lead to complicated edge cases.
I'm open to suggestions, but I don't think solving this at a RS level is simple, I explored that path and quickly foudn myself having to re-architect the backup system table.
For example, the obvious solution is to [retain WALs for addresses we aren't storing](https://github.com/apache/hbase/blob/master/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java#L224). But what if the server joined was removed from the cluster in between backups, how do we make sure we _do_ end up cleaning up the WAL files eventually?
We could have the backup add the host to it's boundary map, but what's the timestamp to set? The backup system table does _not_ include the host start code, so we can't set the entry to a ts of `Long.MAX_VALUE` because if the host does eventually rejoin the cluster, that'd be problematic. We can also have a race where the host joins the cluster exactly as we're adding to the boundary mapping.
To be quite frank, after spending much time with the modern backup system as a whole, I'd much rather err on the side of simplicity, choosing to retain WALs for a bit longer than is necessary rather than continue to make an already complicated system more complicated.
Happy to hear your thoughts, but I think (with a fix for the case of missing timestamps) is a non-trivial statement from what I've seen.
> Do we have a way to know whether a file offered to the cleaner is part of a table covered by backups
We don't, other than by reading the WAL. A WAL file can contain edits from mulitple tables.
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "hgromer (via GitHub)" <gi...@apache.org>.
hgromer commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2598959044
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
This makes sense to me. I think it's still safe given the addition of the new start code, and avoids WAL retention for hosts that are added to, and then removed from the cluster between backups.
Let me know what you think of the current implementation.
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3627986848
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 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 | 5m 28s | | master passed |
| +1 :green_heart: | compile | 0m 36s | | master passed |
| +1 :green_heart: | javadoc | 0m 26s | | master passed |
| +1 :green_heart: | shadedjars | 9m 6s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 49s | | the patch passed |
| +1 :green_heart: | compile | 0m 32s | | the patch passed |
| +1 :green_heart: | javac | 0m 32s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 20s | | the patch passed |
| +1 :green_heart: | shadedjars | 10m 26s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 17m 31s | | hbase-backup in the patch passed. |
| | | 50m 48s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/7/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux 8142368e27b4 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 / ae69b33f416198c83734615913546f8eb2bab528 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/7/testReport/ |
| Max. process+thread count | 3586 (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-7523/7/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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "DieterDP-ng (via GitHub)" <gi...@apache.org>.
DieterDP-ng commented on code in PR #7523:
URL: https://github.com/apache/hbase/pull/7523#discussion_r2599837824
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
General flow looks good. Added some minor points of feedback.
--
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-29744: Data loss scenario for WAL files belonging to RS added between backups [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #7523:
URL: https://github.com/apache/hbase/pull/7523#issuecomment-3627571291
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 2m 19s | | 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 | 4m 2s | | master passed |
| +1 :green_heart: | compile | 0m 36s | | master passed |
| +1 :green_heart: | checkstyle | 0m 15s | | master passed |
| +1 :green_heart: | spotbugs | 0m 35s | | master passed |
| +1 :green_heart: | spotless | 0m 59s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 19s | | the patch passed |
| +1 :green_heart: | compile | 0m 36s | | the patch passed |
| +1 :green_heart: | javac | 0m 36s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 0m 13s | [/results-checkstyle-hbase-backup.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/6/artifact/yetus-general-check/output/results-checkstyle-hbase-backup.txt) | hbase-backup: The patch generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0) |
| +1 :green_heart: | spotbugs | 0m 40s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 12m 56s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.1. |
| +1 :green_heart: | spotless | 0m 52s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 13s | | The patch does not generate ASF License warnings. |
| | | 35m 27s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7523/6/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/7523 |
| JIRA Issue | HBASE-29744 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 6956eedc6736 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 / 2c22144e482d82a990bc3208d40bff3f5be9649a |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 85 (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-7523/6/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