Repository: kopia/kopia
Pull request #3240 (2023-08-21 06:03:40)

feat(repository): allow configuring auto compaction count
Adds a manager option to configure the threshold after which auto compaction kicks in.

This is useful when there are large number of concurrent writers in a repository and the default threshold
causes auto-compaction to thrash.

In a follow-up PR - will allow configuring this in repository open.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kopia/kopia/pull/3240?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) Report
Patch coverage: **`100.00%`** and project coverage change: **`-0.02%`** :warning:
> Comparison is base [(`9237b29`)](https://app.codecov.io/gh/kopia/kopia/commit/9237b29023b462b6d18ba411c6ec208385377db3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) 75.43% compared to head [(`a164f11`)](https://app.codecov.io/gh/kopia/kopia/pull/3240?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) 75.42%.
> Report is 5 commits behind head on master.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #3240      +/-   ##
==========================================
- Coverage   75.43%   75.42%   -0.02%     
==========================================
  Files         459      459              
  Lines       36534    36550      +16     
==========================================
+ Hits        27560    27568       +8     
- Misses       7056     7059       +3     
- Partials     1918     1923       +5     
```


| [Files Changed](https://app.codecov.io/gh/kopia/kopia/pull/3240?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) | Coverage Δ | |
|---|---|---|
| [repo/manifest/committed\_manifest\_manager.go](https://app.codecov.io/gh/kopia/kopia/pull/3240?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-cmVwby9tYW5pZmVzdC9jb21taXR0ZWRfbWFuaWZlc3RfbWFuYWdlci5nbw==) | `82.05% <100.00%> (+0.07%)` | :arrow_up: |
| [repo/manifest/manifest\_manager.go](https://app.codecov.io/gh/kopia/kopia/pull/3240?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-cmVwby9tYW5pZmVzdC9tYW5pZmVzdF9tYW5hZ2VyLmdv) | `92.85% <100.00%> (+0.19%)` | :arrow_up: |

... and [13 files with indirect coverage changes](https://app.codecov.io/gh/kopia/kopia/pull/3240/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/kopia/kopia/pull/3240?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).

jkowalski: LGTM as-is, but i'd like to rethink the whole auto-compaction business and perhaps move it to maintenance. I don't want to block what you're trying to do here, so let's work on maintenance separately.
vkamra: > LGTM as-is, but i'd like to rethink the whole auto-compaction business and perhaps move it to maintenance. I don't want to block what you're trying to do here, so let's work on maintenance separately.

Agree with moving to maintenance. @ashmrtn and I were looking into this with our workload (lots of concurrent writers) and the approach we've been discussing is leveraging the flag here to disable compaction during backup and let maintenance run with default parameters.

Maintenance does end up triggering manifest compaction today (indirectly) as a result of [this](https://github.com/kopia/kopia/blob/master/repo/maintenance/maintenance_params.go#L117) call
jkowalski: yeah, we should make it explicit, so that only maintenance ever calls it.
julio-lopez: @vkamra can you rebase please? We are not able to do it.
vkamra: > @vkamra can you rebase please? We are not able to do it.

Yep - I got this. Will push shortly.

Patch:
diff --git a/repo/manifest/committed_manifest_manager.go b/repo/manifest/committed_manifest_manager.go
index 672c7d52..a392300e 100644
--- a/repo/manifest/committed_manifest_manager.go
+++ b/repo/manifest/committed_manifest_manager.go
@@ -33,6 +33,10 @@ type committedManifestManager struct {
 	committedEntries map[ID]*manifestEntry
 	// +checklocks:cmmu
 	committedContentIDs map[content.ID]bool
+
+	// autoCompactionThreshold controls the threshold after which the manager auto-compacts
+	// manifest contents
+	autoCompactionThreshold int
 }
 
 func (m *committedManifestManager) getCommittedEntryOrNil(ctx context.Context, id ID) (*manifestEntry, error) {
@@ -218,7 +222,7 @@ func (m *committedManifestManager) maybeCompactLocked(ctx context.Context) error
 
 	// Don't attempt to compact manifests if the repo was opened in read only mode
 	// since we'll just end up failing.
-	if m.b.IsReadOnly() || len(m.committedContentIDs) < autoCompactionContentCount {
+	if m.b.IsReadOnly() || len(m.committedContentIDs) < m.autoCompactionThreshold {
 		return nil
 	}
 
@@ -357,16 +361,17 @@ func loadManifestContent(ctx context.Context, b contentManager, contentID conten
 	return man, errors.Wrapf(err, "unable to parse manifest %q", contentID)
 }
 
-func newCommittedManager(b contentManager) *committedManifestManager {
+func newCommittedManager(b contentManager, autoCompactionThreshold int) *committedManifestManager {
 	debugID := ""
 	if os.Getenv("KOPIA_DEBUG_MANIFEST_MANAGER") != "" {
 		debugID = fmt.Sprintf("%x", rand.Int63()) //nolint:gosec
 	}
 
 	return &committedManifestManager{
-		b:                   b,
-		debugID:             debugID,
-		committedEntries:    map[ID]*manifestEntry{},
-		committedContentIDs: map[content.ID]bool{},
+		b:                       b,
+		debugID:                 debugID,
+		committedEntries:        map[ID]*manifestEntry{},
+		committedContentIDs:     map[content.ID]bool{},
+		autoCompactionThreshold: autoCompactionThreshold,
 	}
 }
diff --git a/repo/manifest/manifest_manager.go b/repo/manifest/manifest_manager.go
index 6357b8bb..9b02459d 100644
--- a/repo/manifest/manifest_manager.go
+++ b/repo/manifest/manifest_manager.go
@@ -32,8 +32,8 @@
 
 // ContentPrefix is the prefix of the content id for manifests.
 const (
-	ContentPrefix              = "m"
-	autoCompactionContentCount = 16
+	ContentPrefix                     = "m"
+	autoCompactionContentCountDefault = 16
 )
 
 // TypeLabelKey is the label key for manifest type.
@@ -264,7 +264,8 @@ func copyLabels(m map[string]string) map[string]string {
 
 // ManagerOptions are optional parameters for Manager creation.
 type ManagerOptions struct {
-	TimeNow func() time.Time // Time provider
+	TimeNow                 func() time.Time // Time provider
+	AutoCompactionThreshold int
 }
 
 // NewManager returns new manifest manager for the provided content manager.
@@ -276,11 +277,16 @@ func NewManager(ctx context.Context, b contentManager, options ManagerOptions, m
 		timeNow = clock.Now
 	}
 
+	autoCompactionThreshold := options.AutoCompactionThreshold
+	if autoCompactionThreshold == 0 {
+		autoCompactionThreshold = autoCompactionContentCountDefault
+	}
+
 	m := &Manager{
 		b:              b,
 		pendingEntries: map[ID]*manifestEntry{},
 		timeNow:        timeNow,
-		committed:      newCommittedManager(b),
+		committed:      newCommittedManager(b, autoCompactionThreshold),
 	}
 
 	return m, nil


Test patch:
diff --git a/repo/manifest/manifest_manager_test.go b/repo/manifest/manifest_manager_test.go
index f063e473..bd28a4d0 100644
--- a/repo/manifest/manifest_manager_test.go
+++ b/repo/manifest/manifest_manager_test.go
@@ -29,7 +29,7 @@ func TestMain(m *testing.M) { testutil.MyTestMain(m) }
 func TestManifest(t *testing.T) {
 	ctx := testlogging.Context(t)
 	data := blobtesting.DataMap{}
-	mgr := newManagerForTesting(ctx, t, data)
+	mgr := newManagerForTesting(ctx, t, data, ManagerOptions{})
 
 	item1 := map[string]int{"foo": 1, "bar": 2}
 	item2 := map[string]int{"foo": 2, "bar": 3}
@@ -89,7 +89,7 @@ func TestManifest(t *testing.T) {
 
 	// flush underlying content manager and verify in new manifest manager.
 	mgr.b.Flush(ctx)
-	mgr2 := newManagerForTesting(ctx, t, data)
+	mgr2 := newManagerForTesting(ctx, t, data, ManagerOptions{})
 
 	for _, tc := range cases {
 		verifyMatches(ctx, t, mgr2, tc.criteria, tc.expected)
@@ -140,7 +140,7 @@ func(ci content.Info) error {
 
 	mgr.b.Flush(ctx)
 
-	mgr3 := newManagerForTesting(ctx, t, data)
+	mgr3 := newManagerForTesting(ctx, t, data, ManagerOptions{})
 
 	verifyItem(ctx, t, mgr3, id1, labels1, item1)
 	verifyItem(ctx, t, mgr3, id2, labels2, item2)
@@ -340,12 +340,12 @@ func newContentManagerForTesting(ctx context.Context, t *testing.T, data blobtes
 	return bm
 }
 
-func newManagerForTesting(ctx context.Context, t *testing.T, data blobtesting.DataMap) *Manager {
+func newManagerForTesting(ctx context.Context, t *testing.T, data blobtesting.DataMap, options ManagerOptions) *Manager {
 	t.Helper()
 
 	bm := newContentManagerForTesting(ctx, t, data, contentManagerOpts{})
 
-	mm, err := NewManager(ctx, bm, ManagerOptions{}, nil)
+	mm, err := NewManager(ctx, bm, options, nil)
 	require.NoError(t, err)
 
 	return mm
@@ -354,7 +354,7 @@ func newManagerForTesting(ctx context.Context, t *testing.T, data blobtesting.Da
 func TestManifestInvalidPut(t *testing.T) {
 	ctx := testlogging.Context(t)
 	data := blobtesting.DataMap{}
-	mgr := newManagerForTesting(ctx, t, data)
+	mgr := newManagerForTesting(ctx, t, data, ManagerOptions{})
 
 	cases := []struct {
 		labels        map[string]string
@@ -377,7 +377,7 @@ func TestManifestAutoCompaction(t *testing.T) {
 	ctx := testlogging.Context(t)
 	data := blobtesting.DataMap{}
 
-	mgr := newManagerForTesting(ctx, t, data)
+	mgr := newManagerForTesting(ctx, t, data, ManagerOptions{})
 
 	for i := 0; i < 100; i++ {
 		item1 := map[string]int{"foo": 1, "bar": 2}
@@ -400,6 +400,66 @@ func TestManifestAutoCompaction(t *testing.T) {
 	}
 }
 
+func TestManifestConfigureAutoCompaction(t *testing.T) {
+	ctx := testlogging.Context(t)
+	data := blobtesting.DataMap{}
+	item1 := map[string]int{"foo": 1, "bar": 2}
+	labels1 := map[string]string{"type": "item", "color": "red"}
+	compactionCount := 99
+
+	mgr := newManagerForTesting(ctx, t, data, ManagerOptions{AutoCompactionThreshold: compactionCount})
+
+	for i := 0; i < compactionCount-1; i++ {
+		addAndVerify(ctx, t, mgr, labels1, item1)
+		require.NoError(t, mgr.Flush(ctx))
+		require.NoError(t, mgr.b.Flush(ctx))
+	}
+
+	// Should not trigger compaction
+	_, err := mgr.Find(ctx, labels1)
+	require.NoError(t, err)
+
+	foundContents := getManifestContentCount(ctx, t, mgr)
+
+	if got, want := foundContents, compactionCount-1; got != want {
+		t.Errorf("unexpected number of blocks: %v, want %v", got, want)
+	}
+
+	// Add another manifest
+	addAndVerify(ctx, t, mgr, labels1, item1)
+
+	require.NoError(t, mgr.Flush(ctx))
+	require.NoError(t, mgr.b.Flush(ctx))
+
+	// *Should* trigger compaction
+	_, err = mgr.Find(ctx, labels1)
+	require.NoError(t, err)
+
+	foundContents = getManifestContentCount(ctx, t, mgr)
+
+	if got, want := foundContents, 1; got != want {
+		t.Errorf("unexpected number of blocks: %v, want %v", got, want)
+	}
+}
+
+func getManifestContentCount(ctx context.Context, t *testing.T, mgr *Manager) int {
+	t.Helper()
+
+	foundContents := 0
+
+	if err := mgr.b.IterateContents(
+		ctx,
+		content.IterateOptions{Range: index.PrefixRange(ContentPrefix)},
+		func(ci content.Info) error {
+			foundContents++
+			return nil
+		}); err != nil {
+		t.Errorf("unable to list manifest content: %v", err)
+	}
+
+	return foundContents
+}
+
 func TestManifestAutoCompactionWithReadOnly(t *testing.T) {
 	ctx := testlogging.Context(t)
 	data := blobtesting.DataMap{}