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.

Sign up for GitHub

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

Jump to bottom

Schema agreement #223

Merged
merged 4 commits into from
Apr 27, 2021
Merged

Schema agreement #223

merged 4 commits into from
Apr 27, 2021

Conversation

Kejmer
Copy link
Contributor

@Kejmer Kejmer commented Apr 19, 2021 •
edited

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I added appropriate Fixes: annotations to PR description.

Fixes: #83

Created waiting for schema agreement mechanism for Session and setting the interval of that check in SessionBuilder.
User can now check local schema version and await for schema agreement with timeout or with endless timeout.
New methods: Session::fetch_schema_version, Session::await_schema_agreement, Session::await_timed_schema_agreement, SessionBuilder::schema_agreement_interval.

I'm not really sure what kind of tests should I provide to test that feature, so I'm asking now if adding line that awaits schema agreement in existing test is satisfactory? @psarna // solved

@Kejmer Kejmer requested a review from psarna April 19, 2021 11:55
@psarna
Copy link
Contributor

psarna commented Apr 19, 2021

@Kejmer we don't have any environment for automated distributed tests (yet), so yes, it's satisfactory. But, please also test manually that this code works on a multi-node cluster as well. If I remember correctly, I sent the instructions to run a local multi-node cluster with ccm to @havaker once, so he might be able to give some more guidance (https://github.com/scylladb/scylla/wiki/Using-CCM)

@Kejmer Kejmer force-pushed the schema_agreement branch 3 times, most recently from aead6cb to 181e685 Compare April 19, 2021 12:33
piodul
piodul requested changes Apr 19, 2021
View reviewed changes
examples/schema_agreement.rs Outdated Show resolved Hide resolved
scylla/src/transport/session.rs Outdated Show resolved Hide resolved
scylla/src/transport/session.rs Outdated Show resolved Hide resolved
scylla/src/transport/session.rs Outdated Show resolved Hide resolved
scylla/src/transport/session.rs Outdated Show resolved Hide resolved
@Jasperav
Copy link
Contributor

@Kejmer we don't have any environment for automated distributed tests (yet), so yes, it's satisfactory. But, please also test manually that this code works on a multi-node cluster as well. If I remember correctly, I sent the instructions to run a local multi-node cluster with ccm to @havaker once, so he might be able to give some more guidance (https://github.com/scylladb/scylla/wiki/Using-CCM)

@psarna we can add a cluster to the CI like this: https://github.com/Jasperav/docker-compose-check/blob/master/docker-compose.yml. Sadly Scylla sometimes fails to startup with docker compose :( scylladb/scylladb#8458

@psarna
Copy link
Contributor

psarna commented Apr 21, 2021

Yeah, eventually we'd like to have a multi-node cluster CI, but it shouldn't block this series. @Kejmer please rebase on top of master, since there are some conflicts (probably due to events getting merged)

@Kejmer
Add a method to session that checks if all nodes have agreed schema v…
5584f44 
…ersion.

User can check what is the current schema version with `fetch_schema_version` method.
There are two ways of waiting for schema agreement, with or without timeout.
Without timeout `await_schema_agreement` can be used to create a co_await that user can wait on
as long as schema is not in an agreement, there is no return value.
With timeout user can't wait longer than provided timeout in miliseconds.
If timeout is reached function returns false as a return value, true otherwise.

User can specify interval that driver checks the schema agreement by setting
`schema_agreement_interval` in SessionBuilder.

Simple schema agreement example

Showcase how to use methods introduced in schema_agreemnet pull request.
@Kejmer
Schema agreement mdbook entry
ed24151
@Kejmer
Simple tests for schema agreement functions.
254d960
@Kejmer Kejmer requested a review from piodul April 21, 2021 18:47
@Kejmer
Copy link
Contributor Author

Kejmer commented Apr 22, 2021

@psarna @piodul me and @havaker confirmed that schema agreement works on multi node cluster. Driver sleeps for a moment if immediately after creating new keyspace we'll await for schema agreement.

piodul
piodul reviewed Apr 23, 2021
View reviewed changes
Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

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

Left several minor comments, but apart from that - looks good.

scylla/src/transport/session.rs Outdated Show resolved Hide resolved
book/src/queries/schema_agreement.md Outdated Show resolved Hide resolved
book/src/queries/schema_agreement.md Outdated Show resolved Hide resolved
scylla/src/transport/connection.rs Show resolved Hide resolved
scylla/src/transport/session_builder.rs Outdated Show resolved Hide resolved
scylla/src/transport/session.rs Outdated Show resolved Hide resolved
@psarna
Copy link
Contributor

psarna commented Apr 26, 2021 •
edited

@piodul were all your review comments addressed or is something still pending?

@piodul
Copy link
Collaborator

piodul commented Apr 26, 2021

@piodul were all your review comments addressed or is something still pending?

No, comments from my second review are still pending. However, they are mostly nitpicks with respect to style and documentation - the implementation itself looks good, so if you want to merge now then I think I'm OK with fixing the rest in a followup.

@Kejmer
Copy link
Contributor Author

Kejmer commented Apr 26, 2021

Sorry I missed notification about new review, I'll finish it as fast as possible

@Kejmer
Copy link
Contributor Author

Kejmer commented Apr 26, 2021

I think it's done, @piodul @psarna sorry for trouble

piodul
piodul reviewed Apr 27, 2021
View reviewed changes
book/src/queries/schema_agreement.md Outdated Show resolved Hide resolved
book/src/queries/schema_agreement.md Outdated Show resolved Hide resolved
scylla/src/transport/session_builder.rs Outdated Show resolved Hide resolved
@Kejmer
Reorganize schema agreement query execution
1b1532c 
Now schema agreemnt works on connection level so it can use the same connection to get information what
is the local schema_version and what are peers' schema_versions.

Timed schema_agreement await takes now Duration as an argument so it will be more descriptive at execution.

Made check_schema_agreement into a public method.
@piodul piodul merged commit 86ac0f7 into scylladb:main Apr 27, 2021
@rukai rukai mentioned this pull request Nov 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piodul piodul piodul approved these changes

@psarna psarna Awaiting requested review from psarna

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a way to wait for schema agreement
4 participants
@Kejmer @psarna @Jasperav @piodul