[GOBBLIN-2168] Compare with the current time in UTC

[linweihs]

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

• My PR addresses the following GOBBLIN-2168

Description

• Here are some details about my PR, including screenshots (if applicable):

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

• To allow configuration of whether to use UTC or the system's default time zone for comparison, introduce a new configuration property gobblin.trash.snapshot.retention.comparison.useUTC.

Tests

• My PR adds the following unit tests :

Implement a unit test class [TimeBasedSnapshotCleanupPolicyTest.java](https://github.com/apache/gobblin/compare/master...linweihs:incubator-gobblin:welin/trash?expand=1#diff-59e30e386f99115a2511854af4f01812ea1cf832430c696bb48182af91713f77) ensuring the comparison are asserted expectedly.

Commits

• My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

[linweihs]

updated the implementation according to comment suggested. thanks for the review

[Will-Lo]

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.

[linweihs]

this is helpful ! i've addressed according to the suggestion, thanks for the reviewing :)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.30%. Comparing base (45ad13e) to head (3ead802).
Report is 22 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4070      +/-   ##
============================================
- Coverage     45.12%   41.30%   -3.82%     
+ Complexity     3199     2233     -966     
============================================
  Files           705      485     -220     
  Lines         26949    20599    -6350     
  Branches       2680     2382     -298     
============================================
- Hits          12160     8508    -3652     
+ Misses        13781    11190    -2591     
+ Partials       1008      901     -107     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[phet]

good impl fix here... but the test needs reworking

[phet]

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 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

[phet]

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