Copilot commented on code in PR #11828:
URL: https://github.com/apache/ozone/pull/11828#discussion_r4405244801
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/OpenKeyCleanupService.java:
##########
@@ -250,6 +250,8 @@ public BackgroundTaskResult call() throws Exception {
private OMRequest createCommitKeyRequest(
CommitKeyRequest.Builder request) {
+ // Submitting to Ratis skips preExecute, which sets the modification
time for client commits.
+ request.getKeyArgsBuilder().setModificationTime(Time.now());
Review Comment:
This unconditionally overwrites `KeyArgs.modificationTime`. If this helper
can be invoked with a request that already has a meaningful modification time
(even accidentally in the future), it would silently clobber it. Prefer setting
the modification time only when it is unset/zero (or assert it is zero) so this
internal fix doesn’t change semantics for any pre-populated requests.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestOpenKeyCleanupService.java:
##########
@@ -273,6 +274,7 @@ public void testCommitExpiredHsyncKeys() throws Exception {
when(om.getScmClient().getContainerClient().getContainerWithPipeline(anyLong()))
.thenReturn(new
ContainerWithPipeline(Mockito.mock(ContainerInfo.class), pipeline));
+ final long createTime = Time.now();
Review Comment:
This test relies on wall-clock ordering (`Time.now()`), which can be flaky
if the system clock shifts (e.g., NTP adjustments) or if timestamps are
recorded with different resolutions. A more robust assertion is to validate
`modificationTime > 0` and/or `modificationTime >= creationTime` for the
newly-created keys (or use a controllable/fake clock if available in this test
suite) instead of comparing against a second wall-clock snapshot (`commitTime`).
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestOpenKeyCleanupService.java:
##########
@@ -286,6 +288,7 @@ public void testCommitExpiredHsyncKeys() throws Exception {
assertExpiredOpenKeys(false, true,
BucketLayout.FILE_SYSTEM_OPTIMIZED);
+ final long commitTime = Time.now();
Review Comment:
This test relies on wall-clock ordering (`Time.now()`), which can be flaky
if the system clock shifts (e.g., NTP adjustments) or if timestamps are
recorded with different resolutions. A more robust assertion is to validate
`modificationTime > 0` and/or `modificationTime >= creationTime` for the
newly-created keys (or use a controllable/fake clock if available in this test
suite) instead of comparing against a second wall-clock snapshot (`commitTime`).
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestOpenKeyCleanupService.java:
##########
@@ -294,6 +297,11 @@ public void testCommitExpiredHsyncKeys() throws Exception {
List<OmKeyInfo> listKeyInfo =
getKeyInfo(BucketLayout.FILE_SYSTEM_OPTIMIZED, false);
// Verify keyName and fileName is same after auto commit key.
listKeyInfo.stream().forEach(key -> assertEquals(key.getKeyName(),
key.getFileName()));
+ // Verify auto commit sets the modification time, ignoring keys left by
earlier tests.
+ assertThat(listKeyInfo)
+ .filteredOn(key -> key.getCreationTime() >= createTime)
+ .hasSize(keyCount)
+ .allSatisfy(key ->
assertThat(key.getModificationTime()).isGreaterThanOrEqualTo(commitTime));
Review Comment:
This test relies on wall-clock ordering (`Time.now()`), which can be flaky
if the system clock shifts (e.g., NTP adjustments) or if timestamps are
recorded with different resolutions. A more robust assertion is to validate
`modificationTime > 0` and/or `modificationTime >= creationTime` for the
newly-created keys (or use a controllable/fake clock if available in this test
suite) instead of comparing against a second wall-clock snapshot (`commitTime`).
--
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]
