content: require backing store for checkpoint #6255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chu11
force-pushed
the
issue6251_content_checkpoint_require_backing
branch
3 times, most recently
from
431e867 to
72db5f3
Compare
This was referenced Sep 5, 2024
Problem: An accidental 'd' was added to remove, making it "removed". Fix spelling.
Problem: A test in t0028-content-backing-none.t incorrectly calls checkpoint_put when it should call checkpoint_get. Fix invalid test.
Problem: The typical message unpack style is to place key names and storage pointers on the same line, but that is not done in several locations in the content and content backing modules. Correct code style to be more consistent to the rest of flux-core.
Problem: A backing store is required for content.flush but it is not required for content.checkpoint-put. This is inconsistent and can lead to checkpointing problems done the line. Require content.checkpoint-put to only work if there is a backing store available. As a consequence, remove code that handled "cached" checkpoints when a backing store is not available. Fixes flux-framework#6251
Problem: Now that the content backing store is required for checkpoints, many tests fail. Remove tests that previously assumed that checkpointing worked without a content backing store. Adjust some tests that now have an new error message.
Problem: There is no coverage to ensure that the "none" backing store works identically to when no backing store is never loaded. Add coverage in t0028-content-backing-none.t.
chu11
force-pushed
the
issue6251_content_checkpoint_require_backing
branch
from
72db5f3 to
9363a9a
Compare
|
I guess one caveat to this change is that, if running without a backing store, you could no longer reload the KVS module without losing all the data. Do we care about that? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6255 +/- ##
==========================================
+ Coverage 83.61% 83.63% +0.01%
==========================================
Files 522 522
Lines 87734 87650 -84
==========================================
- Hits 73356 73302 -54
+ Misses 14378 14348 -30
|
I think it's ok? The reason for this fix was simply the inconsistency. a content-flush + checkpoint-put is (will always be?) a combination to be done. One of those can work without a backing store right now, but the other one can't. So it sort of doesn't make sense. So presumably the alternative would be to try and support content-flush and checkpoint-put if a backing store isn't loaded? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem: A backing store is required for content.flush but it is not required for content.checkpoint-put. This is inconsistent
and can lead to checkpointing problems done the line.
Require content.checkpoint-put to only work if there is a backing store available. As a consequence, remove code that handled "cached" checkpoints when a backing store is not available.