Skip to content

C21328 mutation tracking version#4774

Open
bdeggleston wants to merge 7 commits intoapache:cep-45-mutation-trackingfrom
bdeggleston:C21328-mutation-tracking-version
Open

C21328 mutation tracking version#4774
bdeggleston wants to merge 7 commits intoapache:cep-45-mutation-trackingfrom
bdeggleston:C21328-mutation-tracking-version

Conversation

@bdeggleston
Copy link
Copy Markdown
Member

Thanks for sending a pull request! Here are some tips if you're new here:

  • Ensure you have added or run the appropriate tests for your PR.
  • Be sure to keep the PR description updated to reflect all changes.
  • Write your PR title to summarize what this PR proposes.
  • If possible, provide a concise example to reproduce the issue for a faster review.
  • Read our contributor guidelines
  • If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

@iamaleksey iamaleksey self-requested a review April 28, 2026 11:02
@iamaleksey
Copy link
Copy Markdown
Member

iamaleksey commented Apr 28, 2026

One thing we did when implementing equivalent functionality for Accord was inspect existing serializers and convert the majority of them to the unversioned variant. I expect most of the lower level serializers to not change (ever), and if they ever do, we can convert them to versioned variants one case-by-case basis.

@Override
public void serialize(Version v, DataOutputPlus out, int version) throws IOException
{
Preconditions.checkArgument(v.messagingVersion <= version);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is dead logic? And also Version itself should have an unversioned serializer.


public void skip(DataInputPlus in, int version) throws IOException
{
in.readByte();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can see that this skips the version, but it could be made more clear.

Also, realizing that we are writing out a mutation id (now 1 byte fatter) for all mutations, even if if a keyspace has no mutation tracking enabled, for none(), and I don't think that is fine - that's 17 bytes of waste for folks not opted into MT.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I reckon we could use 0 byte for none() here.

@iamaleksey
Copy link
Copy Markdown
Member

At the high level, I think this is done on a slightly wrong level. MutationTrackingSerializer writes out the version even for nested serializers, redundantly - instead of it being specified just once at the outermost point - just for MT related messages. Accord does this better I believe - with EmbeddedAsymmetricVersionedSerializer, which we could reuse.

It's probably the wrong thing to introduce a different style of sub-messaging-service versioning here. Should mimic how Accord does it, and, where makes sense, reuse some of its code as well - but not have two parallel approaches.

@iamaleksey
Copy link
Copy Markdown
Member

I can make the changes that I have in mind if you like.

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