-
Notifications
You must be signed in to change notification settings - Fork 642
Virtual keyspace metadata support immplementation #1794
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
base: trunk
Are you sure you want to change the base?
Conversation
558eec1 to
5bcb70b
Compare
| const stmt = ` | ||
| SELECT | ||
| table_name, | ||
| comment | ||
| FROM system_virtual_schema.tables | ||
| WHERE keyspace_name = ?` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please fix indent:
| const stmt = ` | |
| SELECT | |
| table_name, | |
| comment | |
| FROM system_virtual_schema.tables | |
| WHERE keyspace_name = ?` | |
| const stmt = ` | |
| SELECT | |
| table_name, | |
| comment | |
| FROM system_virtual_schema.tables | |
| WHERE keyspace_name = ?` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, fixed
| const stmt = ` | ||
| SELECT | ||
| table_name, | ||
| column_name, | ||
| clustering_order, | ||
| kind, | ||
| type | ||
| FROM system_virtual_schema.columns | ||
| WHERE keyspace_name = ?` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same ident problem:
| const stmt = ` | |
| SELECT | |
| table_name, | |
| column_name, | |
| clustering_order, | |
| kind, | |
| type | |
| FROM system_virtual_schema.columns | |
| WHERE keyspace_name = ?` | |
| const stmt = ` | |
| SELECT | |
| table_name, | |
| column_name, | |
| clustering_order, | |
| kind, | |
| type | |
| FROM system_virtual_schema.columns | |
| WHERE keyspace_name = ?` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| func (s *virtualSchemaDescriber) getSchema(keyspaceName string) (*VirtualKeyspaceMetadata, error) { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| metadata, found := s.cache[keyspaceName] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you invalidate it at some point.
Say you upgrade cluster from one version to another, driver will still keep old schema.
Probably on handleNodeUp you could read release_version select release_version from system.local where key = 'local' and invalidate or update cache if it has changed.
Another problem if your cluster half on new version half on another.
Now you depends on where have your control connection landed, if it has landed on a node with new version, session will give you a new schema, while old nodes don't support it.
Way to solve it is to run queries not on control connection, but rather on the node with oldest version available, if you were do so, you can't pick all the nodes from the cluster, you have to adhere to HostSelectionPolicy, say if it is dcAwareRR you will need to filter out all nodes from other datacenters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your point about invalidation, but virtual metadata is only requested on the user's purpose, not by some node event, also virtual tables can't be changed, if we talk about some schemaAgreement case. In my opinion, it is unnecessary to invalidate it on every getSchema request, and i think it is unnecessary to remove a control connection, it is good to keep the existing pattern of the metadata requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you clear cache a the betting of getSchema it means that there is going to be no cache, why just not drop it.
If you drop the cache then there is no point in having virtualSchemaDescriber, you can just move code to Session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with you, but in my opinion, it is good to keep an existing pattern of processing the metadata (virtual or not)
| mu sync.Mutex | ||
| cache map[string]*VirtualKeyspaceMetadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend, in this particular case, use sync.Map instead of Mutex + map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is preferable to use mutex, to keep the existing pattern, and avoid unnecessary code complexity.
5bcb70b to
4b78420
Compare
4b78420 to
4c3aefe
Compare
4c3aefe to
cd6cadd
Compare
This PR provides access to the virtual metadata tables exposed through the
system_virtual_schemakeyspace.Example query:
The
system_virtual_schemakeyspace containskeyspaces,tables, andcolumnsthat users can now parse by specifying a keyspace.Users can retrieve parsed tables using the
VirtualKeyspaceMetadata(keyspace string)method for theSession.The implementation is based on how the driver retrieves regular
keyspace metadata.