[BEAM-4130] Add tests for FlinkJobServerDriver #6703
Conversation
|
|
||
| @Option(name = "--flink-master-url", usage = "Flink master url to submit job.") | ||
| private String flinkMasterUrl = "[auto]"; |
There was a problem hiding this comment.
Why these changes to the modifiers?
There was a problem hiding this comment.
Testing. They are package-local now.
There was a problem hiding this comment.
Few lines further :) There is no need to have a getter, especially not after the visibility of that was changed, too. Somehow it feels as couple recent changes in this area could have benefitted from more thorough review? (BTW I don't expect this to be addressed as part of this PR, but in master.)
There was a problem hiding this comment.
Considering that the code was more or less untested, I think we are much better off.
We can change the getter back to public.
There was a problem hiding this comment.
This is really a minor issue considering that FlinkJobServerDriver is not user-facing at all.
There was a problem hiding this comment.
I would probably remove it, but as it stands it is redundant.
The host default change that broke pipeline submission and this PR reverts was user facing. And yes, test coverage added here puts us into a much better place. Test coverage is always preferred, but looking at the PRs a bit more closely sometimes also helps.
There was a problem hiding this comment.
Yes, we should be careful. That said, all PRs were reviewed and mistakes do happen.
It helps to cut off the release branch a bit earlier so there is some time for testing.
| @@ -169,11 +170,6 @@ public void run() { | |||
| } | |||
| } | |||
|
|
|||
| public String start() throws IOException { | |||
There was a problem hiding this comment.
This should not be removed (I think we need to adjust this cherry-pick as it introduced other issues in master).
This adds a few test cases for FlinkJobServerDriver. It also changes the default host from empty host to `localhost`.
|
@tweise Should be good to go now. |
|
Run Java PreCommit |
|
@aaltay note that we are waiting to merge this PR - it is blocked by unrelated Java pre-commit issues. This PR needs to go into the release. |
|
Run Java PreCommit |
1 similar comment
|
Run Java PreCommit |
This adds a few test cases for FlinkJobServerDriver. It also changes the default
host from empty host to
localhost.Backported from master (cfaa01a)
CC @tweise @angoenka
Post-Commit Tests Status (on master branch)