Skip to content
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

fixes [21715] - fix ring range read for CassandraIO read() #21786

Conversation

vmarquez
Copy link
Contributor

@vmarquez vmarquez commented Jun 10, 2022

fixes #21715 (missing data in CassandraIO)

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@asf-ci
Copy link

asf-ci commented Jun 10, 2022

Can one of the admins verify this patch?

1 similar comment
@asf-ci
Copy link

asf-ci commented Jun 10, 2022

Can one of the admins verify this patch?

@vmarquez vmarquez force-pushed the bugfix/21715/fix_cassandraio_wraparound_tokens branch from 48b5298 to 1e7fc95 Compare June 16, 2022 04:32
@vmarquez vmarquez changed the title WIP: fixing the missing wrap around ring range read fixing the missing wrap around ring range read for #21715 Jun 16, 2022
@vmarquez vmarquez changed the title fixing the missing wrap around ring range read for #21715 fixing the missing wrap around ring range read for CassandraIO read() Jun 16, 2022
@pabloem
Copy link
Member

pabloem commented Jun 16, 2022

Run Java PostCommit

@vmarquez vmarquez force-pushed the bugfix/21715/fix_cassandraio_wraparound_tokens branch 2 times, most recently from fc53b8a to 3e10063 Compare June 17, 2022 03:24
@vmarquez
Copy link
Contributor Author

@pabloem I fixed a query. Looks ok now. Could you or maybe @damccorm help out with a review if possible? Thanks.

@vmarquez vmarquez force-pushed the bugfix/21715/fix_cassandraio_wraparound_tokens branch from 3e10063 to b8041fc Compare June 17, 2022 17:08
@pabloem
Copy link
Member

pabloem commented Jun 21, 2022

Run Java PostCommit

@brucearctor
Copy link
Contributor

Run Java PreCommit

@vmarquez
Copy link
Contributor Author

@brucearctor thanks, looks like the unrelated, flaky test now passed. What are the next steps to getting this merged?

@brucearctor
Copy link
Contributor

I haven't worked with this component [ cassandraIO ] -- so hoping either for more context, or for someone that's worked closer with it to step in.

Is there an associated issue [jira/github] that this relates to/addresses. That would be a start to provide context on what this is/why - which is also useful more generally for anyone looking through the code/changes/history.

Ex, from the PR template: Mention the appropriate issue in your description (for example: "addresses https://github.com/apache/beam/pull/123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment "fixes #" instead.

@vmarquez
Copy link
Contributor Author

@brucearctor thanks. I had "fixing" instead of "fixes" so I guess it didn't auto pick up the issue it addresses, but at the top you should see the issue it's for: #21715

@brucearctor
Copy link
Contributor

brucearctor commented Jun 24, 2022

@KriKroff you wrote --> https://github.com/KriKroff/beam-cassandraio-bug

Can you take a look and verify this PR addresses this issue?

In principle LGTM, but hoping for a set of eyes more familiar with the component/issue.

@vmarquez
Copy link
Contributor Author

Thanks for taking a look, really appreciate it @brucearctor

@vmarquez vmarquez changed the title fixing the missing wrap around ring range read for CassandraIO read() fixes [21715] - fix ring range read for CassandraIO read() Jul 12, 2022
@pabloem
Copy link
Member

pabloem commented Jul 15, 2022

so sorry. I can't look at this.

@pabloem
Copy link
Member

pabloem commented Jul 15, 2022

r: @Abacn

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

Copy link
Contributor

@Abacn Abacn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The added unit test passed, and verified it fails on current master with java.lang.AssertionError: counting/Flatten.PCollections.out: expected:<100> but was:<99>, which means this change fixed the problem

@brucearctor brucearctor merged commit 2ba5317 into apache:master Jul 15, 2022
@vmarquez
Copy link
Contributor Author

@Abacn Thank you so much for the review! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data missing when using CassandraIO.Read
5 participants