joao-r-reis commented on code in PR #1822:
URL:
https://github.com/apache/cassandra-gocql-driver/pull/1822#discussion_r1804745065
##########
conn.go:
##########
@@ -378,10 +379,19 @@ func (s *startupCoordinator) setupConn(ctx
context.Context) error {
}
defer cancel()
+ // Only for proto v5+.
+ // Indicates if STARTUP has been completed.
+ // github.com/apache/cassandra/blob/trunk/doc/native_protocol_v5.spec
+ // 2.3.1 Initial Handshake
+ // In order to support both v5 and earlier formats, the v5 framing
format is not
+ // applied to message exchanges before an initial handshake is
completed.
+ startupCompleted := &atomic.Bool{}
Review Comment:
This will cause the minimum Go version to be 1.19 (instead of the current
1.17 according to my experiments). I raised this on the ASF slack channel to
see if people think it's ok otherwise we might have to use `atomic.Value`
instead...
##########
conn.go:
##########
@@ -1430,6 +1529,34 @@ func (c *Conn) executeQuery(ctx context.Context, qry
*Query) *Iter {
case *resultVoidFrame:
return &Iter{framer: framer}
case *resultRowsFrame:
+ if x.meta.newMetadataID != nil {
+ // Updating the result metadata id in prepared stmt
+ //
+ // If a RESULT/Rows message reports
+ // changed resultset metadata with the
Metadata_changed flag, the reported new
+ // resultset metadata must be used in subsequent
executions
+
+ stmtCacheKey :=
c.session.stmtsLRU.keyFor(c.host.HostID(), c.currentKeyspace, qry.stmt)
+ oldInflight, ok := c.session.stmtsLRU.get(stmtCacheKey)
+ if !ok {
+ // We didn't find the stmt in the cache, so we
just re-prepare it
+ return c.executeQuery(ctx, qry)
Review Comment:
if we can't find the stmt in the cache here we can probably just return the
rows normally and skip trying the update of the result metadata id. If the user
submits the query again then the driver will re-prepare.
##########
frame.go:
##########
@@ -1556,8 +1578,18 @@ func (f *framer) writeQueryParams(opts *queryParams) {
}
if opts.keyspace != "" {
+ if f.proto < protoVersion5 {
+ panic(fmt.Errorf("the keyspace can only be set with
protocol 5 or higher"))
+ }
f.writeString(opts.keyspace)
}
+
+ if opts.nowInSeconds != nil {
+ if f.proto < protoVersion5 {
+ panic(fmt.Errorf("now_in_seconds can only be set with
protocol 5 or higher"))
+ }
+ f.writeInt(int32(*opts.nowInSeconds))
Review Comment:
NIT: it would make a bit more sense to do these checks and panics earlier in
the function when the header flags are being added (similar to how it was
before).
Same for the batch write function
##########
conn.go:
##########
@@ -1430,6 +1529,34 @@ func (c *Conn) executeQuery(ctx context.Context, qry
*Query) *Iter {
case *resultVoidFrame:
return &Iter{framer: framer}
case *resultRowsFrame:
+ if x.meta.newMetadataID != nil {
+ // Updating the result metadata id in prepared stmt
+ //
+ // If a RESULT/Rows message reports
+ // changed resultset metadata with the
Metadata_changed flag, the reported new
+ // resultset metadata must be used in subsequent
executions
+
+ stmtCacheKey :=
c.session.stmtsLRU.keyFor(c.host.HostID(), c.currentKeyspace, qry.stmt)
+ oldInflight, ok := c.session.stmtsLRU.get(stmtCacheKey)
+ if !ok {
+ // We didn't find the stmt in the cache, so we
just re-prepare it
+ return c.executeQuery(ctx, qry)
+ }
+
+ newInflight := &inflightPrepare{
+ done: make(chan struct{}),
+ preparedStatment: &preparedStatment{
+ id:
oldInflight.preparedStatment.id,
+ resultMetadataID: x.meta.newMetadataID,
+ request:
oldInflight.preparedStatment.request,
+ response: x.meta,
+ },
+ }
+
+ c.session.stmtsLRU.add(stmtCacheKey, newInflight)
+ return c.executeQuery(ctx, qry)
Review Comment:
I don't think this is right, if I understand correctly all we want to do is
update the result metadata ID and use the new result metadata that is on this
response instead of the cached one. I don't think it should trigger a new
execution of the query. @lukasz-antoniak does my description seem accurate
according to the investigation you did while trying to write a test for this?
--
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]
