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

Explicit table naming in examples to prevent errors from shared tables across examples #846

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

nsipplswezey
Copy link
Contributor

@nsipplswezey nsipplswezey commented Oct 17, 2023

The following examples use/reuse the following table name and schema:

"CREATE TABLE IF NOT EXISTS ks.t (a int, b int, c text, primary key (a, b))",

basic
execution_profile
query_history
schema-agreement
select-paging
speculative-execution
tls

The following examples currently use/reuse the ks.t table, with a different schema:
custom_deserializion uses "CREATE TABLE IF NOT EXISTS ks.t (pk int primary key, v text)"
compare-tokens uses "CREATE TABLE IF NOT EXISTS ks.compare_tokens_example (pk bigint primary key)"

Depending on what order the examples are run/rerun locally, the examples can create inconsistent table schemas for ks.t that cause errors in the examples.

This PR changes the keyspace name in all examples to examples_ks and uses the example file name as the table name for all example tables. This should address any errors caused by tables that are shared between examples.

Fixes: #844

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
Copy link
Collaborator

This is going to be a problem again when someone adds new example with conflicting schema - even running examples in CI is not guaranteed to catch that.
Maybe a better option than changing custom_deserializion and compare-tokens would be to change all examples to use table name corresponding to their name, and change keyspace name to e.g. examples?

@nsipplswezey nsipplswezey force-pushed the examples-fix-table-names branch 4 times, most recently from 7af1184 to 30acf5d Compare October 17, 2023 19:25
@nsipplswezey
Copy link
Contributor Author

Makes sense to me. Updated PR to match the suggestion.
I used the name examples_ks for key spaces because it seemed clearer to me in the examples. I couldn't find any strong suggestions for naming conventions, so if examples or some other name is better, it can be changed.

All examples except cloud run without error both on my local and in CI on my fork https://github.com/nsipplswezey/scylla-rust-driver/actions/runs/6551696037/job/17793439625.

@Lorak-mmk
Copy link
Collaborator

Code changes look fine to me. Please update PR description (because it no longer changes just custom_deserializion and compare-tokens) and put more information in the commit message so that future developers don't need to open PR to see motivation for the change ( https://github.com/scylladb/scylla-rust-driver/blob/main/CONTRIBUTING.md )

@nsipplswezey nsipplswezey force-pushed the examples-fix-table-names branch from 30acf5d to c021936 Compare October 18, 2023 20:20
@nsipplswezey
Copy link
Contributor Author

nsipplswezey commented Oct 18, 2023

Done!

#847 might block CI checks.

@Lorak-mmk
Copy link
Collaborator

Yep, could you rebase on main, as #847 is fixed now?

Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Looks fine to me now, I think we can merge after rebasing on main.

@nsipplswezey nsipplswezey force-pushed the examples-fix-table-names branch from c021936 to c8abb78 Compare October 19, 2023 14:41
@nsipplswezey
Copy link
Contributor Author

Done!

examples/auth.rs Show resolved Hide resolved
examples/cloud.rs Show resolved Hide resolved
ks is changed from ks to examples_ks
table names are changed from t to the example file name

Examples share an explicitly named keyspace

Examples now have unique table names preventing errors that occur from two examples sharing the same table
@piodul piodul force-pushed the examples-fix-table-names branch from c8abb78 to 86e42f6 Compare January 12, 2024 16:50
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.

I've rebased the pull request - there were conflicts in cql-time-types.rs which I had to resolve. Let's wait for the CI to complete.

LGTM from my side - @Lorak-mmk, please review and merge at your convenience.

@Lorak-mmk Lorak-mmk merged commit 8c20d39 into scylladb:main Jan 12, 2024
8 checks passed
@avelanarius avelanarius added this to the 0.12.0 milestone Jan 15, 2024
@Lorak-mmk Lorak-mmk mentioned this pull request Feb 1, 2024
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.

Error when running examples/compare-tokens.rs and examples/custom_deserialization.rs
4 participants