Skip to content

Token range ownership#944

Merged
mambocab merged 1 commit into
apache:masterfrom
damien-instaclustr:token-fix
Apr 27, 2018
Merged

Token range ownership#944
mambocab merged 1 commit into
apache:masterfrom
damien-instaclustr:token-fix

Conversation

@damien-instaclustr
Copy link
Copy Markdown

@damien-instaclustr damien-instaclustr commented Apr 18, 2018

According to Cassandra:

A Range is responsible for the tokens between (left, right]. **

However, TokenMap.get_replicas() appears to implement [left, right).

This pull request aims to resolve this disagreement.

** Source: https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/dht/Range.java

@datastax-bot
Copy link
Copy Markdown

datastax-bot commented Apr 18, 2018

Hi @damien-instaclustr, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

Sincerely,
DataStax Bot.

@datastax-bot
Copy link
Copy Markdown

datastax-bot commented Apr 18, 2018

Thank you @damien-instaclustr for signing the Contribution License Agreement.

Cheers,
DataStax Bot.

@beltran
Copy link
Copy Markdown
Contributor

beltran commented Apr 18, 2018

This looks good to me, also add that the values in self.ring correspond to the end of the token range up to and including the value listed.

@damien-instaclustr
Copy link
Copy Markdown
Author

damien-instaclustr commented Apr 18, 2018

Thanks @bjmb
I've updated the pull request to include the comment.

@beltran
Copy link
Copy Markdown
Contributor

beltran commented Apr 20, 2018

Thank you @damien-instaclustr , can you verify I've correctly explained the issue in the ticket: https://datastax-oss.atlassian.net/browse/PYTHON-978? Feel free to add something otherwise.

@damien-instaclustr
Copy link
Copy Markdown
Author

damien-instaclustr commented Apr 22, 2018

Thanks @bjmb for creating the ticket. I tried to create a ticket before submitting the PR, but wasn't able to because I was hitting the jira's account limit. Looks like the limit will also prevent me from directly contributing to the ticket aswell.
However, the description looks good to me.

@beltran
Copy link
Copy Markdown
Contributor

beltran commented Apr 23, 2018

Thank you @damien-instaclustr , added the changelog entry

@beltran beltran self-requested a review April 23, 2018 14:54
@beltran beltran requested a review from mambocab April 23, 2018 14:54
@mambocab mambocab mentioned this pull request Apr 24, 2018
Closed
mambocab
mambocab suggested changes Apr 24, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@mambocab mambocab left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, great catch. I've filed #944 so the full test suite will run. On this PR, I'm seeing a failure in Appveyor, I believe on the test for this behavior: https://ci.appveyor.com/project/DataStax/python-driver/build/2761/job/0xb91781er6p7fd0?fullLog=true#L2570

So -1 for the moment -- any idea what's up with this @damien-instaclustr ?

@damien-instaclustr
Copy link
Copy Markdown
Author

damien-instaclustr commented Apr 26, 2018

Yes. It looks like there was another test I missed that assumed the token range ownership was [left, right).
I have pushed a change to the test which should correct this.

@mambocab - Do you mind reviewing again?

@mambocab
Copy link
Copy Markdown
Contributor

mambocab commented Apr 26, 2018

Great, thanks! I've rebased my other branch onto master to bring in some test fixes (see #948). Could you squash this history down to one commit? If the tests run clean, I'll merge. (I can also squash on my end; just let me know.)

@mambocab
Copy link
Copy Markdown
Contributor

mambocab commented Apr 26, 2018

Tests ran clean!

@damien-instaclustr
Copy link
Copy Markdown
Author

damien-instaclustr commented Apr 26, 2018

Thanks @mambocab. I have squashed my commits.

@mambocab mambocab merged commit defd9eb into apache:master Apr 27, 2018
@mambocab
Copy link
Copy Markdown
Contributor

mambocab commented Apr 27, 2018

Many thanks!

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

Reviewers

2 more reviewers

@mambocab mambocab mambocab approved these changes

@beltran beltran beltran approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@damien-instaclustr @datastax-bot @beltran @mambocab