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.

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

make version non-optional #697

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

mr-git
Copy link
Contributor

@mr-git mr-git commented Nov 6, 2024

No description provided.

@coveralls
Copy link

coveralls commented Nov 6, 2024

@dfakhritdinov dfakhritdinov force-pushed the m/make-version-non-optional branch from 4c0f880 to 0b8a879 Compare November 12, 2024 13:48
@@ -96,25 +72,25 @@ object ActionHeader {
final case class Append(
range: SeqRange,
origin: Option[Origin],
version: Option[Version],
version: Version = Version.obsolete,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, we do not need here and in 3 other places the default value, right?

All clients with versions since 0.0.152 have to provide these in payload over Kafka. Oldest client we have is 0.0.177, IIRC, thus all of them must provide version field, right?

I checked the history for those journal/src/test/resources/com/evolutiongaming/kafka/journal and related unit-tests - they are more than 6 years old, they were introduced in version 0.0.10 - I am leaning on upgrading them to state/format, which is expected at 0.0.177 - then we will require these fallbacks

Other option would be to introduce protocol version, but that could be overkill at this moment

@dfakhritdinov dfakhritdinov marked this pull request as draft November 14, 2024 09:33
@dfakhritdinov
Copy link
Contributor

dfakhritdinov commented Nov 14, 2024

This PR contains braking changes in public API (including some case classes). I see no nice & simple option to overcome this by making desired change backward compatible and propose postponing the PR til next major version bump.

cc @mr-git

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.

3 participants