-
Notifications
You must be signed in to change notification settings - Fork 136
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
Refactor Optional parameters in the constructor of strategy #836
Conversation
Pull latest
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.
lgtm, left a few minor comments that you can feel free to ignore
createStickyPartitionAssignmentStrategyObject(0, 0, null, _clusterName); | ||
} | ||
|
||
private void createStickyPartitionAssignmentStrategyObject(int partitionsPerTask, int partitionFullnessFactorPct, |
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.
nit: rather than adding this extra function, can't you just directly call createStickyPartitionAssignmentStrategy
with "enableElasticTask" set to 'false' and just ignore the returned assignment?
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.
Created a default to avoid modifying some of the defaults everywhere. It was easier to refactor and fix at one place.
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.
nit: can you rename this function to either be the same as createStickyPartitionAssignmentStrategy
or createStickyPartitionAssignmentStrategyWithElasticTaskDisabled
?
...ver/src/test/java/com/linkedin/datastream/server/assignment/TestStickyMulticastStrategy.java
Outdated
Show resolved
Hide resolved
...ver/src/test/java/com/linkedin/datastream/server/assignment/TestStickyMulticastStrategy.java
Show resolved
Hide resolved
.../src/main/java/com/linkedin/datastream/server/assignment/StickyMulticastStrategyFactory.java
Outdated
Show resolved
Hide resolved
...st/java/com/linkedin/datastream/server/assignment/TestPartitionAssignmentStrategyConfig.java
Outdated
Show resolved
Hide resolved
...om/linkedin/datastream/server/assignment/TestLoadBasedPartitionAssignmentStrategyConfig.java
Show resolved
Hide resolved
createStickyPartitionAssignmentStrategyObject(0, 0, null, _clusterName); | ||
} | ||
|
||
private void createStickyPartitionAssignmentStrategyObject(int partitionsPerTask, int partitionFullnessFactorPct, |
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.
nit: can you rename this function to either be the same as createStickyPartitionAssignmentStrategy
or createStickyPartitionAssignmentStrategyWithElasticTaskDisabled
?
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.
LGTM
…#836) There are lots of Optional fields added to the constructor of the strategy. Most of these fields are not really used as Optional. Refactoring the code to make it simpler and easier to maintain. We will make the fields Optional in future on need basis.
There are lots of Optional fields added to the constructor of the strategy. Most of these fields are not really used as Optional. Refactoring the code to make it simpler and easier to maintain. We will make the fields Optional in future on need basis.