Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
[GOBBLIN-2174] GoT YarnService Integration with DynamicScaling #4077
Changes from 8 commits
8b8bb5a
8eb3d4e
a788a8f
f1c859c
7e7113c
0528f71
8a51f6c
64eb6e2
7094023
9ccf8ae
606349e
0c87a75
c7a6d1a
3883837
c478eb5
15396fc
e511968
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 methodand then the concrete
FsSourceDynamicScalingYarnServiceManager
would hard code theScalingDirectiveSource
class. you could have a different concreteDSYSM
usingDummyScalingDirectiveSource
. 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.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
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.