Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GOBBLIN-1920] Use db laundered timestamp for reminder event #3788

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

umustafi
Copy link
Contributor

@umustafi umustafi commented Sep 25, 2023

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

Description

  • Here are some details about my PR, including screenshots (if applicable):
    When setting the reminder event we use the trigger time local to each host. Instead we want to use the timestamp from database to see consistency between hosts in case their clock drift is significant. 

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

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"

Copy link
Contributor

@phet phet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

only improvement to recommend is a comment explaining why this is important

prevJobProps.setProperty(ConfigurationKeys.SCHEDULER_PRESERVED_CONSENSUS_EVENT_TIME_MILLIS_KEY,
String.valueOf(triggerEventTimeMillis));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably remove this from function parameter as well?

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2023

Codecov Report

Merging #3788 (93cfff1) into master (c98ef9d) will decrease coverage by 1.63%.
Report is 1 commits behind head on master.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master    #3788      +/-   ##
============================================
- Coverage     48.96%   47.33%   -1.63%     
- Complexity     3577    10955    +7378     
============================================
  Files           697     2152    +1455     
  Lines         28234    85089   +56855     
  Branches       3292     9452    +6160     
============================================
+ Hits          13824    40278   +26454     
- Misses        13007    41158   +28151     
- Partials       1403     3653    +2250     
Files Coverage Δ
...vice/modules/orchestration/FlowTriggerHandler.java 21.35% <33.33%> (ø)

... and 1463 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@meethngala meethngala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch! I was wondering if it even makes sense for us to provide eventTimeMillis to the method signature of scheduleReminderForEvent. Earlier we were using it to create JobDetail, but now the only place we use it is for logging and the info that it conveys is the clock drift (which we might be okay not logging)... let me know what's your take?

@umustafi
Copy link
Contributor Author

nice catch! I was wondering if it even makes sense for us to provide eventTimeMillis to the method signature of scheduleReminderForEvent. Earlier we were using it to create JobDetail, but now the only place we use it is for logging and the info that it conveys is the clock drift (which we might be okay not logging)... let me know what's your take?

I am leaving it in to catch that clock drift in time being. I suspect it's not the root of our problem but until we are reliably bug free keeping logs in.

@Will-Lo Will-Lo merged commit 028b85f into apache:master Sep 25, 2023
6 checks passed
homatthew pushed a commit to homatthew/gobblin that referenced this pull request Oct 11, 2023
…3788)

* Use db laundered timestamp for reminder event

* Add more details to comment and remove extra param

* remove unused param

---------

Co-authored-by: Urmi Mustafi <umustafi@linkedin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants