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

Add a class to pass SeqNr and snapshot payload in #563

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Conversation

rtar
Copy link
Contributor

@rtar rtar commented Feb 5, 2024

The main question here is if we want to have payload as an Option or require it always to be filled.

This idea of Option was taken from a similar Event case class, but I am not sure it applies the same way to a snapshot.

The main question here is if we want to have `payload` as an `Option` or
require it always to be filled.

This idea of `Option` was taken from a similar `Event` case class, but
I am not sure it applies the same way to a snapshot.
@rtar rtar added this to the snapshotter milestone Feb 5, 2024
@coveralls
Copy link

coveralls commented Feb 5, 2024

Pull Request Test Coverage Report for Build 7801971852

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.2%) to 81.497%

Files with Coverage Reduction New Missed Lines %
journal/src/main/scala/com/evolutiongaming/kafka/journal/HeadCache.scala 2 84.62%
eventual-cassandra/src/main/scala/com/evolutiongaming/kafka/journal/eventual/cassandra/ReplicatedCassandra.scala 7 90.37%
Totals Coverage Status
Change from base Build 7759385047: -0.2%
Covered Lines: 3277
Relevant Lines: 4021

💛 - Coveralls

@Z1kkurat
Copy link
Contributor

Z1kkurat commented Feb 6, 2024

I struggle to imagine a case where we'd need a snapshot without a state inside. It looks like a tombstone, but we don't need those for persistent storage such as Cassandra, do we? We either have a snapshot with a state, or we delete the row altogether

@rtar
Copy link
Contributor Author

rtar commented Feb 6, 2024

@Z1kkurat I copied from Event structure.

I now suspect it was added there as part of the c6d0543 commit to ensure backwards compatibility with the previously stored binary snapshot.

I will rework it to remove Option now.

CC: @dfakhritdinov

It was inherited from original `Event` code, but it seems that it
was only needed there to ensure backwards compatibility when introducing
JSON payloads and no longer necessary.

See c6d0543 for more details.
@rtar
Copy link
Contributor Author

rtar commented Feb 6, 2024

I have removed Option now (and also removed it from a large PR, which I am deconstructing into these smaller PRs, but you do not have to look there: #532).

@rtar rtar merged commit 59f2fb6 into master Feb 8, 2024
3 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.

3 participants