HBASE-29744: Data loss scenario for WAL files belonging to RS added between backups #7523
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. You've simplified the logic to determine what can be deleted, but I can see this backfiring in the following case. Hence, I'd prefer the old logic path (with a fix for the case of missing timestamps).
Contributor
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this 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. 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 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.
We don't, other than by reading the WAL. A WAL file can contain edits from mulitple tables.
Contributor
There was a problem hiding this comment. My suggestion would be to change the following in the original code: to: 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.
Contributor
Author
There was a problem hiding this 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.
Contributor
There was a problem hiding this comment. General flow looks good. Added some minor points of feedback. |
hgromer marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| /* | ||
| * 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.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import org.apache.hadoop.fs.Path; | ||
| 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; | ||
|
|
||
| /** | ||
| * Tracks time boundaries for WAL file cleanup during backup operations. Maintains the oldest | ||
| * timestamp per RegionServer included in any backup, enabling safe determination of which WAL files | ||
| * can be deleted without compromising backup integrity. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class BackupBoundaries { | ||
| private static final Logger LOG = LoggerFactory.getLogger(BackupBoundaries.class); | ||
| private static final BackupBoundaries EMPTY_BOUNDARIES = | ||
| new BackupBoundaries(Collections.emptyMap(), 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; | ||
|
|
||
| // The minimum WAL roll timestamp from the most recent backup of each backup root, used as a | ||
| // fallback cleanup boundary for RegionServers without explicit backup boundaries (e.g., servers | ||
| // that joined after backups began) | ||
| private final long oldestStartCode; | ||
hgromer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| private BackupBoundaries(Map<Address, Long> boundaries, long oldestStartCode) { | ||
| this.boundaries = boundaries; | ||
| this.oldestStartCode = oldestStartCode; | ||
| } | ||
|
|
||
| public boolean isDeletable(Path walLogPath) { | ||
| try { | ||
| String hostname = BackupUtils.parseHostNameFromLogFile(walLogPath); | ||
|
|
||
| if (hostname == null) { | ||
| LOG.warn( | ||
| "Cannot parse hostname from RegionServer WAL file: {}. Ignoring cleanup of this log", | ||
| walLogPath); | ||
| return false; | ||
| } | ||
|
|
||
| Address address = Address.fromString(hostname); | ||
| long pathTs = AbstractFSWALProvider.getTimestamp(walLogPath.getName()); | ||
|
|
||
| if (!boundaries.containsKey(address)) { | ||
| boolean isDeletable = pathTs <= oldestStartCode; | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug( | ||
| "Boundary for {} not found. isDeletable = {} based on oldestStartCode = {} and WAL ts of {}", | ||
| walLogPath, isDeletable, oldestStartCode, pathTs); | ||
| } | ||
| return isDeletable; | ||
| } | ||
|
|
||
| long backupTs = boundaries.get(address); | ||
| if (pathTs <= backupTs) { | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug( | ||
| "WAL cleanup time-boundary found for server {}: {}. Ok to delete older file: {}", | ||
| address.getHostName(), pathTs, walLogPath); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("WAL cleanup time-boundary found for server {}: {}. Keeping younger file: {}", | ||
| address.getHostName(), backupTs, walLogPath); | ||
| } | ||
|
|
||
| return false; | ||
| } catch (Exception e) { | ||
| LOG.warn("Error occurred while filtering file: {}. Ignoring cleanup of this log", walLogPath, | ||
| e); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| public Map<Address, Long> getBoundaries() { | ||
| return boundaries; | ||
| } | ||
|
|
||
| public long getOldestStartCode() { | ||
| return oldestStartCode; | ||
| } | ||
|
|
||
| public static BackupBoundariesBuilder builder(long tsCleanupBuffer) { | ||
| return new BackupBoundariesBuilder(tsCleanupBuffer); | ||
| } | ||
|
|
||
| public static class BackupBoundariesBuilder { | ||
| private final Map<Address, Long> boundaries = new HashMap<>(); | ||
| private final long tsCleanupBuffer; | ||
|
|
||
| private long oldestStartCode = Long.MAX_VALUE; | ||
|
|
||
| private BackupBoundariesBuilder(long tsCleanupBuffer) { | ||
| this.tsCleanupBuffer = tsCleanupBuffer; | ||
| } | ||
|
|
||
| public BackupBoundariesBuilder addBackupTimestamps(String host, long hostLogRollTs, | ||
| long backupStartCode) { | ||
| Address address = Address.fromString(host); | ||
| Long storedTs = boundaries.get(address); | ||
| if (storedTs == null || hostLogRollTs < storedTs) { | ||
| boundaries.put(address, hostLogRollTs); | ||
| } | ||
|
|
||
| if (oldestStartCode > backupStartCode) { | ||
| oldestStartCode = backupStartCode; | ||
| } | ||
|
|
||
| return this; | ||
| } | ||
|
|
||
| public BackupBoundaries build() { | ||
| if (boundaries.isEmpty()) { | ||
| return EMPTY_BOUNDARIES; | ||
| } | ||
|
|
||
| oldestStartCode -= tsCleanupBuffer; | ||
| return new BackupBoundaries(boundaries, oldestStartCode); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.