-
Notifications
You must be signed in to change notification settings - Fork 582
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
Remove deprecated IncrementInstanceEnabled property #6001
Conversation
uuidInstanceEnabled is the updated property
...t/java/org/springframework/cloud/dataflow/composedtaskrunner/ComposedRunnerVisitorTests.java
Show resolved
Hide resolved
else if(this.composedTaskProperties.isUuidInstanceEnabled()) { | ||
builder.incrementer(new UuidIncrementer()); | ||
} | ||
builder.incrementer(new UuidIncrementer()); |
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.
Shouldn't this be guarded by this.composedTaskProperties.isUuidInstanceEnabled()
?
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.
Absolutely. And the default of the isUuidInstanceEnabled needs to be set to true.
GREAT CATCH!
...org/springframework/cloud/dataflow/composedtaskrunner/properties/ComposedTaskProperties.java
Show resolved
Hide resolved
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.
Thanks for the cleanup @cppwfs . Couple of small comments/suggestions.
* Guard adding incrementer to the job with the isUuiIdInstanceEnabled * Set uuideInstanceEnabled to true by default since it is now the default incrementer
@@ -109,7 +109,9 @@ public Job getObject() throws Exception { | |||
.start(createFlow()) | |||
.end()) | |||
.end(); | |||
builder.incrementer(new UuidIncrementer()); | |||
if(this.composedTaskProperties.isUuidInstanceEnabled()) { |
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.
[TinyNit] I am not sure if our formatter needs updating, but I am suprised it is not balking about space after if
.
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.
Will polish. Thanks for noticing!
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.
Thanks for the updates @cppwfs !
@onobc Thank you for the great review! Polished, Squashed, Rebased, Merged. |
uuidInstanceEnabled is the updated property