CASSGO-49 fix for the cassandra version unmarshal #1836
Conversation
tengu-alt
force-pushed
the
scan-version-fix
branch
from
e976b1a to
f606c21
Compare
host_source.go
Outdated
|
|
||
| type cassVersion struct { | ||
| Major, Minor, Patch int | ||
| AdditionalNotation string |
There was a problem hiding this comment.
In Cassandra (and many Java-based projects), the bit after the dash is often called a qualifier or sometimes a suffix. In semantic versioning, it might be part of a pre-release segment or a build meta after a plus sign. However, Cassandra uses variations like snapshot, rc, etc., which are typically called qualifiers.
So, I suggest changing the name of the AdditionalNotation field to the Qualifier.
joao-r-reis
left a comment
joao-r-reis
left a comment
There was a problem hiding this comment.
Added some comments. Let's get this in gocql 2.0, can you create a JIRA?
host_source.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("invalid minor version %v: %v", v[1], err) | ||
| } | ||
| c.AdditionalNotation = vMinor[1] |
There was a problem hiding this comment.
Should return here as we're parsing the qualifier which should always be at the end.
Also if the "suffix" of the version is something like -qualifier1-qualifier2 we want this field to either contain the full suffix string -qualifier1-qualifier2 or this field should be of type []string and the slice should be [qualifier1,qualifier2]. Here's the relevant code in the java driver.
Personally I think it's fine if we just parse the full suffix string including the first -, if anyone is actually interested in parsing these qualifiers then they can do it on their app. In this case the name of the field should probably be Suffix instead of Qualifier though I think?
There was a problem hiding this comment.
How common will there be multiple qualifiers? I'm fine with Suffix but if there isn't realistically going to be more than one qualifier, then I think Qualifier is still more appropriate.
There was a problem hiding this comment.
Honestly I don't know, I just noticed there's logic to handle this in the java driver and thought we would do the same here but I don't remember the last time I saw a C* version with multiple qualifiers.
tengu-alt
changed the title
tengu-alt
force-pushed
the
scan-version-fix
branch
from
f606c21 to
1b39d4d
Compare
|
@joao-r-reis , @testisnullus I have renamed the
Could you please clarify why we need to contain the first |
|
I'm fine with including the |
|
James suggested we should keep |
Yep! That sounds good to me |
|
👍 @tengu-alt all we need is to rename |
tengu-alt
force-pushed
the
scan-version-fix
branch
from
1b39d4d to
4e2c30f
Compare
FIx for the issue, when the driver is unable to unmarshal Cassandra version, which contains additional annotation (suffix). patch by Oleksandr Luzhniy; reviewed by João Reis, James Hartig, Danylo Savchenko, for CASSGO-49
tengu-alt
force-pushed
the
scan-version-fix
branch
from
4e2c30f to
b7c0ca4
Compare
|
Done, also updated the commit message and the changelog. |
|
@joao-r-reis should it be merged? |
4 participants
Fix for the #910.
This PR fixes issue, when the driver is unable to unmarshal Cassandra version, which contains additional annotation.