[
https://issues.apache.org/jira/browse/GOBBLIN-2168?focusedWorklogId=943708&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-943708
]
ASF GitHub Bot logged work on GOBBLIN-2168:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 14/Nov/24 06:47
Start Date: 14/Nov/24 06:47
Worklog Time Spent: 10m
Work Description: phet commented on code in PR #4070:
URL: https://github.com/apache/gobblin/pull/4070#discussion_r1841645269
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/trash/TimeBasedSnapshotCleanupPolicyTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.gobblin.data.management.trash;
+
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
+import org.joda.time.DateTime;
+import org.joda.time.DateTimeZone;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+import org.testng.annotations.BeforeMethod;
+
+import java.io.IOException;
+import java.util.Properties;
+
+public class TimeBasedSnapshotCleanupPolicyTest {
+
+ private MockTimeBasedSnapshotCleanupPolicy cleanupPolicy;
+
+ @BeforeMethod
+ public void setUp() {
+ // Initialize the cleanup policy with a retention period (e.g., 1 day)
+ Properties properties = new Properties();
+
properties.setProperty(MockTimeBasedSnapshotCleanupPolicy.SNAPSHOT_RETENTION_POLICY_MINUTES_KEY,
"1440"); // 1440 minutes = 1 day
+
properties.setProperty(MockTimeBasedSnapshotCleanupPolicy.RETENTION_SNAPSHOT_TIMEZONE,
"UTC");
+ // Mock the cutoff time to be 2024-10-30 01:01:00 UTC
+ cleanupPolicy = new MockTimeBasedSnapshotCleanupPolicy(properties, new
DateTime(2024, 10, 30, 1, 1));
+ }
+
+ @Test
+ public void testShouldDeleteSnapshot() throws IOException {
+
+ // Create a Trash
+ TrashTestBase trashTestBase = new TrashTestBase(new Properties());
+ Trash trash = trashTestBase.trash;
+
+ // Create dummy paths
+ FileStatus fs1 = new FileStatus(0, true, 0, 0, 0,
+ new Path(trash.getTrashLocation(), new
DateTime(2024, 10, 29, 1, 0,
DateTimeZone.UTC).toString(Trash.TRASH_SNAPSHOT_NAME_FORMATTER)));
+ FileStatus fs2 = new FileStatus(0, true, 0, 0, 0,
+ new Path(trash.getTrashLocation(), new
DateTime(2024, 10, 29, 2, 0,
DateTimeZone.UTC).toString(Trash.TRASH_SNAPSHOT_NAME_FORMATTER)));
+
+ // Test old snapshot (should be deleted)
+ // 2024-10-29 01:00:00 UTC + 1440 minutes = 2024-10-30 01:00:00 UTC <
Cutoff time a.k.a system current_time (2024-10-30 01:01:00 UTC)
+ Assert.assertTrue(cleanupPolicy.shouldDeleteSnapshot(fs1, trash), "Old
snapshot should be deleted");
+
+ // Test snapshot (should not be deleted)
+ // 2024-10-29 02:00:00 UTC + 1440 minutes = 2024-10-30 02:00:00 UTC >
Cutoff time a.k.a system current_time (2024-10-30 01:01:00 UTC)
+ Assert.assertFalse(cleanupPolicy.shouldDeleteSnapshot(fs2, trash),
"snapshot should not be deleted");
+ }
+
+ /**
+ * Mock the TimeBasedSnapshotCleanupPolicy for testing purposes
+ *
+ * In this class, the current time used in the comparison method
isBefore() can be mocked
+ * Why? The current time is used to determine if a snapshot is older than
the retention period,
+ * and given that the current time is always changing, it is difficult to
test the method shouldDeleteSnapshot()
+ */
+ public class MockTimeBasedSnapshotCleanupPolicy extends
TimeBasedSnapshotCleanupPolicy {
+
+ private final DateTime MOCK_CURRENT_TIME;
+
+ public MockTimeBasedSnapshotCleanupPolicy(Properties props, DateTime
mockCurrentTime) {
+ super(props);
+ this.MOCK_CURRENT_TIME = mockCurrentTime;
+ }
+
+ @Override
+ public boolean shouldDeleteSnapshot(FileStatus snapshot, Trash trash) {
+ DateTime snapshotTime =
Trash.TRASH_SNAPSHOT_NAME_FORMATTER.parseDateTime(snapshot.getPath().getName());
+ return
snapshotTime.plusMinutes(this.retentionMinutes).isBefore(this.MOCK_CURRENT_TIME.withZoneRetainFields(this.retentionSnapshotTimezone));
+ }
Review Comment:
as *THE ONLY* method of the class-under-test, the test itself can in no way
meaningfully verify the impl, when this mock actually reimplements that method!
I see your challenge: you'd like to hard-code a known date-time in the test,
but that's not possible because the `TimeBasedSnapshotCleanupPolicy` isn't
parameterized by a source/provider of the current date-time.
typically [that is the canonical testing
pattern](https://stackoverflow.com/a/9410093) I would recommend, but in this
particular case, where the crux of the impl comes down to the actual use of
`DateTime.now(DateTimeZone)`, I'd suggest instead NOT to hard-code a known time
but rather to calculate one based on the true current time when the test runs.
e.g. set retention at 600 mins (10 hours) and render a path that is between
12 and 13 hours ago in UTC. then instantiate an instance using TZ.UTC to
verify `Assert.assertTrue(x.shouldDeleteSnapshot(...))` and instantiate another
policy using TZ.PST to verify `assertFalse`. (there's nothing special about
PST, but just choose a TZ that is at least >= 4 hours later than UTC.)
then perhaps round out the test by then generating a path that is > 10
hours, even in PST, and also another that's < 9 hours ago in UTC
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/trash/TimeBasedSnapshotCleanupPolicyTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.gobblin.data.management.trash;
+
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
+import org.joda.time.DateTime;
+import org.joda.time.DateTimeZone;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+import org.testng.annotations.BeforeMethod;
+
+import java.io.IOException;
+import java.util.Properties;
+
+public class TimeBasedSnapshotCleanupPolicyTest {
+
+ private MockTimeBasedSnapshotCleanupPolicy cleanupPolicy;
+
+ @BeforeMethod
+ public void setUp() {
+ // Initialize the cleanup policy with a retention period (e.g., 1 day)
+ Properties properties = new Properties();
+
properties.setProperty(MockTimeBasedSnapshotCleanupPolicy.SNAPSHOT_RETENTION_POLICY_MINUTES_KEY,
"1440"); // 1440 minutes = 1 day
+
properties.setProperty(MockTimeBasedSnapshotCleanupPolicy.RETENTION_SNAPSHOT_TIMEZONE,
"UTC");
+ // Mock the cutoff time to be 2024-10-30 01:01:00 UTC
+ cleanupPolicy = new MockTimeBasedSnapshotCleanupPolicy(properties, new
DateTime(2024, 10, 30, 1, 1));
+ }
+
+ @Test
+ public void testShouldDeleteSnapshot() throws IOException {
+
+ // Create a Trash
+ TrashTestBase trashTestBase = new TrashTestBase(new Properties());
+ Trash trash = trashTestBase.trash;
+
+ // Create dummy paths
+ FileStatus fs1 = new FileStatus(0, true, 0, 0, 0,
+ new Path(trash.getTrashLocation(), new
DateTime(2024, 10, 29, 1, 0,
DateTimeZone.UTC).toString(Trash.TRASH_SNAPSHOT_NAME_FORMATTER)));
Review Comment:
speaking of mocks, it seems reasonable to mock the `FileStatus` to only
return `.getPath()`. that demonstrates the check is based solely on the path
itself and not e.g. on the modtime
Issue Time Tracking
-------------------
Worklog Id: (was: 943708)
Time Spent: 1h 40m (was: 1.5h)
> Use isBefore(nowInUTC) to compare snapshots with the current time in UTC.
> -------------------------------------------------------------------------
>
> Key: GOBBLIN-2168
> URL: https://issues.apache.org/jira/browse/GOBBLIN-2168
> Project: Apache Gobblin
> Issue Type: Improvement
> Reporter: Wei-Hsiang Lin
> Priority: Major
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> In existing trash cleaner, the comparison between snapshotTime and the
> current time is NOT done in the same time zone, this leads to longer hard
> deletion time due to time zone difference.
> the fix ensure that the comparison between snapshotTime and the current time
> is done in the same time zone
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
