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

Switch to use server side schema description for KeyspaceMetadata.ToCQL #210

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Switch to use server side schema description for KeyspaceMetadata.ToCQL #210

merged 1 commit into from
Jul 29, 2024

Conversation

dkropachev
Copy link
Collaborator

No need to keep client-side logic which needs to be updated every time there is changes on server-side.

Closes #195

@dkropachev dkropachev force-pushed the dk/rfe-make-ks-metadata-to-cql-use-server-response branch 3 times, most recently from 1b845a0 to 28ecd39 Compare July 7, 2024 11:01
@sylwiaszunejko
Copy link
Collaborator

Does it work against all supported versions of Scylla? Fallback to the old logic is not necessary?

@dkropachev dkropachev force-pushed the dk/rfe-make-ks-metadata-to-cql-use-server-response branch from 28ecd39 to 6b659da Compare July 10, 2024 05:49
@dkropachev
Copy link
Collaborator Author

Does it work against all supported versions of Scylla? Fallback to the old logic is not necessary?

Tested on both old scylla and old cassandra, in both cases errFrame code ends up being a ErrCodeSyntax.
Added logic to backup to construct statement on client side in such case.

sylwiaszunejko
sylwiaszunejko previously approved these changes Jul 10, 2024
View reviewed changes
@sylwiaszunejko
Copy link
Collaborator

I think it LGTM, @wprzytula maybe you could give a second opinion?

wprzytula
wprzytula requested changes Jul 10, 2024
View reviewed changes
testdata/recreate/udt_golden.cql Outdated
Comment on lines 2 to 1
'class': 'SimpleStrategy',
'replication_factor': '2'
};

CREATE KEYSPACE gocqlx_udt WITH replication = {'class': 'org.apache.cassandra.locator.SimpleStrategy', 'replication_factor': '2'} AND durable_writes = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

SimpleStrategy is deprecated. Let's not use it anywhere and rather migrate to NetworkTopologyStrategy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just expected result of .ToCQL(), schema is in other cql files, tcs[id].Input.
For sake of not polluting this PR, let's have a separate issue on switching from SimpleStrategy to NetworkTopologyStrategy

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will you open the issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#217

testdata/recreate/table_golden.cql Outdated Show resolved Hide resolved
testdata/recreate/table_golden.cql Outdated
Comment on lines 20 to 42
AND speculative_retry = '99.0PERCENTILE'
AND paxos_grace_seconds = 864000
AND tombstone_gc = {'mode': 'timeout', 'propagation_delay_in_seconds': '3600'};
CREATE TABLE gocqlx_table.monkeyspecies (
species text PRIMARY KEY,
species text,
average_size int,
common_name text,
population varint
population varint,
PRIMARY KEY (species)
) WITH bloom_filter_fp_chance = 0.01
AND caching = {'keys':'ALL','rows_per_partition':'ALL'}
AND caching = {'keys': 'ALL', 'rows_per_partition': 'ALL'}
AND comment = 'Important biological records'
AND compaction = {'class':'SizeTieredCompactionStrategy'}
AND compression = {'sstable_compression':'org.apache.cassandra.io.compress.LZ4Compressor'}
AND compaction = {'class': 'SizeTieredCompactionStrategy'}
AND compression = {'sstable_compression': 'org.apache.cassandra.io.compress.LZ4Compressor'}
AND crc_check_chance = 1
AND default_time_to_live = 0
AND gc_grace_seconds = 864000
AND max_index_interval = 2048
AND memtable_flush_period_in_ms = 0
AND min_index_interval = 128
AND speculative_retry = '99.0PERCENTILE';

AND speculative_retry = '99.0PERCENTILE'
AND paxos_grace_seconds = 864000
AND tombstone_gc = {'mode': 'timeout', 'propagation_delay_in_seconds': '3600'};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand how these changes are related to the PR description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just expected result of .ToCQL(), schema is in other cql files, tcs[id].Input.
For sake of not polluting this PR, let's have a separate issue on switching from SimpleStrategy to NetworkTopologyStrategy
Since now we read it from server side result has changed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we 100% sure that in tests we would always use the server-side schema description? (i.e. under no circumstances will fallback to client-side description fire up)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will take care of it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added logic to the test to handle both cases.

testdata/recreate/aggregates_golden.cql Outdated Show resolved Hide resolved
metadata_scylla.go
Comment on lines +758 to +760
if !session.useSystemSchema {
return nil, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I read some code and it seems to be an option to support ancient Cassandra versions without system_schema keyspace. If so (please check my findings), do we really won't to keep this? Maybe we should file a PR to upstream that deletes this logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is fallback logic in ToCQL()

metadata_scylla.go Outdated
if !session.useSystemSchema {
return nil, nil
}
rows, err := session.control.query(fmt.Sprintf(`DESCRIBE KEYSPACE %s WITH INTERNALS`, keyspaceName)).SliceMap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where this CQL syntax comes from? Can you provide a source? And put it in a comment for future readers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately nor scylla neither cassandra have a good page with this exact syntax that could be referred.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So where did you get this syntax from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#195

metadata_scylla.go Outdated Show resolved Hide resolved
metadata_scylla.go Outdated Show resolved Hide resolved
recreate.go
Comment on lines +25 to +27
if len(km.CreateStmts) != 0 {
return km.CreateStmts, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I dislike the heuristic that if km.CreateStmts is empty, then server does not support DESCRIBE KEYSPACE. But it seems to work, so let it be.

recreate_test.go Outdated Show resolved Hide resolved
@dkropachev dkropachev force-pushed the dk/rfe-make-ks-metadata-to-cql-use-server-response branch from 6b659da to 3bef1ea Compare July 11, 2024 02:49
@sylwiaszunejko
Copy link
Collaborator

@wprzytula ping

@dkropachev
Copy link
Collaborator Author

@wprzytula ping

I need to address one problem he pointed out

@dkropachev dkropachev force-pushed the dk/rfe-make-ks-metadata-to-cql-use-server-response branch 4 times, most recently from a803fe6 to 0c53cd0 Compare July 20, 2024 03:38
@dkropachev
Switch to use server side schema description for KeyspaceMetadata.ToCQL
dcc0f67 
No need to keep client-side logic which needs to be updated every time
there is changes on server-side.
@dkropachev dkropachev force-pushed the dk/rfe-make-ks-metadata-to-cql-use-server-response branch from 0c53cd0 to dcc0f67 Compare July 20, 2024 12:01
@wprzytula
Copy link
Collaborator

@wprzytula ping

I need to address one problem he pointed out

Has this been resolved?

@sylwiaszunejko sylwiaszunejko merged commit 8a812b2 into scylladb:master Jul 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wprzytula wprzytula wprzytula approved these changes

@sylwiaszunejko sylwiaszunejko sylwiaszunejko left review comments

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

Successfully merging this pull request may close these issues.

RFE: make ToCQL use server side DESCRIBE SCHEMA
3 participants
@dkropachev @sylwiaszunejko @wprzytula