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: add scylla specific options to schema description #14275

Closed
wants to merge 1 commit into from
Closed

schema: add scylla specific options to schema description #14275

wants to merge 1 commit into from

Conversation

Jadw1
Copy link
Contributor

@Jadw1 Jadw1 commented Jun 16, 2023 •
edited

Add paxos_grace_seconds, tombstone_gc, cdc and synchronous_updates options to schema description.

Fixes: #12389
Fixes: https://github.com/scylladb/scylla-enterprise/issues/2979

@Jadw1 Jadw1 requested review from tgrabiec and nyh as code owners June 16, 2023 13:32
@Jadw1 Jadw1 requested a review from bhalevy June 16, 2023 13:33
@Jadw1
Copy link
Contributor Author

Jadw1 commented Jun 16, 2023

@bhalevy let me know if I missed any option

@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/1895/

bhalevy
bhalevy reviewed Jun 18, 2023
View reviewed changes
schema/schema.cc
@@ -906,8 +917,15 @@ std::ostream& schema::describe(replica::database& db, std::ostream& os, bool wit
os << "\n AND memtable_flush_period_in_ms = " << memtable_flush_period();
os << "\n AND min_index_interval = " << min_index_interval();
os << "\n AND read_repair_chance = " << read_repair_chance();
os << "\n AND speculative_retry = '" << speculative_retry().to_sstring() << "';";
os << "\n";
os << "\n AND speculative_retry = '" << speculative_retry().to_sstring() << "'";
Copy link
Member

Choose a reason for hiding this comment

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

Add speculative_retry, paxos_grace_seconds, cdc and synchronous_updates
options to schema description.

It looks like speculative_retry was already described.

@bhalevy
Copy link
Member

bhalevy commented Jun 18, 2023

@bhalevy let me know if I missed any option

Based on https://github.com/scylladb/scylladb/blob/master/cql3/statements/cf_prop_defs.cc:
paxos_grace_seconds

Also, please also examine operator<<(std::ostream& os, const schema& s) to see if it prints the same information as schema::describe().

@Jadw1
Copy link
Contributor Author

Jadw1 commented Jun 22, 2023 •
edited

Based on https://github.com/scylladb/scylladb/blob/master/cql3/statements/cf_prop_defs.cc: paxos_grace_seconds

paxos_grace_seconds is present in a describe.

Also, please also examine operator<<(std::ostream& os, const schema& s) to see if it prints the same information as schema::describe().

schema::describe() doesn't print: tombstoneGcOptions, keyValidator, minCompactionThreshold, maxCompactionThreshold and it prints cdc only if cdc_options().enabled()

operator<<(...) doesn't print: paxos_grace_seconds, crc_check_chance

I've updated the test to include added options, but the CI won't pass because there is sth wrong with boost test's cql env.

  1. Creating table with specified paxos_grace_seconds doesn't take any effect. The value of std::optional<int32_t> schema::raw_schema::_paxos_grace_seconds is std::nullopt_t
  2. The test cannot read speculative_retry because db::find_tag() segfaults. The entry from schema's extensions is found but casting schema_extension to tags_extension fails. The function uses static_pointer_cast (thus the segfault) and when I've changed it to dynamic cast the result pointer is null. But the original pointer from extensions map is not null.

I'll file issues about those things. Cql pytests works correctly tho.

This was referenced Jun 22, 2023
Closed
Closed
@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2006/

@Jadw1 Jadw1 force-pushed the schema_desc branch 2 times, most recently from c15f330 to 995d0f6 Compare July 3, 2023 16:21
@Jadw1
Copy link
Contributor Author

Jadw1 commented Jul 3, 2023

Fixed cql-test env by simply adding db::extensions to cql_test_config.

Added tombstone_gc: https://github.com/scylladb/scylla-enterprise/issues/2979

@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2206/

@Jadw1
schema: add scylla specific options to schema description
8be3c14 
Add `paxos_grace_seconds`, `tombstone_gc`, `cdc` and `synchronous_updates`
options to schema description.

Fixes: scylladb#12389
Fixes: scylladb/scylla-enterprise#2979
@scylladb-promoter
Copy link
Contributor

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2211/

@Jadw1 Jadw1 requested review from bhalevy and eliransin July 4, 2023 08:21
@Jadw1
Copy link
Contributor Author

Jadw1 commented Jul 18, 2023

@bhalevy can we merge this?

@bhalevy
Copy link
Member

bhalevy commented Jul 18, 2023

@bhalevy can we merge this?

It's not up to me...

@scylladb/scylla-maint please merge

@Jadw1 Jadw1 mentioned this pull request Jan 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denesb denesb denesb approved these changes

@bhalevy bhalevy bhalevy approved these changes

@tgrabiec tgrabiec Awaiting requested review from tgrabiec tgrabiec is a code owner

@nyh nyh Awaiting requested review from nyh nyh is a code owner

@eliransin eliransin Awaiting requested review from eliransin

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

Successfully merging this pull request may close these issues.

Server-side DESCRIBE should include scylladb-specific options
4 participants
@Jadw1 @scylladb-promoter @bhalevy @denesb