Allow manifest compaction to generate multiple content pieces #387
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
When compacting manifest data, allow persisting multiple content pieces. This should be alright because flushes on the index are already disabled, which means all changes should appear atomically. Uses a max number of manifests per content piece as that's easily available and can be used as a proxy for total content size to some extent.
Basic tests that check lookups and number of content pieces after various operations.
pandeyabs
approved these changes
Sep 5, 2024
| @@ -18,6 +18,8 @@ import ( | |||
| "github.com/kopia/kopia/repo/content/index" | |||
| ) | |||
|
|
|||
| const maxManifestsPerContent = 1000000 | |||
There was a problem hiding this comment.
Curious, how did we get to this number? Is this assuming some average size per manifest entry from data we have?
| require.NoError(t, err, "compacting manifests") | ||
|
|
||
| foundContents = getManifestContentCount(ctx, t, mgr) | ||
| assert.Equal(t, 2, foundContents) |
vkamra
reviewed
Sep 5, 2024
| @@ -18,6 +18,8 @@ import ( | |||
| "github.com/kopia/kopia/repo/content/index" | |||
| ) | |||
|
|
|||
| const maxManifestsPerContent = 1000000 | |||
There was a problem hiding this comment.
Might be useful (in a follow-up) to make this configurable using an ENV. Allows disabling this feature by setting the value to an arbitrarily high value and tuning it to a lower value if needed
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.
Allow manifest compaction to generate multiple content pieces. This currently uses number of manifests in a content piece as a proxy for the total size the content piece will be. The number of manifests per content piece is a number out of hat that's meant to be high enough to not trigger the creation of multiple content pieces in most cases while still hopefully avoiding errors about content pieces being too large
This change is only enabled during compaction to reduce blast radius and because compaction already disables content manager flushes to ensure content writes and deletions are seen by other clients atomically (at least assuming no other content manager flushes if this returns an error)
(for completeness)
Even if content writes aren't atomic, then I think things would still work out alright if there was concurrent compaction. For example, the following could occur if content writes didn't appear atomically:
The end result would be partial duplication of manifest data as content piece 2 would contain some info present in content pieces 3 and 4. However, content pieces 3 and 4 should still have all manifest entries present in the now-deleted content piece 1 as content piece 1 was included in client B's compaction (that's why it was deleted)