[GOBBLIN-2168] Add TimeBasedSnapshotCleanupPolicyTest and Fix a bug with props.containsKey() instead of props.contains() in Trash class

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

Updated 2024/11/20:

• I found out a bug internally where theres discrepancy between java.utils.Properties and Azkaban prop object during props passing, thats results in parent config is not pass all the way down to TimeBasedSnapshotCleanupPolicy.java in runtime.
• I've reverted TimeBasedSnapshotCleanupPolicy to original code, and tested are expected. the However, i've added unit tests TimeBasedSnapshotCleanupPolicyTest ensuring the test coverage.
• During investigation, also found a bug (in Trash.java) where Trash use props.contains(TRASH_CLASS_KEY)) which leads to unexpected evaluation whereas the intention is to props.containsKey(TRASH_CLASS_KEY))
  
  

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

[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

[linweihs]

Updated 2024/11/20:
I found out a bug internally where theres discrepancy between java.utils.Properties and Azkaban prop object during props passing, thats results in parent config is not pass all the way down to TimeBasedSnapshotCleanupPolicy.java in runtime.

I've reverted TimeBasedSnapshotCleanupPolicy to original code, and tested are expected. the However, i've added unit tests TimeBasedSnapshotCleanupPolicyTest ensuring the test coverage.

During investigation, also found a bug (in Trash.java) where Trash use props.contains(TRASH_CLASS_KEY)) which leads to unexpected evalation whereas the intention is to props.containsKey(TRASH_CLASS_KEY))