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

Persist snapshot metadata #10121

Merged
merged 20 commits into from
Aug 22, 2022
Merged

Persist snapshot metadata #10121

merged 20 commits into from
Aug 22, 2022

Conversation

deepthidevaki
Copy link
Contributor

@deepthidevaki deepthidevaki commented Aug 18, 2022 •
edited

Description

  • Renamed FileBasedSnapshotMetadata to FileBasedSnapshotId
  • Created a new FileBasedSnapshotMetadata for the newly added metadata
  • AsyncSnapshotDirector updates the metadata before persisting the snapshot
  • While persisting the snapshot, metadata is written as a file in snapshot's directory and the checksum is updated to include this file
  • The metadata file is replicated as a SnapshotChunk. So there is no change in replication protocol. On the receiver side, it can identify if a chunk is a metadata or not from the file name. It uses this information to create the metadata object of this snapshot. The receiver is backward compatible because the older version just write the metadata file to disk as the other SnapshotChunk and just ignore it later. If the snapshot is from the older version, the receiver at new version handles this case by creating an incomplete metadata with known information. This is safe to do.
  • Added new tests and refactored existing test to make it work with the new semantics.

Related issues

closes #10115

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement
  • If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 18, 2022 •
edited

Test Results

   848 files  ±    0     848 suites  ±0   1h 38m 6s ⏱️ -6s
6 298 tests  - 123  6 287 ✔️  - 123  11 💤 ±0  0 ❌ ±0 
6 482 runs   - 123  6 471 ✔️  - 123  11 💤 ±0  0 ❌ ±0 

Results for commit 1c6d09d. ± Comparison against base commit 0588f34.

♻️ This comment has been updated with latest results.

@deepthidevaki
test(snapshots): verify metadata is persisted
8b01094
@deepthidevaki
feat(snapshots): allow sending metadata in replicated snapshot
292f83d
@deepthidevaki
test(snapshots): merge SnapshotChunkReaderTest to FileBasedSnapshotCh…
23878d4 
…unkReaderTest

The test was assuming lots of internal details. So it makes sense to move it to the implementation test.
@deepthidevaki
feat(snapshots): when calculating checksum the metadata file should b…
be38e07 
…e added at the end

When calculating the checksum while taking the snapshot, the metadata file is added at the end. To ensure the same semantics while verifying the checksums, we the metadata file explicitly while calculating the checksum. Otherwise, depending on the name of metadata file the checksum calculated while taking a snapshot and checksum calculated when verifying the snapshot will be different.
@deepthidevaki deepthidevaki marked this pull request as ready for review August 19, 2022 12:28
@deepthidevaki
Copy link
Contributor Author

@oleschoenburg This is a big PR. Let me know if you prefer it split into multiple PRs. If not I think it is best reviewed commit by commit.

lenaschoenburg
lenaschoenburg reviewed Aug 19, 2022
View reviewed changes
Copy link
Member

@lenaschoenburg lenaschoenburg left a comment

Choose a reason for hiding this comment

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

Just an initial pass, I will need some more time to properly review this.

snapshot/src/main/java/io/camunda/zeebe/snapshots/impl/FileBasedSnapshotMetadata.java Outdated Show resolved Hide resolved
snapshot/src/main/java/io/camunda/zeebe/snapshots/impl/FileBasedTransientSnapshot.java Outdated Show resolved Hide resolved
snapshot/src/main/java/io/camunda/zeebe/snapshots/impl/FileBasedTransientSnapshot.java Outdated Show resolved Hide resolved
snapshot/src/test/java/io/camunda/zeebe/snapshots/impl/FileBasedTransientSnapshotTest.java Outdated Show resolved Hide resolved
lenaschoenburg
lenaschoenburg approved these changes Aug 22, 2022
View reviewed changes
Copy link
Member

@lenaschoenburg lenaschoenburg left a comment

Choose a reason for hiding this comment

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

Great work, @deepthidevaki 🏅

I only have minor comments, please consider them before merging.

snapshot/src/main/java/io/camunda/zeebe/snapshots/SnapshotMetadata.java Show resolved Hide resolved
snapshot/src/main/java/io/camunda/zeebe/snapshots/TransientSnapshot.java Show resolved Hide resolved
snapshot/src/main/java/io/camunda/zeebe/snapshots/impl/FileBasedSnapshotId.java Show resolved Hide resolved
snapshot/src/main/java/io/camunda/zeebe/snapshots/impl/FileBasedSnapshotId.java Outdated Show resolved Hide resolved
@deepthidevaki
Copy link
Contributor Author

Thanks @oleschoenburg for the review

bors merge

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit 976c14d into main Aug 22, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the dd-10115-snapshot-metadata branch August 22, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npepinpe npepinpe npepinpe left review comments

@lenaschoenburg lenaschoenburg lenaschoenburg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add last written position as metadata in snapshot
3 participants
@deepthidevaki @npepinpe @lenaschoenburg