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-1865] Fix for job id tracking where configuration is unnecessary #3729

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

Will-Lo
Copy link
Contributor

@Will-Lo Will-Lo commented Aug 2, 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):
    With "gobblin.cluster.job.useGeneratedJobIds" configuration, jobs with that prefix should be using the system timestamp of Gobblin cluster instead of a provided flow execution ID.

Instead of this, it is more consistent to append flowExecutionId to a jobName then append a timestamp on top of that, so that all earlystop jobs relating to a flow execution can be tracked.

Now jobNames should have the following structure:
job_ActualJob

The timestamp is needed so that Helix can run concurrent jobs given a job ID.

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"

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2023

Codecov Report

Merging #3729 (07592c1) into master (183bc5c) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #3729      +/-   ##
============================================
+ Coverage     46.99%   47.09%   +0.10%     
- Complexity    10830    10859      +29     
============================================
  Files          2144     2144              
  Lines         84748    84748              
  Branches       9410     9410              
============================================
+ Hits          39824    39910      +86     
+ Misses        41297    41216      -81     
+ Partials       3627     3622       -5     
Files Changed Coverage Δ
...a/org/apache/gobblin/cluster/HelixJobsMapping.java 81.53% <100.00%> (ø)

... and 16 files with indirect coverage changes

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

@Will-Lo Will-Lo merged commit 2087426 into apache:master Aug 2, 2023
6 checks passed
phet pushed a commit to phet/gobblin that referenced this pull request Aug 15, 2023
@Will-Lo Will-Lo deleted the fix-bug-gobblin-cluster-id-fix branch September 26, 2023 21:13
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.

3 participants