anmolnar commented on code in PR #8845:
URL: https://github.com/apache/hbase/pull/8845#discussion_r4530309695
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/HBackupFileSystem.java:
##########
@@ -93,6 +94,46 @@ public static Path getBackupPath(String backupRootDir,
String backupId) {
return new Path(backupRootDir + Path.SEPARATOR + backupId);
}
+ /**
+ * Write a completion marker file to signal a backup has completed
successfully. This is a
+ * best-effort operation: if the write fails, the backup itself is still
considered complete (the
+ * system table is the source of truth). The marker file is intended for
external consumers (e.g.,
+ * another cluster) that cannot access the originating cluster's
BackupSystemTable. Absence of the
+ * marker should not be interpreted as backup failure; consumers should fall
back to checking for
+ * a valid BackupManifest.
+ * @param conf configuration
+ * @param backupRootDir backup root directory
+ * @param backupId backup id
+ */
+ public static void writeBackupCompleteMarker(Configuration conf, String
backupRootDir,
+ String backupId) {
+ Path backupDir = getBackupPath(backupRootDir, backupId);
+ Path markerPath = new Path(backupDir, BACKUP_COMPLETE_FILE_NAME);
+ try {
+ FileSystem fs = markerPath.getFileSystem(conf);
+ fs.create(markerPath, true).close();
+ LOG.info("Backup completion marker file written to: {}", markerPath);
+ } catch (IOException e) {
+ LOG.warn("Failed to write {} marker file to backup directory: {}",
BACKUP_COMPLETE_FILE_NAME,
+ backupDir, e);
+ }
+ }
+
+ /**
+ * Check whether a backup completion marker file exists for the given backup.
+ * @param conf configuration
+ * @param backupRootDir backup root directory
+ * @param backupId backup id
+ * @return true if the marker file exists
+ * @throws IOException if the file system cannot be accessed
+ */
+ public static boolean isBackupComplete(Configuration conf, String
backupRootDir, String backupId)
+ throws IOException {
+ Path markerPath = new Path(getBackupPath(backupRootDir, backupId),
BACKUP_COMPLETE_FILE_NAME);
+ FileSystem fs = markerPath.getFileSystem(conf);
+ return fs.exists(markerPath);
+ }
Review Comment:
The method is used for testing only. It can be package-private.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestFullBackupMarkerFailure.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.apache.hadoop.hbase.backup.HBackupFileSystem.getBackupPath;
+import static
org.apache.hadoop.hbase.backup.TestBackupUtils.assertBackupCompleteFileDoesNotExist;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FilterFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.impl.FullTableBackupClient;
+import org.apache.hadoop.hbase.backup.impl.TableBackupClient;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hdfs.DistributedFileSystem;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Tag(LargeTests.TAG)
+public class TestFullBackupMarkerFailure extends TestBackupBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestFullBackupMarkerFailure.class);
+
+ @Test
+ public void testFullBackupSucceedsWhenMarkerWriteFails() throws Exception {
+ // Use a special backup class that will force a failure when trying to
write the
+ // .backup.complete marker file
+ conf1.set(TableBackupClient.BACKUP_CLIENT_IMPL_CLASS,
+ FullTableBackupClientWithMarkerFailure.class.getName());
+
+ // Track the log to verify the expected WARN message occurs when the
.backup.complete marker
+ // file cannot be created. Use fully-qualified Log4j2 names to avoid
banned-import enforcer rule
+ org.apache.logging.log4j.core.Appender mockAppender =
+ mock(org.apache.logging.log4j.core.Appender.class);
+ when(mockAppender.getName()).thenReturn("mockAppender");
+ when(mockAppender.isStarted()).thenReturn(true);
+ org.apache.logging.log4j.core.Logger log4jLogger =
+ (org.apache.logging.log4j.core.Logger)
org.apache.logging.log4j.LogManager
+ .getLogger(HBackupFileSystem.class);
+ log4jLogger.addAppender(mockAppender);
+
+ try {
+ String backupId = fullTableBackup(List.of(table1));
+
+ // The backup should still succeed despite failure to write the
.backup.complete marker file
+ assertTrue(checkSucceeded(backupId));
+ assertBackupCompleteFileDoesNotExist(conf1, BACKUP_ROOT_DIR, backupId);
+
+ Path backupDir = getBackupPath(BACKUP_ROOT_DIR, backupId);
+ String expectedLogMsg =
+ String.format("Failed to write %s marker file to backup directory: %s",
+ HBackupFileSystem.BACKUP_COMPLETE_FILE_NAME, backupDir);
+
+ // Verify the expected WARN message was logged
+ verify(mockAppender, atLeastOnce())
+ .append(argThat((org.apache.logging.log4j.core.LogEvent event) ->
event.getLevel()
+ .equals(org.apache.logging.log4j.Level.WARN)
+ &&
event.getMessage().getFormattedMessage().contains(expectedLogMsg)));
Review Comment:
Here again.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestFullBackupMarkerFailure.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.apache.hadoop.hbase.backup.HBackupFileSystem.getBackupPath;
+import static
org.apache.hadoop.hbase.backup.TestBackupUtils.assertBackupCompleteFileDoesNotExist;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FilterFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.backup.impl.FullTableBackupClient;
+import org.apache.hadoop.hbase.backup.impl.TableBackupClient;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hdfs.DistributedFileSystem;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Tag(LargeTests.TAG)
+public class TestFullBackupMarkerFailure extends TestBackupBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestFullBackupMarkerFailure.class);
+
+ @Test
+ public void testFullBackupSucceedsWhenMarkerWriteFails() throws Exception {
+ // Use a special backup class that will force a failure when trying to
write the
+ // .backup.complete marker file
+ conf1.set(TableBackupClient.BACKUP_CLIENT_IMPL_CLASS,
+ FullTableBackupClientWithMarkerFailure.class.getName());
+
+ // Track the log to verify the expected WARN message occurs when the
.backup.complete marker
+ // file cannot be created. Use fully-qualified Log4j2 names to avoid
banned-import enforcer rule
+ org.apache.logging.log4j.core.Appender mockAppender =
+ mock(org.apache.logging.log4j.core.Appender.class);
+ when(mockAppender.getName()).thenReturn("mockAppender");
+ when(mockAppender.isStarted()).thenReturn(true);
+ org.apache.logging.log4j.core.Logger log4jLogger =
+ (org.apache.logging.log4j.core.Logger)
org.apache.logging.log4j.LogManager
+ .getLogger(HBackupFileSystem.class);
+ log4jLogger.addAppender(mockAppender);
Review Comment:
I don't think you need to validate a log message. The important thing is
that the backup will succeed regardless of the file creation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
