Skip to content

[GOBBLIN-2037] Start DagActionMonitor functionality after its dependencies#3916

Merged
Will-Lo merged 2 commits intoapache:masterfrom
umustafi:umustafi/moveDagActionMonitorStartup
Apr 5, 2024
Merged

[GOBBLIN-2037] Start DagActionMonitor functionality after its dependencies#3916
Will-Lo merged 2 commits intoapache:masterfrom
umustafi:umustafi/moveDagActionMonitorStartup

Conversation

@umustafi
Copy link
Copy Markdown
Contributor

@umustafi umustafi commented Apr 5, 2024

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):
    The DagActionStoreChangeMonitor cannot be started before the Flowgraph, DagManager, and SpecCompiler are up and running. When the monitor is initialized by Guice, it was previously also starting to load dagActions from the store immediately to compile and process. There is a race condition created where the actions can be loaded too quickly from the store and passed to the specCompiler before it's ready. The SpecCompiler hangs waiting for the flowGraph to load, causing startup to fail. A similar dependency exists between the SpecStoreChangeMonitor and SpecCompiler + Scheduler.

The solution is the bind the monitor in Guice but only enable processing of actions and specs after the GobblinServiceManager ensures other classes are ready to be called by the DagActionStoreChangeMonitor & SpecStoreChangeMonitor.

Tests

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

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"

@umustafi
Copy link
Copy Markdown
Contributor Author

umustafi commented Apr 5, 2024

checks on my branch umustafi#24

Copy link
Copy Markdown
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 sleuth work!

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 4.34783% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 46.64%. Comparing base (125d787) to head (4bd58eb).
Report is 1 commits behind head on master.

Files Patch % Lines
...ervice/monitoring/DagActionStoreChangeMonitor.java 0.00% 12 Missing ⚠️
...lin/service/monitoring/SpecStoreChangeMonitor.java 0.00% 7 Missing ⚠️
...in/service/modules/core/GobblinServiceManager.java 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3916   +/-   ##
=========================================
  Coverage     46.63%   46.64%           
- Complexity    11247    11250    +3     
=========================================
  Files          2248     2248           
  Lines         88512    88520    +8     
  Branches       9700     9701    +1     
=========================================
+ Hits          41281    41288    +7     
- Misses        43500    43503    +3     
+ Partials       3731     3729    -2     

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

@Will-Lo Will-Lo merged commit 1ca4944 into apache:master Apr 5, 2024
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.

5 participants