Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4921
merging.
---
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4921
+1
---
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4921
LGTM +1
---
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/4921
I've addressed your comments @zentol. Additionally I've rebased onto the
latest master and did a force push. Sorry for that.
---
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/4921
Thanks for the review @zentol. I've changed the
`RequiredParameters#applyTo` method as you suggested and it was a less
impacting change.
---
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/4921
Fixed the `RequiredParametersTest` by making `RequiredParameters#applyTo`
create a new `ParameterTool` whenever changes are needed. Note, however, that
this code is not very performant since we
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/4921
Thanks for the review @zentol. Yes you're right that this PR has failing
test cases. I wasn't aware that the `ParameterTool` is so messy (getters with
side effects, storing default values for fu
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4921
Travis failure:
```
Tests run: 17, Failures: 0, Errors: 6, Skipped: 0, Time elapsed: 0.029 sec
<<< FAILURE! - in org.apache.flink.api.java.utils.RequiredParametersTest
testApplyToWithOptio
