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
[WIP] [BEAM-9120] Deprecate onSuccessMatcher, onCreateMatcher #10589
Conversation
Sorry for late review. I'll take a look at it today |
@@ -102,8 +101,6 @@ DataflowPipelineJob run(Pipeline pipeline, DataflowRunner runner) { | |||
job.getJobId(), | |||
expectedNumberOfAssertions); | |||
|
|||
assertThat(job, testPipelineOptions.getOnCreateMatcher()); |
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.
This makes sure test fail-fast if pipeline creation failed. Do we have alternative solutions?
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.
Does it do that? My understanding was that this would be a no-op by default, since OnCreateMatcher is an instance of AlwaysPassMatcher. It will only fail fast if the user sets an OnCreateMatcher, which I don't think is happening anywhere outside of TestDataflowRunnerTest.
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.
No, it wasn't used in any of the integration tests. I think it's okay to remove since the following waitForxxxJobTermination
is monitoring the job status.
Run Java PreCommit |
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions. |
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
WIP: Do not merge until the various places we use these flags have been removed!
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.