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.

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

Added uint32 flags for QUERY and BATCH operations for native protocol v5 #1783

Closed
wants to merge 2 commits into from

Conversation

testisnullus
Copy link

@testisnullus testisnullus commented Jul 22, 2024

The pull request #1783 for the Cassandra gocql driver introduces uint32 flags for QUERY and BATCH operations to support native protocol v5. It refactors the code to handle protocol-specific flag settings more efficiently by retaining byte-based flags for protocol versions less than 5 and introducing integer-based flags for protocol version 5 and higher. This refactoring improves code readability and maintainability, preparing the driver for future protocol changes.

frame.go Outdated
Comment on lines 162 to 169
flagValuesV5 uint32 = 0x01
flagSkipMetaDataV5 = 0x02
flagPageSizeV5 = 0x04
flagWithPagingStateV5 = 0x08
flagWithSerialConsistencyV5 = 0x10
flagDefaultTimestampV5 = 0x20
flagWithNameValuesV5 = 0x40
flagWithKeyspaceV5 = 0x80
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about changing only the type of existing flags?
It allows us to avoid such code duplications. We may implement the new way of constructing the flag field as uint32. Also, if the proto version is less than 5, we can just cast it to byte inside the corresponding methods like framer.writeQueryParams.

@testisnullus testisnullus force-pushed the expand-flag-bitmaps branch from 894ab54 to 6bcffe1 Compare July 22, 2024 14:22
frame.go Outdated
Comment on lines 1459 to 1487
switch {
case f.proto < protoVersion5:
if len(opts.values) > 0 {
flagsByte |= byte(flagValues)
}
if opts.skipMeta {
flagsByte |= byte(flagSkipMetaData)
}
if opts.pageSize > 0 {
flagsByte |= byte(flagPageSize)
}
if len(opts.pagingState) > 0 {
flagsByte |= byte(flagPageSize)
}
if opts.serialConsistency > 0 {
flagsByte |= byte(flagWithSerialConsistency)
}
if f.proto > protoVersion2 {
if opts.defaultTimestamp {
flagsByte |= byte(flagDefaultTimestamp)
}
if len(opts.values) > 0 && opts.values[0].name != "" {
flagsByte |= byte(flagWithNameValues)
names = true
}
}
f.writeByte(flagsByte)

case f.proto >= protoVersion5:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also may avoid these code duplications by implementing only the new way of constructing the flag. As I see, we may construct the flag as uint32, and in the end, we may check the proto version. If the version is less than 5 we may cast it to byte and write it as it is.
What do you think about this approach? Also, if we follow this approach, the flagsByte var may be omitted.

Copy link
Author

Choose a reason for hiding this comment

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

Make sense, fixed

@testisnullus testisnullus force-pushed the expand-flag-bitmaps branch from 6bcffe1 to 3b1c6db Compare July 24, 2024 08:04
@testisnullus testisnullus requested a review from worryg0d July 24, 2024 08:04
Copy link
Contributor

@worryg0d worryg0d left a comment

Choose a reason for hiding this comment

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

lgtm

@absurdfarce absurdfarce added the protocol_v5 Tickets related to supporting protocol version 5 label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol_v5 Tickets related to supporting protocol version 5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants