-
Notifications
You must be signed in to change notification settings - Fork 750
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-2174] GoT YarnService Integration with DynamicScaling #4077
base: master
Are you sure you want to change the base?
Conversation
…ve`s with parser
Previous PR review done here - phet#1 Also please ignore initial commit history as those got added while rebasing |
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.
looks really good. are there any mocks that would help test YarnService
?
...l/src/main/java/org/apache/gobblin/temporal/loadgen/dynamic/DummyScalingDirectiveSource.java
Outdated
Show resolved
Hide resolved
...l/src/main/java/org/apache/gobblin/temporal/loadgen/dynamic/DummyScalingDirectiveSource.java
Outdated
Show resolved
Hide resolved
...l/src/main/java/org/apache/gobblin/temporal/loadgen/dynamic/DummyScalingDirectiveSource.java
Outdated
Show resolved
Hide resolved
...l/src/main/java/org/apache/gobblin/temporal/loadgen/dynamic/DummyScalingDirectiveSource.java
Outdated
Show resolved
Hide resolved
gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/YarnService.java
Outdated
Show resolved
Hide resolved
// TODO: remove this line later | ||
// Using for testing purposes only | ||
ScalingDirectiveSource scalingDirectiveSource = new DummyScalingDirectiveSource(); |
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.
would it be helpful for unit testing if, rather than hard-coding, this class took the ScalingDirectiveSource
FQ class name? I see that could be harder based on the ctor params.
As a simpler alternative, make DynamicScalingYarnServiceManger
abstract w/ a method
abstract protected ScalingDirectiveSource createScalingDirectiveSource();
and then the concrete FsSourceDynamicScalingYarnServiceManager
would hard code the ScalingDirectiveSource
class. you could have a different concrete DSYSM
using DummyScalingDirectiveSource
. one of those such FQ class names would be a param.
... which reminds me.... how is this DSYSM
created and initialized at present?
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.
I am using DummyScalingDirectiveSource();
to launch containers at runtime if i run any job to test complete e2e.
... which reminds me.... how is this DSYSM created and initialized at present?
here after starting yarnservice - https://github.com/apache/gobblin/blob/master/gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/GobblinTemporalApplicationMaster.java#L102 we initialize other service classes whose names are passed through config
gobblin/gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java
Line 78 in e5d897e
public static final String APP_MASTER_SERVICE_CLASSES = GOBBLIN_YARN_PREFIX + "app.master.serviceClasses"; |
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.
I agree with point of creating abstract class, let me add that in next commit.
Refactoted the code to use AbstractDSYSM and also added unit tests.
...emporal/src/main/java/org/apache/gobblin/temporal/yarn/DynamicScalingYarnServiceManager.java
Outdated
Show resolved
Hide resolved
gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/DynamicScalingYarnService.java
Outdated
Show resolved
Hide resolved
gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/DynamicScalingYarnService.java
Outdated
Show resolved
Hide resolved
gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/DynamicScalingYarnService.java
Outdated
Show resolved
Hide resolved
Have added unit test for buildContainerCommand |
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-2174
Description
☑️ Here are some details about my PR, including screenshots (if applicable):
ScalingDirective
s with parser andFsScalingDirectiveSource
#4068 , Functionality for enabling GoT Dynamic Scaling was introduced.ScalingDirectives
Tests
☑️ My PR adds the following unit tests OR does not need testing for this extremely good reason:
DummyDynamicScalingYarnServiceManager
which usesDummyScalingDirectiveSource
which returns set of fixed profiles and their set pointsYarnServiceTest
testBaselineWorkerProfileCreatedWithPassedConfigs
andtestBuildContainerCommand
DynamicScalingYarnServiceManagerTest
Logs from container
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"