-
Notifications
You must be signed in to change notification settings - Fork 751
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-1914] Configurable Gobblin Application Master class for Yarn #3781
[GOBBLIN-1914] Configurable Gobblin Application Master class for Yarn #3781
Conversation
648e2ef
to
c9d1a57
Compare
c9d1a57
to
471a96e
Compare
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #3781 +/- ##
=========================================
Coverage 47.30% 47.30%
- Complexity 10953 10955 +2
=========================================
Files 2152 2152
Lines 85114 85111 -3
Branches 9452 9452
=========================================
+ Hits 40263 40266 +3
+ Misses 41197 41193 -4
+ Partials 3654 3652 -2
... and 10 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
e07ff95
to
3c25ab9
Compare
@@ -342,7 +342,8 @@ public void initializeYarnClients(Config config) { | |||
* @throws IOException if there's something wrong launching the application | |||
* @throws YarnException if there's something wrong launching the application | |||
*/ | |||
public void launch() throws IOException, YarnException, InterruptedException { | |||
public void launch() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it really be checked? e.g. how likely is it that a caller would want to handle CNFE specifically?
we might throw something more general, like IOE... or just wrap it in RuntimeException
. looking at a few other uses of reflection for dynamic instance-construction, I don't see reflection-based failures showing up in the exception signature
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
Show resolved
Hide resolved
@@ -30,6 +30,9 @@ public class GobblinYarnConfigurationKeys { | |||
public static final String GOBBLIN_YARN_PREFIX = "gobblin.yarn."; | |||
|
|||
// General Gobblin Yarn application configuration properties. | |||
public static final String APP_MASTER_CLASS = GOBBLIN_YARN_PREFIX + "app.master.class"; | |||
public static final String DEFAULT_APP_MASTER_CLASS = GobblinApplicationMaster.class.getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are in the same package, so potentially OK... but just noting this introduces a circular dependency between the two classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth calling out. I can't think of a scenario where this would be a problem but open to input
3c25ab9
to
2a5ff84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
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
The current design of the GobblinApplicationMaster has a heavy dependency on Helix task framework. Going forward, we'd like to explore Gobblin on Temporal as an alternative to Helix TF. I am requesting the ability to add specify a custom Application Master class from the Azkaban yarn app launcher to allow experimentation with different GobblinAM implementations that do not rely on Helix.
Tests
Existing unit tests / internal testing
Commits