HBASE-29133: Implement "pitr" Command for Point-in-Time Restore #6717

vinayakphegde · 2025-02-24T16:54:48Z

This PR introduces the pitr command to restore tables from the most recent backup before --to-datetime and replay WAL logs for changes made after the last backup.

Command Usage:

hbase pitr 
    [-t <table_name[,table_name]>] 
    [-s <backup_set_name>] 
    [-q <name>] 
    [-c] 
    [-m <target_tables>] 
    [-o] 
    [--to-datetime <end_time>]

kgeisz · 2025-04-28T23:39:17Z

hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java

+          } else {
+            // This server is no longer active (e.g., RS moved or removed); skip
+            if (LOG.isDebugEnabled()) {
+              LOG.debug("Skipping replication marker timestamp for invalid server: {}", server);


Is theLOG.isDebugEnabled() check necessary here? Shouldn't log4j just not log this message if the log level is set to INFO or higher?

Correct, it's unnecessary here. We typically use the conditional check when building a complex string or performing expensive operations, which isn't the case in this line.

anmolnar

Overall lgtm, but surprising how low the level of unit tests' granularity is: for instance BackupAdminImpl is such a big class and there's no corresponding test class. It has a lot of private methods which could be tested as "units" if they were package private and we had a BackupAdminTest class. Please consider that.

anmolnar · 2025-05-27T23:09:12Z

hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java

    return request;
  }

+  public static PointInTimeRestoreRequest createPointInTimeRestoreRequest(String backupRootDir,


nit: Since you have Builder implemented for PointInTimeRestoreRequest, this method is redundant. Creating a new request is already a one-liner.

anmolnar · 2025-05-27T23:59:31Z

hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java

+          } else {
+            // This server is no longer active (e.g., RS moved or removed); skip
+            if (LOG.isDebugEnabled()) {
+              LOG.debug("Skipping replication marker timestamp for invalid server: {}", server);


"inactive" maybe instead of "invalid" - could be slightly less misleading, these servers are probably not invalid

anmolnar · 2025-05-28T01:00:16Z

@vinayakphegde You also need to rebase the patch. Some flaky tests have already been fixed on the feature branch.

vinayakphegde · 2025-05-28T11:26:10Z

Overall lgtm, but surprising how low the level of unit tests' granularity is: for instance BackupAdminImpl is such a big class and there's no corresponding test class. It has a lot of private methods which could be tested as "units" if they were package private and we had a BackupAdminTest class. Please consider that.

True, there isn’t a separate test class specifically for BackupAdminImpl. However, if you look at the overall structure, the core implementation resides in BackupAdminImpl, while the other classes primarily handle input preparation (including validation) and delegate the calls to BackupAdminImpl. We have a lot of tests for those classes, which indirectly cover almost all parts of BackupAdminImpl.

That said, I’ll still take a closer look at the class and add tests if I find any gaps.

anmolnar · 2025-05-28T13:41:54Z

Overall lgtm, but surprising how low the level of unit tests' granularity is: for instance BackupAdminImpl is such a big class and there's no corresponding test class. It has a lot of private methods which could be tested as "units" if they were package private and we had a BackupAdminTest class. Please consider that.

True, there isn’t a separate test class specifically for BackupAdminImpl. However, if you look at the overall structure, the core implementation resides in BackupAdminImpl, while the other classes primarily handle input preparation (including validation) and delegate the calls to BackupAdminImpl. We have a lot of tests for those classes, which indirectly cover almost all parts of BackupAdminImpl.

That said, I’ll still take a closer look at the class and add tests if I find any gaps.

Could you please provide examples of that?
Are they really just proxy calls?

vinayakphegde · 2025-05-28T18:49:28Z

Overall lgtm, but surprising how low the level of unit tests' granularity is: for instance BackupAdminImpl is such a big class and there's no corresponding test class. It has a lot of private methods which could be tested as "units" if they were package private and we had a BackupAdminTest class. Please consider that.

True, there isn’t a separate test class specifically for BackupAdminImpl. However, if you look at the overall structure, the core implementation resides in BackupAdminImpl, while the other classes primarily handle input preparation (including validation) and delegate the calls to BackupAdminImpl. We have a lot of tests for those classes, which indirectly cover almost all parts of BackupAdminImpl.
That said, I’ll still take a closer look at the class and add tests if I find any gaps.

Could you please provide examples of that? Are they really just proxy calls?

Yes, for example, mergeBackups() is already covered by tests in TestBackupMerge, TestIncrementalBackupMergeWithBulkLoad, and IntegrationTestBackupRestore. The checkIfValidForMerge() method is private and is only called from mergeBackups(), which is the case for most of the methods in this class.

That said, we can definitely create a dedicated test class for this and add the remaining/uncovered test cases. I’ll open a separate Jira for that. Does that sound good?

anmolnar · 2025-05-29T13:35:57Z

Overall lgtm, but surprising how low the level of unit tests' granularity is: for instance BackupAdminImpl is such a big class and there's no corresponding test class. It has a lot of private methods which could be tested as "units" if they were package private and we had a BackupAdminTest class. Please consider that.

True, there isn’t a separate test class specifically for BackupAdminImpl. However, if you look at the overall structure, the core implementation resides in BackupAdminImpl, while the other classes primarily handle input preparation (including validation) and delegate the calls to BackupAdminImpl. We have a lot of tests for those classes, which indirectly cover almost all parts of BackupAdminImpl.
That said, I’ll still take a closer look at the class and add tests if I find any gaps.

Could you please provide examples of that? Are they really just proxy calls?

Yes, for example, mergeBackups() is already covered by tests in TestBackupMerge, TestIncrementalBackupMergeWithBulkLoad, and IntegrationTestBackupRestore. The checkIfValidForMerge() method is private and is only called from mergeBackups(), which is the case for most of the methods in this class.

That said, we can definitely create a dedicated test class for this and add the remaining/uncovered test cases. I’ll open a separate Jira for that. Does that sound good?

Yeah, these examples are all integration tests as the same suggests in some cases: they create "real"/mini HBase cluster, sets up a connection, get reference to Admin and call methods which are publicly accessible. Looks like this is historically the main approach for HBase testing, but this is not unit testing.

What I'm talking about is instantiating BackupAdminImpl class directly with a mocked connection and test the methods individually. In order to do this you have to make methods accessible from unit tests, for instance, by changing visibility to package private. I don't insist doing this strictly, just leaving here as something to think about. Let me approve this patch regardless.

kgeisz

Looks good overall. I just have some minor comments, most of which are for AbstractRestoreDriver.java.

kgeisz · 2025-05-29T20:56:36Z