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

scylla_node: add support to run scylla types tool #621

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

lkshminarayanan
Copy link
Member

@lkshminarayanan lkshminarayanan commented Oct 17, 2024

This PR implements the run_scylla_types method in the ScyllaNode class
to run the types tool. Also added a new class, ScyllaKeyType to allow
callers to define the key type that is being sent to the types tool.

This is required by scylladb/scylla-dtest#5093 to deduce shards of the
partition keys.

Refs scylladb/scylladb#18011

@lkshminarayanan lkshminarayanan self-assigned this Oct 17, 2024
Raises: subprocess.CalledProcessError if scylla-types serialize returns a non-zero exit code.

"""
if len(values) > len(types):
Copy link

Choose a reason for hiding this comment

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

What about using a dict? That way it cannot get mismatched

Copy link
Member Author

Choose a reason for hiding this comment

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

@pehala We cannot use a dict because we need to allow duplicate keys.

I tried using List[Tuple[str, str]], but if prefixable_compounds is set to True, the length of values can be less than the length of types and the existing code seemed more user friendly for such cases. So, I have left it as it is.

ccmlib/scylla_node.py Outdated Show resolved Hide resolved
ccmlib/scylla_node.py Outdated Show resolved Hide resolved
ccmlib/scylla_node.py Outdated Show resolved Hide resolved
@lkshminarayanan
Copy link
Member Author

Changelog V2 :

  • Removed the separate methods for different actions
  • Added the ScyllaKeyType class to allow define a key type

@lkshminarayanan lkshminarayanan changed the title scylla_node: implement methods to run types tool and actions scylla_node: add support to run scylla types tool Oct 18, 2024
ccmlib/scylla_node.py Outdated Show resolved Hide resolved
ccmlib/scylla_node.py Outdated Show resolved Hide resolved
ccmlib/scylla_node.py Outdated Show resolved Hide resolved
ccmlib/scylla_node.py Outdated Show resolved Hide resolved
@lkshminarayanan lkshminarayanan force-pushed the implement-shardof branch 3 times, most recently from e6cace6 to 321d730 Compare October 21, 2024 09:50
ccmlib/scylla_node.py Outdated Show resolved Hide resolved
@lkshminarayanan lkshminarayanan force-pushed the implement-shardof branch 2 times, most recently from 5bf8d76 to 2c18d02 Compare October 21, 2024 11:50
Copy link
Contributor

@denesb denesb left a comment

Choose a reason for hiding this comment

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

LGTM, one nit remaining.

ccmlib/scylla_node.py Outdated Show resolved Hide resolved
This PR implements the `run_scylla_types` method in the ScyllaNode class
to run the `types` tool. Also added a new class, ScyllaType to allow
callers to define the type that is being sent to the types tool.

This is required by scylladb/scylla-dtest#5093 to deduce shards of the
partition keys.

Refs scylladb/scylladb#18011

Signed-off-by: Lakshmi Narayanan Sreethar <[email protected]>
@lkshminarayanan lkshminarayanan added the status/merge candidate Item needs maintainer attention label Oct 21, 2024
@denesb
Copy link
Contributor

denesb commented Oct 22, 2024

@fruch please review/merge.

@fruch fruch merged commit c7854b0 into scylladb:next Oct 22, 2024
4 checks passed
@lkshminarayanan lkshminarayanan deleted the implement-shardof branch October 22, 2024 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/merge candidate Item needs maintainer attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants