Skip to content

Conversation

namn-grg
Copy link

@namn-grg namn-grg commented Oct 9, 2025

Close #21

@namn-grg namn-grg requested review from Copilot and removed request for Copilot October 9, 2025 20:59
Copy link
Collaborator

@julio4 julio4 left a comment

Choose a reason for hiding this comment

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

Great, thank you for this contribution!
I would just like to change how we handle tag in different checkpoint instances, but else this looks good to me!

@Copilot Copilot AI review requested due to automatic review settings October 10, 2025 09:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces tagged checkpoint functionality, replacing the previous named barrier system with a more flexible tagging mechanism. The changes allow checkpoints to be tagged with metadata for easier reference and identification in history queries.

  • Replaces named barriers with a flexible tagging system on checkpoints
  • Adds new methods for querying checkpoint history by tags
  • Updates display and debug output to include tag information

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/test_utils/step.rs Updates test utilities to use tagged barriers instead of named barriers
src/payload/ext/checkpoint.rs Adds new methods for querying checkpoint history by tags
src/payload/checkpoint.rs Implements the core tagging functionality and removes named barrier mutation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Collaborator

@karim-agha karim-agha left a comment

Choose a reason for hiding this comment

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

I'm not sure if we should allow changing any kind of state on an existing checkpoint.

In my opinion tags (if we chose to go with this design) should be attached during checkpoint creation and remain immutable for the whole duration of checkpoints lifetime and the lifetime of all its clones.

@julio4
Copy link
Collaborator

julio4 commented Oct 10, 2025

I'm not sure if we should allow changing any kind of state on an existing checkpoint.

In my opinion tags (if we chose to go with this design) should be attached during checkpoint creation and remain immutable for the whole duration of checkpoints lifetime and the lifetime of all its clones.

Hm I agree. Lets make checkpoint immutable by nature. Then we could overload apply/barrier with a _with_tag

@namn-grg
Copy link
Author

I'm not sure if we should allow changing any kind of state on an existing checkpoint.

In my opinion tags (if we chose to go with this design) should be attached during checkpoint creation and remain immutable for the whole duration of checkpoints lifetime and the lifetime of all its clones.

Makes sense, I agree.

  • moved the tag field to inner
  • added _with helpers
  • removed the tag value changing fns (set, clear)

@julio4 julio4 requested a review from karim-agha October 13, 2025 09:33
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.

Tagged checkpoints

3 participants