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

libroach: automatically compact sstables with any range deletions #32385

Merged
merged 4 commits into from Nov 16, 2018

Conversation

Projects
None yet
3 participants
@petermattis
Copy link
Contributor

petermattis commented Nov 15, 2018

Add a table property collector which marks any sstable containing a
range tombstone as requiring compaction. This ensures that the disk
space for deleted data is reclaimed promptly.

Change RocksDBMap.Clear to force a flush after adding a range
tombstone. Combined with the compaction of any sstable containing a
range tombstone, this ensures that space in the temporary storage
directory is reclaimed quickly.

Release note (bug fix): Ensure that space in the temporary storage
directory is reclaimed more promptly.

@petermattis petermattis requested a review from cockroachdb/core-prs as a code owner Nov 15, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 15, 2018

This change is Reviewable

@petermattis petermattis requested review from benesch and tbg Nov 15, 2018

@petermattis

This comment has been minimized.

Copy link
Contributor

petermattis commented Nov 15, 2018

This is based on #31975, but without any attempt to remove the compaction queue. I'd like to add a test before this merges.

@petermattis petermattis force-pushed the petermattis:pmattis/delete-range-prop-collector2 branch from 8e36105 to d6aeefc Nov 15, 2018

@benesch
Copy link
Collaborator

benesch left a comment

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


c-deps/libroach/include/libroach.h, line 136 at r1 (raw file):

// Disable/enable automatic compactions. Automatic compactions are
// enabled by default. Disabling is provided for testing purposes so
// that automatic compactions do not interfere with test expecations.

"expecations"


pkg/storage/engine/disk_map.go, line 188 at r1 (raw file):

	// range tombstone is pushed to disk which will kick off compactions that
	// will eventually free up the deleted space.
	return r.store.Flush()

Why in RocksDBMap.Clear but not elsewhere?

@benesch
Copy link
Collaborator

benesch left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


c-deps/libroach/options.cc, line 198 at r1 (raw file):

  // deletions.
  options.table_properties_collector_factories.emplace_back(
      new DeleteRangeTblPropCollectorFactory());

nitty nit: this should be consistent with the time bound property collector installation. Maybe these table properties collectors should live in a "tableprops.h" header? But I think giving the time bound one its own file while the delete range one gets stuffed into options.cc is sotra weird.

petermattis added some commits Jun 15, 2018

libroach: automatically compact sstables with any range deletions
Add a table property collector which marks any sstable containing a
range tombstone as requiring compaction. This ensures that the disk
space for deleted data is reclaimed promptly.

Change `RocksDBMap.Clear` to force a flush after adding a range
tombstone. Combined with the compaction of any sstable containing a
range tombstone, this ensures that space in the temporary storage
directory is reclaimed quickly.

Release note (bug fix): Ensure that space in the temporary storage
directory is reclaimed more promptly.

@petermattis petermattis force-pushed the petermattis:pmattis/delete-range-prop-collector2 branch from d6aeefc to e740d2c Nov 15, 2018

@petermattis
Copy link
Contributor

petermattis left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


c-deps/libroach/options.cc, line 198 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

nitty nit: this should be consistent with the time bound property collector installation. Maybe these table properties collectors should live in a "tableprops.h" header? But I think giving the time bound one its own file while the delete range one gets stuffed into options.cc is sotra weird.

I've moved timebound.{cc,h} to table_props.{cc,h} and moved the delete range property collector there. PTAL.


c-deps/libroach/include/libroach.h, line 136 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

"expecations"

Done.


pkg/storage/engine/disk_map.go, line 188 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Why in RocksDBMap.Clear but not elsewhere?

Where else are you thinking? I'm intentionally punting on trying to get rid of the compaction queue in this PR.

@benesch
Copy link
Collaborator

benesch left a comment

:lgtm:

Are you planning to add a test when you remove the compaction queue?

Reviewed 1 of 1 files at r2, 2 of 5 files at r3, 3 of 3 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


c-deps/libroach/db.cc, line 741 at r4 (raw file):

  // by the time bounded iterator optimization.
  options->table_properties_collector_factories.emplace_back(DBMakeTimeBoundCollector());
  // TODO(peter): DeleteRangeCollector

Ping.


pkg/storage/engine/disk_map.go, line 188 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Where else are you thinking? I'm intentionally punting on trying to get rid of the compaction queue in this PR.

Oh, I see, the idea is that everywhere else the compaction queue will trigger the flush? Makes sense to me.

@petermattis petermattis force-pushed the petermattis:pmattis/delete-range-prop-collector2 branch from e740d2c to 8bf0e0d Nov 15, 2018

@petermattis
Copy link
Contributor

petermattis left a comment

Are you planning to add a test when you remove the compaction queue?

I'm still planning to add a test before merging. Shouldn't take long as soon as I have a free hour.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


c-deps/libroach/db.cc, line 741 at r4 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Ping.

Heh, that's why I added that TODO. Fixed.


pkg/storage/engine/disk_map.go, line 188 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Oh, I see, the idea is that everywhere else the compaction queue will trigger the flush? Makes sense to me.

Yes, the compaction queue will trigger the flush elsewhere. And it will do a compaction as well which will be less necessary, but oh well.

@benesch
Copy link
Collaborator

benesch left a comment

Reviewed 1 of 1 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@petermattis petermattis force-pushed the petermattis:pmattis/delete-range-prop-collector2 branch from 8bf0e0d to d4361ce Nov 15, 2018

@petermattis

This comment has been minimized.

Copy link
Contributor

petermattis commented Nov 15, 2018

@benesch Added the promised test. It fails without the delete-range prop collector, or if I disable the flush in RocksDBMap.Clear. PTAL.

storage/engine: add TestRocksDBMapClose
Test that a RocksDBMap will free up deleted data when closed.

Release note: None

@petermattis petermattis force-pushed the petermattis:pmattis/delete-range-prop-collector2 branch from c0fce34 to 7428d86 Nov 16, 2018

@petermattis

This comment has been minimized.

Copy link
Contributor

petermattis commented Nov 16, 2018

@benesch Can you take a look at the new test? Or rubber stamp it?

@benesch
Copy link
Collaborator

benesch left a comment

:lgtm:

Reviewed 1 of 1 files at r6, 1 of 1 files at r7.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@petermattis

This comment has been minimized.

Copy link
Contributor

petermattis commented Nov 16, 2018

bors r=benesch

@petermattis

This comment has been minimized.

Copy link
Contributor

petermattis commented Nov 16, 2018

Hello, bors. Are you there?

bors r=benesch

craig bot pushed a commit that referenced this pull request Nov 16, 2018

Merge #32385
32385: libroach: automatically compact sstables with any range deletions r=benesch a=petermattis

Add a table property collector which marks any sstable containing a
range tombstone as requiring compaction. This ensures that the disk
space for deleted data is reclaimed promptly.

Change `RocksDBMap.Clear` to force a flush after adding a range
tombstone. Combined with the compaction of any sstable containing a
range tombstone, this ensures that space in the temporary storage
directory is reclaimed quickly.

Release note (bug fix): Ensure that space in the temporary storage
directory is reclaimed more promptly.

Co-authored-by: Peter Mattis <petermattis@gmail.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 16, 2018

Build succeeded

@craig craig bot merged commit 7428d86 into cockroachdb:master Nov 16, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@petermattis petermattis deleted the petermattis:pmattis/delete-range-prop-collector2 branch Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment