-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-222] Respect checkpointing contract in TestCountingSource #235
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
Conversation
|
test? |
|
I'm going to follow your advice in the issue and nuke this entirely. Stand by. |
|
It's Friday, I'm already 3 steps removed from the problem I set out to solve. |
|
Looks like the test is flaky: from this log |
|
Sorry, more accurately, looks like this may have caused a different test using this source to become flaky. |
| } else { | ||
| return false; | ||
| } | ||
| return current < numMessagesPerShard; |
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.
the indentation looks off here, but maybe it's a tabs-vs-spaces thing? Except we shouldn't have tabs.. hmm.. can you please investigate?
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.
Fixed.
…st cirtainly would have resulting in global meltdown. Here's to you maven-checkstyle-plugin.
|
Fixed flaky test - it was a real bug that I introduced so that's good. |
|
R: @dhalperi |
| public void testRespectsCheckpointContract() throws IOException { | ||
| TestCountingSource source = new TestCountingSource(3); | ||
| PipelineOptions options = PipelineOptionsFactory.create(); | ||
| TestCountingSource.CountingSourceReader reader = source.createReader(options, null); |
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.
null -> null /* no checkpoint */
| return current < numMessagesPerShard; | ||
| } | ||
|
|
||
| @Override |
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.
Ensure that all the get/close calls ensure that start has been called.
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.
As above.
|
LGTM Need to fund getting rid of TestCountingSource in favor of CountingSource and having a test artifact |
|
Agree. On Mon, Apr 25, 2016 at 4:03 PM, lukecwik notifications@github.com wrote:
|
This is a (partial) backport of: apache/beam#132 apache/beam#235 apache/beam#248
[euphoria-hbase] fix bulkloading
* Feat: Adding composite and base job (#215) * Added python_release_candidate job. * Testing pr from rc_tag. * Adding GH_TOKEN env var * Set PR head as WORKING_BRANCH * Removed unused code. * Adding Run RC Validation job in CI.md doc file. * Removed Set Environment Variables step. Setting variables in env property. * Removed composite rc-validation action. * Rc validation workflow for Dataflow Taxi, Python Cross Validation and Runners (#227) * RC Validation Workflow (#228) Adding the following jobs: * sql_taxi_with_dataflow * python_cross_validation * generate_shared_pubsub * java_injector * direct_runner_leaderboard * dataflow_runner_leaderboard * direct_runner_gamestats * dataflow_runner_gamestats * remove_shared_pubsub * Adding extras jobs to CI.MD (#230) * Fixing workflow linter error (#231) * Using inputs in composite action (#235) * Using inputs in composite action Co-authored-by: Elias Segundo <elias.segundo@luisrazo.local> * Added Verify Working Branch step. * Adding extras jobs to CI.MD (#230) Co-authored-by: Elias Segundo Antonio <eliassegundo.segundo@gmail.com> Co-authored-by: Elias Segundo <elias.segundo@luisrazo.local>
* chore: release 2.0.0-dev2 * Update CHANGELOG.md, manually separate dev2 changes Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Christopher Wilcox <crwilcox@google.com>
R: @dhalperi