worryg0d commented on code in PR #1926:
URL:
https://github.com/apache/cassandra-gocql-driver/pull/1926#discussion_r2716002293
##########
metadata.go:
##########
@@ -519,6 +551,39 @@ func getKeyspaceMetadata(session *Session, keyspaceName
string) (*KeyspaceMetada
return keyspace, nil
}
+// getAllKeyspaceNames queries system tables for all keyspace names.
+func getAllKeyspaceNames(session *Session) ([]string, error) {
Review Comment:
Basically, only the query statements are different in both branches, so it
is possible to simplify the whole function to:
```go
var queryStmt string
if session.useSystemSchema { // Cassandra 3.x+
queryStmt = `SELECT keyspace_name FROM system_schema.keyspaces`
} else { // Cassandra 2.x
queryStmt = `SELECT keyspace_name FROM system.schema_keyspaces`
}
iter := session.control.query(stmt)
var keyspaces []string
var keyspaceName string
for iter.Scan(&keyspaceName) {
keyspaces = append(keyspaces, keyspaceName)
}
if err := iter.Close(); err != nil {
return nil, fmt.Errorf("error querying keyspace names: %v", err)
}
...
```
Also, I'm unsure if we have to support Cassandra < 3, as we don't even have
it in our CI. I don't say you should remove this, we can leave this for now to
be consistent with other schema related functions
##########
metadata.go:
##########
@@ -278,6 +278,38 @@ func (s *schemaDescriber) getSchema(keyspaceName string)
(*KeyspaceMetadata, err
return metadata, nil
}
+// returns schema metadata of all the cached keyspaces
+func (s *schemaDescriber) getSchemas() ([]*KeyspaceMetadata, error) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ schemas := make([]*KeyspaceMetadata, 0, len(s.cache))
+ for _, metadata := range s.cache {
+ schemas = append(schemas, metadata)
+ }
+ return schemas, nil
Review Comment:
error return value seems to be redundant; it is never returned anything
except `nil.`
##########
session.go:
##########
@@ -598,6 +606,23 @@ func (s *Session) KeyspaceMetadata(keyspace string)
(*KeyspaceMetadata, error) {
return s.schemaDescriber.getSchema(keyspace)
}
+// GetAllKeyspacesMetadata returns metadata for all keyspaces in the cluster.
+// Uses cached metadata where available.
+func (s *Session) GetAllKeyspacesMetadata() ([]*KeyspaceMetadata, error) {
+ if s.Closed() {
+ return nil, ErrSessionClosed
+ }
+ return s.schemaDescriber.getSchemas()
+}
+
Review Comment:
Looks like `shemaDescriber.getSchemas()` method doesn't query C* to retrieve
schema information, but returns only the cached one, so the comment `Uses
cached metadata where available` is not entirely true.
I think we should update cached metadata when `GetAllKeyspacesMetadata()` is
called, as it will return outdated schema metadata to the token-aware policy
itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
