Skip to content

Conversation

@arjun4084346
Copy link
Contributor

@arjun4084346 arjun4084346 commented May 11, 2021

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):
    Currently Add/update/delete of flow specs and cancellation of a job execution happen via putting/deleting these specs in job catalog. Listeners of Spec Catalog take appropriate actions. While for add/update/delete this makes sense to modify job catalog, it makes little sense for job execution cancellation request to update job catalog in order to trigger the cancellation because job and execution are two different entities.
    This PR will move cancel-job-triggering logic from job catalog listeners to kafkajob monitor. This will also clean a lot of code which exists due to the need to pass cancellation request from job monitor to job catalog to spec consumer to jobConfigurationManager to jobScheduler

Tests

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

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 May 11, 2021

Codecov Report

Merging #3277 (f1407d8) into master (ae62d77) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3277   +/-   ##
=========================================
  Coverage     46.54%   46.54%           
+ Complexity    10035    10031    -4     
=========================================
  Files          2037     2038    +1     
  Lines         79272    79267    -5     
  Branches       8838     8835    -3     
=========================================
  Hits          36895    36895           
+ Misses        38957    38950    -7     
- Partials       3420     3422    +2     
Impacted Files Coverage Δ Complexity Δ
...che/gobblin/cluster/FsJobConfigurationManager.java 74.41% <ø> (+3.30%) 7.00 <0.00> (ø)
...ache/gobblin/cluster/GobblinHelixJobScheduler.java 33.89% <0.00%> (-0.59%) 6.00 <0.00> (ø)
...pache/gobblin/cluster/JobConfigurationManager.java 78.00% <ø> (+4.41%) 11.00 <0.00> (ø)
...blin/cluster/ScheduledJobConfigurationManager.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...blin/cluster/StreamingJobConfigurationManager.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...apache/gobblin/runtime/api/JobCatalogListener.java 100.00% <ø> (+23.07%) 0.00 <0.00> (ø)
.../apache/gobblin/runtime/api/MutableJobCatalog.java 86.66% <ø> (+5.41%) 0.00 <0.00> (ø)
...n/runtime/job_catalog/JobCatalogListenersList.java 73.68% <ø> (+10.04%) 10.00 <0.00> (ø)
...lin/runtime/job_catalog/JobConfigArrivalEvent.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../runtime/job_catalog/NonObservingFSJobCatalog.java 57.50% <ø> (+1.68%) 5.00 <0.00> (-1.00) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae62d77...f1407d8. Read the comment docs.

Copy link
Contributor

@aplex aplex left a comment

Choose a reason for hiding this comment

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

Can you also update PR description/jira ticket reference?

@arjun4084346 arjun4084346 changed the title move 'cancel trigger code' from job catalog to job monitor [GOBBLIN-1440] move 'cancel trigger code' from job catalog to job monitor May 11, 2021
@aplex
Copy link
Contributor

aplex commented May 14, 2021

@sv2000 , can you merge this?

@arjun4084346
Copy link
Contributor Author

@sv2000, I ran a kafka fast ingestion job, ran a replanner job with gobblin.helix.replanner.sql=select true from gobblinReplannableJob as suggested by @autumnust and found that the ingestion job kept ingesting the data.

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