DieterDP-ng commented on code in PR #7746:
URL: https://github.com/apache/hbase/pull/7746#discussion_r3032828550
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -133,17 +144,34 @@ public BackupBoundariesBuilder addBackupTimestamps(String
host, long hostLogRoll
if (oldestStartCode > backupStartCode) {
oldestStartCode = backupStartCode;
}
-
- return this;
}
- public BackupBoundaries build() {
- if (boundaries.isEmpty()) {
- return EMPTY_BOUNDARIES;
+ private DeleteStatus getStatus(Address address, long hostLogRollTs) {
+ Long storedTs = boundaries.get(address);
+
+ if (storedTs == null) {
+ return hostLogRollTs <= oldestStartCode
+ ? DeleteStatus.OK
+ : DeleteStatus.NOT_DELETABLE_START_CODE;
}
- oldestStartCode -= tsCleanupBuffer;
- return new BackupBoundaries(boundaries, oldestStartCode);
+ return hostLogRollTs <= storedTs ? DeleteStatus.OK :
DeleteStatus.NOT_DELETABLE_BOUNDARY;
+ }
+ }
+
+ private enum DeleteStatus {
Review Comment:
I'd go for another name, since the goal is to check whether something is
deletable, not what it's "status of its delete" is. Suggestions:
`DeletionEligibility` or `CleanupDecision`.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -67,64 +66,76 @@ public boolean isDeletable(Path walLogPath) {
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);
+ for (Map.Entry<String, BoundaryInfo> entry : boundaries.entrySet()) {
+ String backupId = entry.getKey();
+ BoundaryInfo boundary = entry.getValue();
+ DeleteStatus status = boundary.getStatus(address, pathTs);
+ if (!status.isDeletable()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Backup {} preventing deletion of {} with ts of {} due
to {}", backupId,
+ walLogPath, pathTs, status);
+ }
+ return false;
}
- 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;
+ return true;
} catch (Exception e) {
LOG.warn("Error occurred while filtering file: {}. Ignoring cleanup of
this log", walLogPath,
e);
return false;
}
}
- public Map<Address, Long> getBoundaries() {
+ public Map<String, BoundaryInfo> 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 Map<String, BoundaryInfo> 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) {
+ public BackupBoundariesBuilder addBackupTimestamps(String backupId, String
host,
+ long hostLogRollTs, long backupStartCode) {
+ BoundaryInfo boundary = boundaries.computeIfAbsent(backupId, ignore ->
new BoundaryInfo());
Address address = Address.fromString(host);
+ boundary.add(address, hostLogRollTs, backupStartCode);
+ return this;
+ }
+
+ public BackupBoundaries build() {
+ if (boundaries.isEmpty()) {
+ return EMPTY_BOUNDARIES;
+ }
+
+ for (BoundaryInfo boundary : boundaries.values()) {
+ boundary.oldestStartCode -= tsCleanupBuffer;
Review Comment:
Nit: You're adjusting a private field here, it would be cleaner if the
`BoundaryInfo` would be all-final fields, and `build()` method would just
create the `BoundaryInfo`s in their final form.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -67,64 +66,76 @@ public boolean isDeletable(Path walLogPath) {
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);
+ for (Map.Entry<String, BoundaryInfo> entry : boundaries.entrySet()) {
+ String backupId = entry.getKey();
+ BoundaryInfo boundary = entry.getValue();
+ DeleteStatus status = boundary.getStatus(address, pathTs);
+ if (!status.isDeletable()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Backup {} preventing deletion of {} with ts of {} due
to {}", backupId,
+ walLogPath, pathTs, status);
+ }
+ return false;
}
- 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;
+ return true;
} catch (Exception e) {
LOG.warn("Error occurred while filtering file: {}. Ignoring cleanup of
this log", walLogPath,
e);
return false;
}
}
- public Map<Address, Long> getBoundaries() {
+ public Map<String, BoundaryInfo> 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 Map<String, BoundaryInfo> 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) {
+ public BackupBoundariesBuilder addBackupTimestamps(String backupId, String
host,
+ long hostLogRollTs, long backupStartCode) {
+ BoundaryInfo boundary = boundaries.computeIfAbsent(backupId, ignore ->
new BoundaryInfo());
Address address = Address.fromString(host);
+ boundary.add(address, hostLogRollTs, backupStartCode);
+ return this;
+ }
+
+ public BackupBoundaries build() {
+ if (boundaries.isEmpty()) {
+ return EMPTY_BOUNDARIES;
+ }
+
+ for (BoundaryInfo boundary : boundaries.values()) {
+ boundary.oldestStartCode -= tsCleanupBuffer;
Review Comment:
Perhaps worth adding a comment that the buffer is only added to the start
code (and not the per-host bounds) intentionally.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java:
##########
@@ -67,64 +66,76 @@ public boolean isDeletable(Path walLogPath) {
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);
+ for (Map.Entry<String, BoundaryInfo> entry : boundaries.entrySet()) {
+ String backupId = entry.getKey();
+ BoundaryInfo boundary = entry.getValue();
+ DeleteStatus status = boundary.getStatus(address, pathTs);
+ if (!status.isDeletable()) {
+ if (LOG.isDebugEnabled()) {
Review Comment:
No need to use `LOG.isDebugEnabled()` here, since it's a non-complex log
statement.
--
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]
