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.

Sign up for GitHub

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

Jump to bottom

version: kill version::release() #17820

Closed
wants to merge 1 commit into from
Closed

version: kill version::release() #17820

wants to merge 1 commit into from

Conversation

denesb
Copy link
Contributor

@denesb denesb commented Mar 15, 2024

This method returns an arbitrary version of Cassandra, we consider ourselves most compatible with. At least that is the theory behind it, but saw no update in years, so it is probably stale. This method appears in the output of nodetool version, in the system.versions virtual table, among others. It is even gossiped around and stored in topology metadata for some reason.
It is a regular source of confusion as users and even developers confuse it with the real scylla version. To end the confusion, kill version::release(), replacing all its usage with the version of ScyllaDB proper.

Fixes: scylladb/scylla-tools-java#45

@denesb
version: kill version::release()
cd21f8e 
This method returns an arbitrary version of Cassandra, we consider
ourselves most compatible with. At least that is the theory behind it,
but saw no update in years, so it is probably stale.
This method appears in the output of `nodetool version`, in the
system.versions virtual table, among others. It is even gossiped around
and stored in topology metadata for some reason.
It is a regular source of confusion as users and even developers confuse
it with the real scylla version. To end the confusion, kill
version::release(), replacing all its usage with the version of ScyllaDB
proper.

Fixes: scylladb/scylla-tools-java#45
@denesb denesb added area/nodetool area/internals an issue which refers to some internal class or something which has little exposure to users and is labels Mar 15, 2024
@denesb denesb added this to the 6.0 milestone Mar 15, 2024
@denesb denesb self-assigned this Mar 15, 2024
@denesb denesb requested a review from tgrabiec as a code owner March 15, 2024 11:41
@denesb
Copy link
Contributor Author

denesb commented Mar 15, 2024

@kbr-scylla @asias @bhalevy any idea what the role of version::release() is in toplogy/gossip? Will anything change/break if we use the real version, instead of this stale fake one?

@denesb denesb added the backport/none Backport is not required label Mar 15, 2024
@scylladb-promoter
Copy link
Contributor

🔴 CI State: ABORTED

✅ - Build
✅ - Container Test
✅ - Unit Tests
❌ - dtest
❌ - dtest with topology changes

Failed Tests (73/29677):

Build Details:

  • Duration: 6 hr 40 min
  • Builder: spider7.cloudius-systems.com

@kbr-scylla
Copy link
Contributor

@kbr-scylla @asias @bhalevy any idea what the role of version::release() is in toplogy/gossip? Will anything change/break if we use the real version, instead of this stale fake one?

No idea.

I see we store release_version in system.topology and even take care to update it when restarting nodes, if they change the version...

But does it actually do anything? I don't know.

@gleb-cloudius you originally added it; is it needed for anything? Maybe you just added it because it was one of the things we gossiped, but maybe we could just get rid of it at this point?

@gleb-cloudius
Copy link
Contributor

I added it to raft because is was in the gossiper, but see 5985f22

@gleb-cloudius
Copy link
Contributor

I guess in Cassandra it can be used the way we use features.

@kbr-scylla
Copy link
Contributor

Thanks.

By following pointers there's also #14225

@denesb
Copy link
Contributor Author

denesb commented Mar 18, 2024

Ok, closing then. Looks like we are stuck with this ancient version for now. I will make a change targeted at just nodetool version.

@denesb denesb closed this Mar 18, 2024
@asias
Copy link
Contributor

asias commented Mar 19, 2024

@kbr-scylla @asias @bhalevy any idea what the role of version::release() is in toplogy/gossip? Will anything change/break if we use the real version, instead of this stale fake one?

Does this break the cql driver? IIRC, the version is used by the driver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgrabiec tgrabiec Awaiting requested review from tgrabiec tgrabiec is a code owner

Assignees

@denesb denesb

Labels
area/internals an issue which refers to some internal class or something which has little exposure to users and is area/nodetool backport/none Backport is not required
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.

'nodetool version' shows wrong version
5 participants
@denesb @scylladb-promoter @kbr-scylla @gleb-cloudius @asias