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

engineccl: only emit tombstones in incremental export #16690

Merged
merged 1 commit into from Jun 26, 2017

Conversation

dt
Copy link
Member

@dt dt commented Jun 22, 2017

while the import correctly handles them either way, they are just dead weight in non-incrmental export.
eliding them should make backups smaller and faster.

Semantics for row counts on incremental backups are now number of rows changed, which makes sense.

@dt dt requested a review from danhhz June 22, 2017 14:41
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@danhhz
Copy link
Contributor

danhhz commented Jun 26, 2017

:lgtm:

Wanna put "Closes #16652" in the commit message?


Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/ccl/storageccl/engineccl/mvcc.go, line 165 at r1 (raw file):

		}

		if (i.startTime == hlc.Timestamp{}) && len(i.iter.UnsafeValue()) == 0 {

This wants a comment.


Comments from Reviewable

While the import correctly handles them either way, they are just dead weight in non-incrmental export.
eliding them should make backups smaller and faster.

Semantics for row counts on incremental backups are now number of rows changed, which makes sense.

Fixes cockroachdb#16652.
@dt
Copy link
Member Author

dt commented Jun 26, 2017

Done.


Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.


pkg/ccl/storageccl/engineccl/mvcc.go, line 165 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

This wants a comment.

Done.


Comments from Reviewable

@dt dt merged commit 35b2f22 into cockroachdb:master Jun 26, 2017
@dt dt deleted the tombstones branch August 15, 2017 18:50
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.

None yet

3 participants