Copilot commented on code in PR #11069:
URL: https://github.com/apache/ozone/pull/11069#discussion_r3818387776
##########
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 or disabled. Stopping
task for bucket {}.", bucketName);
Review Comment:
shouldRun() can be false due to leader not being ready (in addition to
suspended/disabled). This log message is misleading in those cases and makes
diagnosing leader-transfer/leader-ready aborts harder.
This issue also appears in the following locations of the same file:
- line 1033
- line 1098
##########
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) {
+ sendSaveScanStateRequest(scanStateBuilder, scanFinished);
+ }
Review Comment:
When a scan is aborted (scanFinished=false) and there are no expired
keys/dirs, this branch currently skips persisting the scan state. That can drop
the latest lastScannedKey/Dir checkpoint and/or leave a previous scanEndTime in
the table, so a new leader may not be able to resume accurately. Persist the
scan state even when aborted, but pass scanFinished=false so scanEndTime is not
set.
##########
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()));
Review Comment:
Please remove commented-out code in the test setup; it adds noise and makes
it harder to understand the intended control flow.
This issue also appears on line 602 of the same file.
##########
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));
Review Comment:
The log capture is started after suspending and resuming the injector. Since
the task continues asynchronously after resume(), the expected log line can be
emitted before capture starts, making this assertion flaky. Start the
LogCapturer before triggering the abort/resume.
--
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]
