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

Support protocol v3 #46

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

josebama
Copy link

Allow a Cassandra protocol_version to be passed in as an argument in the CLI and use that when creating a Cluster. If no protocol is passed in, the default, existing behaviour with continue to happen.

Also, replace toTimestamp with dateof when inserting the DB migration version fi the cluster is using protocol version 3, since toTimestamp was introduced in version 4

@danielkza
Copy link
Contributor

According to the docs the driver should downgrade the protocol version to match the server:

If not set in the constructor, the driver will automatically downgrade version based on a negotiation with the server, but it is most efficient to set this to the maximum supported by your version of Cassandra. Setting this will also prevent conflicting versions negotiated if your cluster is upgraded.

Did that not work in your case? Would the change to avoid using toTimestamp be sufficient?

cassandra_migrate/migrator.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@@ -70,6 +70,8 @@ def main():
migrations are run against. This option must be used in
conjuction with the -k option. This option is ignored
unless the -s option is provided.""")
parser.add_argument('-v', '--protocol-version', type=int, default=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a choices argument here matching the enumeration in ProtocolVersion.SUPPORTED_VERSIONS (docs)

Copy link
Author

@josebama josebama Apr 30, 2019

Choose a reason for hiding this comment

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

OK, I will hardcode versions 1, 2, 3, 4, 5 instead of passing ProtocolVersion.SUPPORTED_VERSIONS to choices, to avoid potential import errors and to keep it separate from how ProtocolVersion might work internally in the future

I haven't marked this conversation as resolved as I'm not 100% sure about the range solution. If you like it please mark it as resolved :). Let me know otherwise

@josebama
Copy link
Author

josebama commented Apr 30, 2019

Did that not work in your case? Would the change to avoid using toTimestamp be sufficient?

It did automatically downgrade version when I was testing locally with Docker, but once I pointed the migrations to my development Cassandras in the cloud, it failed to do so. I guess there is some variation on behaviour depending on the version

@danielkza
Copy link
Contributor

danielkza commented Apr 30, 2019

It did automatically downgrade version when I was testing locally with Docker, but once I pointed the migrations to my development Cassandras in the cloud, it failed to do so. I guess there is some variation on behaviour depending on the version

That's unfortunate, but not really unexpected; I've seen similar issues pretty much every time when running cqlsh from outside the Cassandra machines.


Otherwise, everything looks good, I'll just do some quick testing and get it merged soon. Thank you!

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

Successfully merging this pull request may close these issues.

2 participants