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

[DRAFT] move cloud snitches to external python script #444

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

syuu1228
Copy link
Contributor

This is scylla-machine-image part of "move cloud snitches to external python script" PR.
Currently this PR is DRAFT version since it doesn't implemented Ec2MultiRegionSnitch yet.


Since it's hard to debug scylla-core's cloud snitch support, we want to move them out to python script.
On the python script we will generate cassandra-rackdc.properties file, and scylla-core will read it via GossipingPropertyFileSnitch code.

See scylladb/scylladb#12306

Since it's hard to debug scylla-core's cloud snitch support, we want to move
them out to python script.
On the python script we will generate cassandra-rackdc.properties file,
and scylla-core will read it via GossipingPropertyFileSnitch code.

See scylladb/scylladb#12306
@syuu1228
Copy link
Contributor Author

scylla-core part of the PR is at: scylladb/scylladb#13623

@syuu1228
Copy link
Contributor Author

I implemented JavaPropertiesParser on scylla_cloud_snitch, it's modified version of sysconfig_parser, since the format is very similar.
I wanted it just for read / write cassandra-rackdc.properties, without doing regexp stuff for each read / write, since it would be hard to maintain.

But there is a pip module to parse java properties https://pypi.org/project/javaproperties/, maybe we can consider to use it.

@syuu1228
Copy link
Contributor Author

TODO:

  • Implement Ec2MultiRegionSnitch
    • find a way to configure broadcast_address, broadcast_rpc_address from the script. I think we need to modify scylla.yaml to do that
  • Implement exponential backoff on curl(). Currently it just sleep 5 sec for each retries


def start(self):
# Split "us-east-1a" or "asia-1a" into "us-east"/"1a" and "asia"/"1a".
splited_zone = self.__aws_instance.availability_zone().rsplit('-', 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is only the legacy naming support

I think we should add the support for both:
https://cassandra.apache.org/doc/latest/cassandra/configuration/cass_rackdc_file.html#ec2_naming_scheme

and consider changing the default to match cassandra 4.x

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's not that lot of code, but I would recommending having unittests coverage for it.

@mykaul
Copy link

mykaul commented Apr 23, 2023

@avelanarius - hopefully, the drivers may be able to (re)use this functionality as well.

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.

3 participants