[
https://issues.apache.org/jira/browse/GOBBLIN-2168?focusedWorklogId=943632&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-943632
]
ASF GitHub Bot logged work on GOBBLIN-2168:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 13/Nov/24 18:54
Start Date: 13/Nov/24 18:54
Worklog Time Spent: 10m
Work Description: Will-Lo commented on code in PR #4070:
URL: https://github.com/apache/gobblin/pull/4070#discussion_r1841003482
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/trash/TimeBasedSnapshotCleanupPolicyTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 implements
SnapshotCleanupPolicy {
+
+ public static final String SNAPSHOT_RETENTION_POLICY_MINUTES_KEY =
"gobblin.trash.snapshot.retention.minutes";
+ public static final int SNAPSHOT_RETENTION_POLICY_MINUTES_DEFAULT =
1440; // one day
+ public static final String RETENTION_SNAPSHOT_TIMEZONE =
"gobblin.trash.snapshot.retention.timezone";
+
+ private final int retentionMinutes;
+ private final DateTime MOCK_CURRENT_TIME;
+ private final DateTimeZone retentionSnapshotTimezone;
+
+ public MockTimeBasedSnapshotCleanupPolicy(Properties props, DateTime
mockCurrentTime) {
+ this.retentionMinutes =
Integer.parseInt(props.getProperty(SNAPSHOT_RETENTION_POLICY_MINUTES_KEY,
+ Integer.toString(SNAPSHOT_RETENTION_POLICY_MINUTES_DEFAULT)));
+ this.MOCK_CURRENT_TIME = mockCurrentTime;
+ this.retentionSnapshotTimezone =
props.containsKey(RETENTION_SNAPSHOT_TIMEZONE) ?
DateTimeZone.forID(props.getProperty(RETENTION_SNAPSHOT_TIMEZONE)) :
DateTimeZone.getDefault();
+ }
Review Comment:
Last nit, when it comes to creating a mock class its best to try to reuse as
much as possible from the original class you want to test, otherwise the code
can easily diverge from the `TimeBasedSnapshotCleanupPolicy` if that class is
changed, this test may not capture those changes.
I would recommend this mock class `extend TimeBasedSnapshotCleanupPolicy`
instead of implementing the same interface again, and then you can just add
`private final DateTime MOCK_CURRENT_TIME;` as an additional field and use the
super constructor for the other properties.
Issue Time Tracking
-------------------
Worklog Id: (was: 943632)
Time Spent: 1h 10m (was: 1h)
> 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 10m
> 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)
