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.

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

storage: suggest compaction when replicas are deleted #21178

Conversation

spencerkimball
Copy link
Member

In aggregate, if enough contiguous replicas are migrated away from a
range, we would like to compact the underlying RocksDB storage engine.
To that end, on clear a replica's data, we suggest a compaction.

Release note: None

@spencerkimball spencerkimball requested review from tbg and a team January 2, 2018 23:07
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member

tbg commented Jan 3, 2018

:lgtm:


Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/storage/client_raft_test.go, line 3898 at r1 (raw file):

	testutils.SucceedsSoon(t, func() error {
		// Check for a compaction metric indicating non-zero bytes are queued.
		if c := mtc.stores[2].Compactor().Metrics.BytesQueued.Value(); c == 0 {

The compaction could also be processed (maybe not in today's code, but in principle), so check there's something queued or processed or skipped.


pkg/storage/client_raft_test.go, line 3904 at r1 (raw file):

		// other stores.
		for i := 0; i < 2; i++ {
			if c := mtc.stores[i].Compactor().Metrics.BytesQueued.Value(); c != 0 {

Ditto.


Comments from Reviewable

@spencerkimball
Copy link
Member Author

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions.


pkg/storage/client_raft_test.go, line 3898 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

The compaction could also be processed (maybe not in today's code, but in principle), so check there's something queued or processed or skipped.

Done.


pkg/storage/client_raft_test.go, line 3904 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Ditto.

Done.


Comments from Reviewable

@tbg
Copy link
Member

tbg commented Jan 3, 2018

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@spencerkimball
Copy link
Member Author

@tschottdorf note that in the process of testing this with an actual migration, I noticed some other problems, which I've updated this commit to fix. PTAL.

@bdarnell
Copy link
Contributor

bdarnell commented Jan 3, 2018

:lgtm:


Review status: 3 of 5 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@tbg
Copy link
Member

tbg commented Jan 3, 2018

:lgtm:


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

In aggregate, if enough contiguous replicas are migrated away from a
range, we would like to compact the underlying RocksDB storage engine.
To that end, on clear a replica's data, we suggest a compaction.

Fixed various bugs with the compactor queue - in particular, the call
in the main processing loop to stop the timer was misplaced and caused
the timer to be permanently disabled. Also, fixed an indexing error
in log messages and updated comments.

Release note: None
@spencerkimball spencerkimball merged commit 17ae6e7 into cockroachdb:master Jan 4, 2018
@spencerkimball spencerkimball deleted the migration-compaction-suggestion branch January 4, 2018 13:17
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.

4 participants