Skip to content

HBASE-28705 backup log cleaner cleans required walls when using multiple backuproots #6040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ndimiduk merged 3 commits into from
Sep 25, 2024
Merged

HBASE-28705 backup log cleaner cleans required walls when using multiple backuproots #6040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f371d51
HBASE-28705 BackupLogCleaner cleans required WALs when using multiple…
DieterDP-ng Jul 2, 2024
7d76842
HBASE-28705 BackupLogCleaner cleans required WALs when using multiple…
DieterDP-ng Jul 2, 2024
a31a19c
Process review comments, bugfix
DieterDP-ng Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 55 additions & 30 deletions hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,39 +81,64 @@ public void init(Map<String, Object> params) {
}
}

private Map<Address, Long> getServerToNewestBackupTs(List<BackupInfo> backups)
/**
* Calculates the timestamp boundary up to which all backup roots have already included the WAL.
* I.e. WALs with a lower (= older) or equal timestamp are no longer needed for future incremental
* backups.
*/
private Map<Address, Long> serverToPreservationBoundaryTs(List<BackupInfo> backups)
Copy link
Contributor

@rmdmattingly rmdmattingly Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this method have a precondition asserting that all BackupInfos in the list have an identical table set?

Otherwise I think this could cause data loss in an incremental backup. Let's say we have one-table and another-table on our cluster, and we back up each one independently. Let's say we first took a backup for one-table, then a backup for another-table. Then we passed the corresponding BackupInfos into this method, this would yield preservation boundaries which would remove WALs necessary for the next incremental backup of one-table

Copy link
Contributor Author

@DieterDP-ng DieterDP-ng Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an issue. Trying to follow your example:

  • Imagine 3 servers S1, S2, S3
  • table T1 has regions on S1 and S2, table T2 has regions on S2 and S3
  • we have 2 backup roots R1 that backs up T1, and R2 that backs up T2

At time:

  • t=0, we backup T1 in R1 => backup B1
  • t=0, we backup T2 in R2 => backup B2
  • t=10, we backup T1 in R1 => backup B3
  • t=20, we backup T2 in R2. => backup B4

Following the logic in this method:

  • newestBackupPerRootDir will contain: (R1: B3, R2: B4)
  • boundaries will contain: (S1: 10, S2: 10, S3: 20)

So for T1, all WALs up to t=10 can be deleted, for T2, WALs will be preserved from t=10 or t=20, depending whether other tables are present.

Copy link
Member

@ndimiduk ndimiduk Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation, both this and below. I think I follow you. Let me rephrase.

newestBackupPerRootDir is a "race to the top" in terms of identifying the most recent timestamp across backups, while boundaries is a "race to the bottom" in terms of identifying the least recent backup present on each server. By selecting the "oldest of the newest" you determine the minimum timestamp that must be preserved for each server. Any WALs older than this minimum timestamp can be safely discarded.

Copy link
Contributor

@rmdmattingly rmdmattingly Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I think the missing piece for me was that a backup root was specific to a given table

we have 2 backup roots R1 that backs up T1, and R2 that backs up T2

Copy link
Contributor Author

@DieterDP-ng DieterDP-ng Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I think the missing piece for me was that a backup root was specific to a given table

It isn't. Each backup root contains one or more tables, i.e. a set of tables, these table sets may or may not have tables in common.

Copy link
Contributor

@rmdmattingly rmdmattingly Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, okay so reframing your example in a single root:

Imagine
3 servers S1, S2, S3
table T1 has regions on S1 and S2, table T2 has regions on S2 and S3
we have 1 backup root R1 that backs up T1 and T2

At time:
t=0, we backup T1 in R1 => backup B1
t=0, we backup T2 in R1 => backup B2
t=10, we backup T1 in R1 => backup B3
t=20, we backup T2 in R1 => backup B4

Following the logic in this method:

newestBackupPerRootDir will contain: (R1: B4)

We would calculate the preservation boundaries here:

    // This map tracks, for every address, the least recent (= oldest / lowest timestamp) inclusion
    // in any backup. In other words, it is the timestamp boundary up to which all backups roots
    // have included the WAL in their backup.
    Map<Address, Long> boundaries = new HashMap<>();
    for (BackupInfo backupInfo : newestBackupPerRootDir.values()) {
      for (TableName table : backupInfo.getTables()) {
        for (Map.Entry<String, Long> entry : backupInfo.getTableSetTimestampMap().get(table)
          .entrySet()) {
          Address address = Address.fromString(entry.getKey());
          Long storedTs = boundaries.get(address);
          if (storedTs == null || entry.getValue() < storedTs) {
            boundaries.put(address, entry.getValue());
          }
        }
      }
    }

Since the newest backup in R1 is B4, serverToPreservationBoundaryTs will contain (S1: 20, S2: 20, S3: 20)

In this situation, we must not delete WALs from S1 or S2 between times 10 and 20 because a subsequent incremental backup of T1 will require those WALs.

In the BackupLogCleaner, though, we will end up calling canDeleteFile and hitting this code with that serverToPreservationBoundaryTs (S1: 20, S2: 20, S3: 20) renamed as addressToBoundaryTs:

      if (!addressToBoundaryTs.containsKey(walServerAddress)) { // for S1 & S2, this will be false
         // irrelevant ...
      }

      Long backupBoundary = addressToBoundaryTs.get(walServerAddress); // for S1 & S2, this will be 20
      if (backupBoundary >= walTimestamp) { // For WALs between times 10 and 20, this will be true
        if (LOG.isDebugEnabled()) {
          LOG.debug(
            "Backup found for server: {}. All backups from {} are newer than file, so deleting: {}",
            walServerAddress.getHostName(), backupBoundary, path);
        }
        // so we will erroneously delete WALs <20, despite the next inc backup of T1 requiring them
        return true;
      }

I guess this depends on how ancestry is defined? If we consider all backups in a root to be ancestors regardless of their table set, then maybe it is okay to delete these WALs. But, if not, then I don't see how it is okay?

Copy link
Contributor Author

@DieterDP-ng DieterDP-ng Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newestBackupPerRootDir will contain: (R1: B4)

Agreed

Since the newest backup in R1 is B4, serverToPreservationBoundaryTs will contain (S1: 20, S2: 20, S3: 20)

I think this is wrong.

To make things more concrete, I'm going to assume all backups were full backups, and B1 == B2 (since it has the same timestamp). I.e. the backups were:

  • B1: at timestamp 0, containing tables T1 & T2
  • B3: at timestamp 10, containing T1
  • B4: at timestamp 20, containing T2

At this point, the data in backupInfo.getTableSetTimestampMap() will be:

T1:
  S1: 10
  S2: 10
T2:
  S2: 20
  S3: 20

and serverToPreservationBoundaryTs will be (S1: 10, S2: 10, S3: 20).

The reason that the BackupInfo of B4 contains the log timestamps of tables not included in B4 is due to how the backup client updates these:

// From: FullTableBackupClient

// Updates only the rows for the tables included in B4
backupManager.writeRegionServerLogTimestamp(backupInfo.getTables(), newTimestamps);

// Reads the rows for all tables once included in a backup in this backup root
Map<TableName, Map<String, Long>> newTableSetTimestampMap = backupManager.readLogTimestampMap();

Copy link
Contributor

@rmdmattingly rmdmattingly Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being a pain here, but I'm not sure I agree. When building serverToPreservationBoundaryTs we loop through:

Map<Address, Long> boundaries = new HashMap<>();
for (BackupInfo backupInfo : newestBackupPerRootDir.values()) {
  // build boundaries via tableSetTimestampMap, like you said
}
// logging omitted
return boundaries;

and we agree that newestBackupPerRootDir.values() will only contain B4. So our boundaries would end up only being based on the timestamps from B4? How does the second newest backup, and beyond, in the root come into play?

Copy link
Contributor

@rmdmattingly rmdmattingly Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the second newest backup, and beyond, in the root come into play?

Answering my own questions a bit here, I think I needed to look deeper into how the BackupInfo gets constructed. When the tableSetTimestampMap is populated, it will contain entries for every table in the root, not just every table in the given backup. So while B4 might only pertain to T2, its corresponding BackupInfo will be aware of the timestamps for T1. That's a confusing design imo, but if true then I think this logic is sound

Copy link
Contributor Author

@DieterDP-ng DieterDP-ng Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's indeed like that. Doing it that way is a good way to avoid some special cases where backups in the same root backup do not contain the same tables, but the docs surrounding those BackupInfo concepts is indeed lacking.

throws IOException {
if (LOG.isDebugEnabled()) {
LOG.debug(
"Cleaning WALs if they are older than the newest backups. "
"Cleaning WALs if they are older than the WAL cleanup time-boundary. "
+ "Checking WALs against {} backups: {}",
backups.size(),
backups.stream().map(BackupInfo::getBackupId).sorted().collect(Collectors.joining(", ")));
}
Map<Address, Long> serverAddressToNewestBackupMap = new HashMap<>();

Map<TableName, Long> tableNameBackupInfoMap = new HashMap<>();
for (BackupInfo backupInfo : backups) {
for (TableName table : backupInfo.getTables()) {
tableNameBackupInfoMap.putIfAbsent(table, backupInfo.getStartTs());
if (tableNameBackupInfoMap.get(table) <= backupInfo.getStartTs()) {
tableNameBackupInfoMap.put(table, backupInfo.getStartTs());
for (Map.Entry<String, Long> entry : backupInfo.getTableSetTimestampMap().get(table)
.entrySet()) {
serverAddressToNewestBackupMap.put(Address.fromString(entry.getKey()),
entry.getValue());

// This map tracks, for every backup root, the most recent created backup (= highest timestamp)
Map<String, BackupInfo> newestBackupPerRootDir = new HashMap<>();
Copy link
Member

@ndimiduk ndimiduk Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can there be multiple BackupInfo objects with the same root? (what is a "root" anyway? a FileSystem + Path?) Should this be a Map<String, List<BackupInfo>>?

Copy link
Contributor Author

@DieterDP-ng DieterDP-ng Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A backup root corresponds to the path you specify when doing hbase backup create <type> <backup_path>. Within a single root, you'll have a series of backups. Having multiple roots allows you to manage backups independently (eg: if you delete one root, backups in other roots are unaffected). Incremental backups are always built on top of previous backups from the same root.

Eg, (F = full backup, I = incremental backup):

Time --------------------------------------------->
Root1: F    I     I     I     F    I    I    I
Root2: F          F  I    I   I              F

So to answer your questions:

  • There are multiple BackupInfos per root: 1 per backup created in that root. A root is a Path.
  • In this piece of logic, we only need the most recent (= newest) backup, so Map<String, BackupInfo>, where the root is represented as a String.

for (BackupInfo backup : backups) {
BackupInfo existingEntry = newestBackupPerRootDir.get(backup.getBackupRootDir());
if (existingEntry == null || existingEntry.getStartTs() < backup.getStartTs()) {
newestBackupPerRootDir.put(backup.getBackupRootDir(), backup);
}
}

if (LOG.isDebugEnabled()) {
LOG.debug("WAL cleanup time-boundary using info from: {}. ",
newestBackupPerRootDir.entrySet().stream()
.map(e -> "Backup root " + e.getKey() + ": " + e.getValue().getBackupId()).sorted()
.collect(Collectors.joining(", ")));
}

// 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.
Map<Address, Long> boundaries = new HashMap<>();
for (BackupInfo backupInfo : newestBackupPerRootDir.values()) {
// Iterate over all tables in the timestamp map, which contains all tables covered in the
// backup root, not just the tables included in that specific backup (which could be a subset)
for (TableName table : backupInfo.getTableSetTimestampMap().keySet()) {
for (Map.Entry<String, Long> entry : backupInfo.getTableSetTimestampMap().get(table)
.entrySet()) {
Address address = Address.fromString(entry.getKey());
Long storedTs = boundaries.get(address);
if (storedTs == null || entry.getValue() < storedTs) {
boundaries.put(address, entry.getValue());
}
}
}
}

if (LOG.isDebugEnabled()) {
for (Map.Entry<Address, Long> entry : serverAddressToNewestBackupMap.entrySet()) {
LOG.debug("Server: {}, Newest Backup: {}", entry.getKey().getHostName(), entry.getValue());
for (Map.Entry<Address, Long> entry : boundaries.entrySet()) {
LOG.debug("Server: {}, WAL cleanup boundary: {}", entry.getKey().getHostName(),
entry.getValue());
}
}

return serverAddressToNewestBackupMap;
return boundaries;
}

@Override
Expand All @@ -128,18 +153,19 @@ public Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
return files;
}

Map<Address, Long> addressToNewestBackupMap;
Map<Address, Long> serverToPreservationBoundaryTs;
try {
try (BackupManager backupManager = new BackupManager(conn, getConf())) {
addressToNewestBackupMap = getServerToNewestBackupTs(backupManager.getBackupHistory(true));
serverToPreservationBoundaryTs =
serverToPreservationBoundaryTs(backupManager.getBackupHistory(true));
}
} catch (IOException ex) {
LOG.error("Failed to analyse backup history with exception: {}. Retaining all logs",
ex.getMessage(), ex);
return Collections.emptyList();
}
for (FileStatus file : files) {
if (canDeleteFile(addressToNewestBackupMap, file.getPath())) {
if (canDeleteFile(serverToPreservationBoundaryTs, file.getPath())) {
filteredFiles.add(file);
}
}
Expand Down Expand Up @@ -174,7 +200,7 @@ public boolean isStopped() {
return this.stopped;
}

protected static boolean canDeleteFile(Map<Address, Long> addressToNewestBackupMap, Path path) {
protected static boolean canDeleteFile(Map<Address, Long> addressToBoundaryTs, Path path) {
if (isHMasterWAL(path)) {
return true;
}
Expand All @@ -190,28 +216,27 @@ protected static boolean canDeleteFile(Map<Address, Long> addressToNewestBackupM
Address walServerAddress = Address.fromString(hostname);
long walTimestamp = AbstractFSWALProvider.getTimestamp(path.getName());

if (!addressToNewestBackupMap.containsKey(walServerAddress)) {
if (!addressToBoundaryTs.containsKey(walServerAddress)) {
if (LOG.isDebugEnabled()) {
LOG.debug("No backup found for server: {}. Deleting file: {}",
LOG.debug("No cleanup WAL time-boundary found for server: {}. Ok to delete file: {}",
walServerAddress.getHostName(), path);
}
return true;
}

Long lastBackupTs = addressToNewestBackupMap.get(walServerAddress);
if (lastBackupTs >= walTimestamp) {
Long backupBoundary = addressToBoundaryTs.get(walServerAddress);
if (backupBoundary >= walTimestamp) {
if (LOG.isDebugEnabled()) {
LOG.debug(
"Backup found for server: {}. Backup from {} is newer than file, so deleting: {}",
walServerAddress.getHostName(), lastBackupTs, path);
"WAL cleanup time-boundary found for server {}: {}. Ok to delete older file: {}",
walServerAddress.getHostName(), backupBoundary, path);
}
return true;
}

if (LOG.isDebugEnabled()) {
LOG.debug(
"Backup found for server: {}. Backup from {} is older than the file, so keeping: {}",
walServerAddress.getHostName(), lastBackupTs, path);
LOG.debug("WAL cleanup time-boundary found for server {}: {}. Keeping younger file: {}",
walServerAddress.getHostName(), backupBoundary, path);
}
} catch (Exception ex) {
LOG.warn("Error occurred while filtering file: {}. Ignoring cleanup of this log", path, ex);
Expand Down
2 changes: 1 addition & 1 deletion hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ protected String backupTables(BackupType type, List<TableName> tables, String pa
try {
conn = ConnectionFactory.createConnection(conf1);
badmin = new BackupAdminImpl(conn);
BackupRequest request = createBackupRequest(type, tables, path);
BackupRequest request = createBackupRequest(type, new ArrayList<>(tables), path);
backupId = badmin.backupTables(request);
} finally {
if (badmin != null) {
Expand Down
Loading