[GOBBLIN-2027] move DagActionStore, MultiActiveLeaseArbiter and their implementation/test classes from gobblin-runtime to gobblin-service

[arjun4084346]

move LeaseAttemptStatus out of MultiActiveLeaseArbiter interface

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 JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
  • https://issues.apache.org/jira/browse/GOBBLIN-2027

Description

• Here are some details about my PR, including screenshots (if applicable):
  This PR will move DagActionStore, MultiActiveLeaseArbiter and their implementation/test classes from gobblin-runtime to gobblin-service module, because DagAction is a concept used exclusively in Gobblin Service, not in core gobblin.
  Also move LeaseAttemptStatus out of MultiActiveLeaseArbiter class.

This change also helped add a method getDagNodeId() in DagAction class, which would otherwise not be possible due to circular dependencies between the modules.

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]

Codecov Report

❌ Patch coverage is 44.73684% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.60%. Comparing base (de50eaf) to head (49aa9ac).
⚠️ Report is 227 commits behind head on master.

Files with missing lines	Patch %	Lines
...vice/modules/orchestration/LeaseAttemptStatus.java	50.00%	7 Missing ⚠️
...rvice/modules/orchestration/FlowLaunchHandler.java	0.00%	5 Missing ⚠️
...odules/orchestration/InstrumentedLeaseArbiter.java	0.00%	4 Missing ⚠️
.../service/modules/orchestration/DagActionStore.java	0.00%	2 Missing ⚠️
...hestration/ReminderSettingDagProcLeaseArbiter.java	0.00%	2 Missing ⚠️
...es/orchestration/MysqlMultiActiveLeaseArbiter.java	85.71%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3904      +/-   ##
============================================
- Coverage     46.61%   46.60%   -0.02%     
+ Complexity    11219    11216       -3     
============================================
  Files          2241     2241              
  Lines         88342    88344       +2     
  Branches       9673     9673              
============================================
- Hits          41184    41169      -15     
- Misses        43447    43465      +18     
+ Partials       3711     3710       -1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[phet]

looks really good... just some minor feedback

also, are you thinking to make any of the other mods suggested here into a third PR? such as:

• reworking DagProc to remove the type param and perhaps the commit method too
• changing the KJSM::addJobStatusToStateStore method (as suggested below)
• (and similar)

[phet]

since they're part and parcel of the MALA abstraction, I'd personally keep these as static inner classes. did you find a particular motivation to move them?

either way is not a big deal actually...

[arjun4084346]

The only motivation was that there were too many (inner) classes in one file, total 4.
and to access LeaseObtainedStatus, one would have to write MultiActiveLeaseArbiter.LeaseObtainedStatus while imo LeaseAttemptStatus.LeaseObtainedStatus makes more sense, because it is closer to LeaseObtainedStatus than to MultiActiveLeaseArbiter

[phet]

it's atypical to have a derived class defined inside the class it's extending. is this borne of a desire to keep these all together, rather than having a separate top-level file for each?

(I do agree that separating into four separate files would be far less comprehensible.)

honestly, what I liked about having these defined inside MultiActiveLeaseArbiter was that they lived all together (as they do here) AND that they also all resided alongside the sole abstraction that uses them (the MALA itself).

[umustafi]

I agree that it's much easier to understand these classes when lie together in one file. I am okay with them being separated from the MALA file since removing the prefix from every place they are used makes code more readable and concise.

[arjun4084346]

After thinking about the structure, here's my opinion,
It maybe (?) atypical for a derived class be inside the parent class, but it looks more atypical for an interface to contain an abstract parent class and extended classes.
Being able to write LeaseAttemptStatus.LeaseObtainedStatus looked like a determining factor for choosing this structure. This is like how enums are addressed too.

[phet]

actually, seems you're moving more than just DagActionStore, so title of commit/PR could be updated

[umustafi]

I agree that it's much easier to understand these classes when lie together in one file. I am okay with them being separated from the MALA file since removing the prefix from every place they are used makes code more readable and concise.

[umustafi]

I agree with renaming the PR bc most of the changes are related to moving LeaseAttemptStatus to its own file. Let's also add javadoc for NoLongerLeasing