vinayakphegde commented on code in PR #6710:
URL: https://github.com/apache/hbase/pull/6710#discussion_r1968919345
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupCommands.java:
##########
@@ -339,14 +341,24 @@ public void execute() throws IOException {
boolean ignoreChecksum = cmdline.hasOption(OPTION_IGNORECHECKSUM);
+ BackupType backupType = BackupType.valueOf(args[1].toUpperCase());
+ List<TableName> tableNameList = null;
+ if (tables != null) {
+ tableNameList =
Lists.newArrayList(BackupUtils.parseTableNames(tables));
+ }
+ boolean continuousBackup =
cmdline.hasOption(OPTION_ENABLE_CONTINUOUS_BACKUP);
+ if (continuousBackup && !BackupType.FULL.equals(backupType)) {
+ System.out.println("ERROR: Continuous backup can Only be specified for
Full Backup");
+ printUsage();
+ throw new IOException(INCORRECT_USAGE);
+ }
Review Comment:
Yeah, we discussed this and agreed to go with a new flag for the full backup.
- A continuous backup just takes a full backup and starts the WAL
replication process. This happens only once—after that, we only need to take
full backups. So, I don’t see a need for a separate backup type.
- Also, introducing a new type would unnecessarily differentiate full and
continuous backups.
- In the future, we want to phase out the old incremental backup
implementation. The plan is to use WALs replicated through continuous backup
for incremental backups. So eventually, we might just have FULL and INCREMENTAL
backups, where incremental backups rely on continuously replicated WALs.
- From a code perspective, our backup code has a lot of(almost everywhere)
if-else checks for backup types. Adding a new type would mean modifying all of
them.
What do you think?
--
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]
