rich7420 commented on code in PR #11069:
URL: https://github.com/apache/ozone/pull/11069#discussion_r3819187025
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyLifecycleService.java:
##########
@@ -677,10 +681,8 @@ private void evaluateKeyAndDirTable(OmBucketInfo bucket,
long volumeObjId, Table
HashSet<Long> deletedDirSet = new HashSet<>();
while (!stack.isEmpty()) {
if (!shouldRun()) {
- LOG.info("LifecycleActionTask for bucket {} stopping. " +
- "Service enabled: {}, suspended: {}, leader ready: {}",
- bucketName, isServiceEnabled.get(), suspended.get(),
- getOzoneManager() != null ? getOzoneManager().isLeaderReady() :
"N/A");
+ scanAborted = true;
+ LOG.info("KeyLifecycleService is suspended, disabled, or leader not
ready. Stopping task for bucket {}.", bucketName);
Review Comment:
Over 120 chars (checkstyle), and the wording differs from the other abort
logs (L1034/L1099), which breaks the new `contains("suspended or disabled")`
assertion for FSO. Revert to the shared message.
```suggestion
LOG.info("KeyLifecycleService is suspended or disabled. Stopping
task for bucket {}.", bucketName);
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyLifecycleService.java:
##########
@@ -569,6 +572,55 @@ void testInFlightClearedWhenTaskSkipsRun() throws
Exception {
}
}
+ @ParameterizedTest
+ @MethodSource("parameters1")
+ void testAbortedScanDoesNotMarkScanComplete(BucketLayout bucketLayout,
boolean createPrefix)
+ throws Exception {
+ final String volumeName = getTestName();
+ final String bucketName = uniqueObjectName("bucket");
+ String keyPrefix = "key";
+ String rulePrefix = bucketLayout == FILE_SYSTEM_OPTIMIZED ? "" : "key";
+ int testKeyCount = 3;
+
+ keyLifecycleService.setListMaxSize(1);
+ //keyLifecycleService.suspend();
+ KeyLifecycleService.setInjectors(Arrays.asList(new FaultInjectorImpl()));
+
+ List<OmKeyArgs> keyList =
+ createKeys(volumeName, bucketName, bucketLayout, testKeyCount, 1,
keyPrefix, null);
+ assertEquals(testKeyCount, keyList.size());
+
+ ZonedDateTime now = ZonedDateTime.now(ZoneOffset.UTC);
+ ZonedDateTime date = now.plusSeconds(EXPIRE_SECONDS);
+ if (createPrefix) {
+ createLifecyclePolicy(volumeName, bucketName, bucketLayout,
rulePrefix, null, date.toString(), true);
+ } else {
+ OmLCFilter.Builder filter = getOmLCFilterBuilder(rulePrefix, null,
null);
+ createLifecyclePolicy(volumeName, bucketName, bucketLayout, null,
filter.build(), date.toString(), true);
+ }
+
+ String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+ //keyLifecycleService.resume();
+
+ GenericTestUtils.waitFor(() ->
keyLifecycleService.status().getRunningBucketsList().contains(bucketKey),
+ WAIT_CHECK_INTERVAL, 10000);
+ keyLifecycleService.suspend();
+ KeyLifecycleService.getInjector(0).resume();
+
+ GenericTestUtils.LogCapturer logCapturer =
GenericTestUtils.LogCapturer.captureLogs(
+ LoggerFactory.getLogger(KeyLifecycleService.class));
+ GenericTestUtils.waitFor(() ->
keyLifecycleService.status().getRunningBucketsList().isEmpty(),
+ WAIT_CHECK_INTERVAL, 10000);
+
+ OmLifecycleScanState scanState =
metadataManager.getLifecycleScanStateTable().get(bucketKey);
+ assertNotNull(scanState);
+ assertNull(scanState.getScanEndTime(),
+ "Aborted scan must not persist scanEndTime so a new leader can
resume");
+ assertTrue(logCapturer.getOutput().contains("KeyLifecycleService is
suspended or disabled"));
+ keyLifecycleService.resume();
+ deleteLifecyclePolicy(volumeName, bucketName);
Review Comment:
`LogCapturer` is installed after the `resume()` that emits the log, so it
can miss it (flaky). And `resume()` runs only on success — if an assertion
fails the service stays suspended and the other parameters time out. Capture
first, resume in `finally`.
```suggestion
GenericTestUtils.LogCapturer logCapturer =
GenericTestUtils.LogCapturer.captureLogs(
LoggerFactory.getLogger(KeyLifecycleService.class));
GenericTestUtils.waitFor(() ->
keyLifecycleService.status().getRunningBucketsList().contains(bucketKey),
WAIT_CHECK_INTERVAL, 10000);
keyLifecycleService.suspend();
KeyLifecycleService.getInjector(0).resume();
try {
GenericTestUtils.waitFor(() ->
keyLifecycleService.status().getRunningBucketsList().isEmpty(),
WAIT_CHECK_INTERVAL, 10000);
OmLifecycleScanState scanState =
metadataManager.getLifecycleScanStateTable().get(bucketKey);
assertNotNull(scanState);
assertNull(scanState.getScanEndTime(),
"Aborted scan must not persist scanEndTime so a new leader can
resume");
assertTrue(logCapturer.getOutput().contains("KeyLifecycleService is
suspended or disabled"));
} finally {
keyLifecycleService.resume();
deleteLifecyclePolicy(volumeName, bucketName);
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyLifecycleService.java:
##########
@@ -436,9 +437,12 @@ public BackgroundTaskResult call() {
evaluateBucket(bucket, keyTable, expirationRules,
expiredKeyList, scanStateBuilder);
}
+ boolean scanFinished = !scanAborted;
if (expiredKeyList.isEmpty() && expiredDirList.isEmpty()) {
LOG.info("No expired keys/dirs found/remained for bucket {}",
bucketKey);
- sendSaveScanStateRequest(scanStateBuilder, true);
+ if (scanFinished || test) {
Review Comment:
On abort+empty this persists nothing in production (only under `test`),
losing the checkpoint. Send `sendSaveScanStateRequest(scanStateBuilder, false)`
unconditionally on abort and drop `|| test`.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyLifecycleService.java:
##########
@@ -569,6 +572,55 @@ void testInFlightClearedWhenTaskSkipsRun() throws
Exception {
}
}
+ @ParameterizedTest
+ @MethodSource("parameters1")
+ void testAbortedScanDoesNotMarkScanComplete(BucketLayout bucketLayout,
boolean createPrefix)
+ throws Exception {
+ final String volumeName = getTestName();
+ final String bucketName = uniqueObjectName("bucket");
+ String keyPrefix = "key";
+ String rulePrefix = bucketLayout == FILE_SYSTEM_OPTIMIZED ? "" : "key";
+ int testKeyCount = 3;
+
+ keyLifecycleService.setListMaxSize(1);
+ //keyLifecycleService.suspend();
Review Comment:
Leftover commented-out code here and at L603 — please remove.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyLifecycleService.java:
##########
@@ -677,10 +681,8 @@ private void evaluateKeyAndDirTable(OmBucketInfo bucket,
long volumeObjId, Table
HashSet<Long> deletedDirSet = new HashSet<>();
while (!stack.isEmpty()) {
if (!shouldRun()) {
- LOG.info("LifecycleActionTask for bucket {} stopping. " +
- "Service enabled: {}, suspended: {}, leader ready: {}",
- bucketName, isServiceEnabled.get(), suspended.get(),
- getOzoneManager() != null ? getOzoneManager().isLeaderReady() :
"N/A");
+ scanAborted = true;
Review Comment:
The `catch (CapacityFullException)` returns at L676/L794/L807 don't set this
flag, so a capacity abort still marks the bucket complete — same issue as this
Jira. Set `scanAborted = true` there too.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
