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

Fix concurrent DDL queries #1127

Merged
merged 4 commits into from
Nov 21, 2024
Merged

Fix concurrent DDL queries #1127

merged 4 commits into from
Nov 21, 2024

Conversation

Lorak-mmk
Copy link
Collaborator

Fixes: #1126

This PR implements the third solution from the ones described in the issue.

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 have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@Lorak-mmk Lorak-mmk requested a review from muzarski November 20, 2024 17:40
@Lorak-mmk Lorak-mmk changed the title Fix concurrent DDLs Fix concurrent DDL queries Nov 20, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 20, 2024 •
edited
Loading

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 019cf1f

@Lorak-mmk Lorak-mmk self-assigned this Nov 20, 2024
muzarski
muzarski reviewed Nov 20, 2024
View reviewed changes
Copy link
Contributor

@muzarski muzarski left a comment

Choose a reason for hiding this comment

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

Looks good. Just a small question: I saw that you adjusted the tests marked with #[cfg(cassandra_tests)] as well. Is it necessary? And this implies more general question: should we use this LBP when running tests against Cassandra?

I'm not against that, as it should not cause any problems. WDYT?

scylla/src/utils/test_utils.rs Outdated Show resolved Hide resolved
@Lorak-mmk
Copy link
Collaborator Author

Looks good. Just a small question: I saw that you adjusted the tests marked with #[cfg(cassandra_tests)] as well. Is it necessary? And this implies more general question: should we use this LBP when running tests against Cassandra?

I'm not against that, as it should not cause any problems. WDYT?

Using the ddl method for ddl queries everywhere is imo beneficial, because it allows us to easily find all DDL queries. I had to find them manually to do this PR and it was not a fun task.

What the ddl method does is up for discussion. We can make it not use the LBP in cassandra tests. Do you think there is any benefit in that?

I just had an idea: maybe I should add info to CONTRIBUTING.md that in DDL queries in tests should use the new method?

@muzarski
Copy link
Contributor

Using the ddl method for ddl queries everywhere is imo beneficial, because it allows us to easily find all DDL queries. I had to find them manually to do this PR and it was not a fun task.

That's for sure.

What the ddl method does is up for discussion. We can make it not use the LBP in cassandra tests. Do you think there is any benefit in that?

This is what I had in mind - to adjust the ddl method logic based on whether the tests are run against Scylla or Cassandra. But as I said, I'm not against using the custom LBP for Cassandra as well. Personally, I don't see any benefit in not doing this. We can leave it as is.

I just had an idea: maybe I should add info to CONTRIBUTING.md that in DDL queries in tests should use the new method?

Definitely, let's do that.

@Lorak-mmk
Tests: Add util functions to perform DDL
69f71ba 
Scylla doesn't like executing multiple DDLs from multiple nodes.
To avoid it we will perform DDLs on a single node from all tests.

Functions added in this commit will help do just that.
@Lorak-mmk
Tests: Use DDL helper functions
0a382f8 
This commit changes all DDL queries to use helper functions introduced
in previous commit. Should fix scylladb#1126
@Lorak-mmk
Tests: Remove allow(unused) labels
38ecb16 
Those are no longer necessary.
@Lorak-mmk
CONTRIBUTING.md: Add guidelines for writing integration tests
019cf1f
@Lorak-mmk
Copy link
Collaborator Author

I just had an idea: maybe I should add info to CONTRIBUTING.md that in DDL queries in tests should use the new method?

Definitely, let's do that.

Done. I also changed the struct to be unit struct.

@Lorak-mmk Lorak-mmk requested a review from muzarski November 21, 2024 11:30
@Lorak-mmk Lorak-mmk merged commit 64647f9 into scylladb:main Nov 21, 2024
11 checks passed
@Lorak-mmk Lorak-mmk mentioned this pull request Dec 4, 2024
Open
wprzytula pushed a commit to wprzytula/scylla-rust-driver that referenced this pull request Dec 11, 2024
@Lorak-mmk @wprzytula
Merge pull request scylladb#1127 from Lorak-mmk/fix-concurrent-ddl
ccc58ce 
Fix concurrent DDL queries

(cherry picked from commit 64647f9)
@wprzytula wprzytula mentioned this pull request Dec 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muzarski muzarski muzarski approved these changes

Assignees

@Lorak-mmk Lorak-mmk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI: "group 0 concurrent modification"
2 participants
@Lorak-mmk @muzarski