-
Notifications
You must be signed in to change notification settings - Fork 623
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
Changelog and README updates for 1.7.0 #1826
Conversation
| 1.19 | yes | yes | | ||
| 1.20 | yes | yes | | ||
| 1.22 | yes | yes | | ||
| 1.23 | yes | yes | | ||
|
||
Gocql has been tested in production against many versions of Cassandra. Due to limits in our CI setup we only | ||
test against the latest 2 GA releases. |
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 debated about adding some text here about why we didn't include Cassandra 5.0.x here. We probably need at least a GH issue around support for 5.0.x before we call it good... and then there's the need to get vector support in as well. Anyways, in the end I decided not to include any additional text here but I'm very open to discussion on this point.
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.
As I see, Cassandra 5 is partially supported and works with the driver - #1812. So, it can be added as a partially supported version. However, the vector type is not merged, and native protocol v4 should be used instead of v5 until it is merged.
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 believe that's correct @taaraora. I fully expect the existing functionality of gocql will work pretty well with Cassandra 5.0.x. What concerns me is CASSANDRA-19906. We've been adding support for Cassandra 5.0.x to our test matrices for the other drivers DataStax maintains and we've hit an issue with precisely this problem on every one of them. The PR you reference doesn't have this concern because it also moves to testcontainers, but that's a change (a) I'm not entirely sold on and (b) I certainly don't want to take on in order to get 1.7.0 out the door.
Given all of that I was fine leaving 5.0.0 out for now. There's a fix to that issue coming in 5.0.1 so (barring some other major issue) I'd be fine with adding 5.0.1 to the existing setup once it's officially released (the vote is going on as I write this).
Make sense?
CHANGELOG.md
Outdated
This release is the first after the donation of gocql to the Apache Software Foundation (ASF) | ||
|
||
### Changed | ||
- DRIVER_NAME parameter in STARTUP messages changed to new project URL after donation. This should provide |
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.
suggestion: Should the pull request number and/or Cassandra issue number be mentioned as well? For previous versions the pull request numbers were mentioned in case a user is interested in more details.
nitpick: Technically, the value in the STARTUP message is not a proper URL, as it lacks //
in the beginning (if parsed as URL, everything goes to the path component). It is not a new module path either as we haven't changed the module path yet. Perhaps new project path
or new project name
would be more accurate?
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.
Good points both @martin-sucha! I will absolutely add the PR numbers to the relevant entries and come up with better terminology. Maybe something more generic like "identifier" makes more sense here?
### Changed | ||
- Update DRIVER_NAME parameter in STARTUP messages to a different value intended to clearly identify this | ||
driver as an ASF driver. This should clearly distinguish this release (and future gocql-cassandra-driver | ||
releases) from prior versions. (#1824) |
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.
@martin-sucha Does this wording make more sense to you? I tried to stay away from discussion about what the format of the string was (or is, for that matter) and focused more on the intent behind the change.
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.
Yes, this is much better, thank you!
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.
Looks good to me 👍 Thank you for preparing the change!
Please squash the changes and format the commit message according to https://github.com/apache/cassandra-gocql-driver/blob/trunk/CONTRIBUTING.md#commit-message
### Changed | ||
- Update DRIVER_NAME parameter in STARTUP messages to a different value intended to clearly identify this | ||
driver as an ASF driver. This should clearly distinguish this release (and future gocql-cassandra-driver | ||
releases) from prior versions. (#1824) |
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.
Yes, this is much better, thank you!
patch by Bret McGuire; reviewed by Martin Sucha reference: apache#1826
b8ae252
to
d77c2fc
Compare
+1 |
patch by Bret McGuire; reviewed by Martin Sucha reference: apache#1826
No description provided.