Skip to content

Conversation

@dkropachev
Copy link

@dkropachev dkropachev commented Sep 16, 2025

Implement class to store information about all features negotiated.
The main idea is to have a single place where we store that information, instead of spreading all over channel, and ProtocolInitHandler

It is required by #599

@dkropachev dkropachev force-pushed the dk/introduce-protocol-feature-store branch 3 times, most recently from 3380b23 to a367d0a Compare September 16, 2025 15:56
Implement class to store information about all features negotiated.
@dkropachev dkropachev force-pushed the dk/introduce-protocol-feature-store branch from a367d0a to 245ea63 Compare September 16, 2025 16:11
@dkropachev dkropachev force-pushed the dk/introduce-protocol-feature-store branch from 245ea63 to 7c9a37f Compare September 16, 2025 16:16
@dkropachev dkropachev marked this pull request as ready for review September 16, 2025 16:24
@dkropachev dkropachev self-assigned this Sep 16, 2025
Copy link

@Bouncheck Bouncheck left a comment

Choose a reason for hiding this comment

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

LGTM.

There may be very slight performance hit, probably negligible.
Instead of getting specific features directly the getters will now get whole feature store and then the specific feature from it.

@Bouncheck
Copy link

I see that two features had method names changed to populateStartupOptions and loadFromSupportedOptions. If you'd like to enforce a convention for the future features consider making an Interface and turning the existing ones into implementation of it

No need to pull this information all the time from the channel.
Let's cache it when it is populated to the channel and use it.
@dkropachev
Copy link
Author

I see that two features had method names changed to populateStartupOptions and loadFromSupportedOptions. If you'd like to enforce a convention for the future features consider making an Interface and turning the existing ones into implementation of it

I was thinking about the same, loadFromSupportedOptions is static, so it can't go into interface and having interface for only one method does not make sense, so I did not do it.

@dkropachev dkropachev merged commit abd1be8 into scylladb:scylla-4.x Sep 16, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants