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

Allow truncating raft logs via debug tool #3345

Merged
merged 4 commits into from
Apr 30, 2019
Merged

Conversation

manishrjain
Copy link
Contributor

@manishrjain manishrjain commented Apr 30, 2019

This PR would add a flag in debug tool, which would allow dropping data from Raft log entries of type EntryNormal.


This change is Reviewable

@manishrjain manishrjain requested a review from a team as a code owner April 30, 2019 19:44
@manishrjain manishrjain changed the title Allow truncating raft logs via debug tool. Allow truncating raft logs via debug tool Apr 30, 2019
flag.BoolVar(&opt.sizeHistogram, "histogram", false,
"Show a histogram of the key and value sizes.")

flag.StringVarP(&opt.wdir, "wal", "w", "", "Directory where Raft write-ahead logs are stored.")
flag.Uint64VarP(&opt.wtruncateUntil, "truncate", "t", 0, "Remove data from Raft entries up until this index.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line is 111 characters (from lll)

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain)

Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @golangcibot and @manishrjain)


dgraph/cmd/debug/run.go, line 92 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 111 characters (from lll)

I feel like "truncate" is the wrong term here. Truncate means to cut off the end (e.g. truncate a file), and I think here you're removing things from the start. Possible alternatives: drop, prune, expire,


dgraph/cmd/debug/run.go, line 93 at r2 (raw file):

	flag.StringVarP(&opt.wdir, "wal", "w", "", "Directory where Raft write-ahead logs are stored.")
	flag.Uint64VarP(&opt.wtruncateUntil, "truncate", "t", 0,
		"Remove data from Raft entries up until this index.")

"until" is a ambiguous about whether the index itself is removed or not. I'd suggest changing it to "until and including" or "until but not including" to be clear.


dgraph/cmd/debug/run.go, line 855 at r2 (raw file):

				case ent.Type == raftpb.EntryNormal && ent.Index < opt.wtruncateUntil:
					if len(ent.Data) == 0 {
						continue

Is there a reason for not removing empty entries?

Copy link
Contributor Author

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @codexnull and @golangcibot)


dgraph/cmd/debug/run.go, line 93 at r2 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

"until" is a ambiguous about whether the index itself is removed or not. I'd suggest changing it to "until and including" or "until but not including" to be clear.

Done.


dgraph/cmd/debug/run.go, line 855 at r2 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

Is there a reason for not removing empty entries?

Entries aren't being removed. Only their data is being dropped.

@manishrjain manishrjain merged commit 3fd5836 into master Apr 30, 2019
@manishrjain manishrjain deleted the mrjn/truncate-raft branch April 30, 2019 21:22
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
This PR would add a flag in debug tool, which would allow dropping data from Raft log entries of type EntryNormal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants