[GOBBLIN-1870] Fix missing flow execution id bug causing SQL Error

[umustafi]

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-1870

Description

• Here are some details about my PR, including screenshots (if applicable):
  Seeing the following issue when trying to acquire lease for a flow action:
  java.sql.SQLIntegrityConstraintViolationException: Column 'flow_execution_id' cannot be null"
  Flow action is created in orchestrate function, during refactoring missed executing the line to add flow execution id to metadata to be extracted for multi-active case. Adding a log line as well to check the flow action object created and verify it has all the flow information.

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

Merging #3733 (e4620ab) into master (01036cc) will increase coverage by 1.76%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master    #3733      +/-   ##
============================================
+ Coverage     45.34%   47.10%   +1.76%     
- Complexity     2138    10862    +8724     
============================================
  Files           413     2146    +1733     
  Lines         17956    84783   +66827     
  Branches       2184     9409    +7225     
============================================
+ Hits           8142    39939   +31797     
- Misses         8947    41221   +32274     
- Partials        867     3623    +2756     

Files Changed	Coverage Δ
...vice/modules/orchestration/FlowTriggerHandler.java	2.73% <0.00%> (ø)
...in/service/modules/orchestration/Orchestrator.java	50.54% <100.00%> (ø)

... and 1734 files with indirect coverage changes

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

[phet]

I'm all for whatever logging is needed to understand and debug. just wondering here: couldn't essentially similar logging instead live within tryAcquireLease, which is basically what this message announces is about to be invoked?

e.g. are there too many code paths leading to tryAcquireLease, yet no good way within the method to pinpoint that it was specifically this path?

[phet]

seems an important property to add.

on that note, do we have any existing Orchestrator.orchestrate test case we might be able to augment with an assert that this is in fact set? (e.g. using test inputs that would fail that assert w/o the above code change.)