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

chore: Update Raft migrate tool to handle new proposal format #7123

Merged
merged 19 commits into from
Dec 16, 2020

Conversation

rahulgurnani
Copy link
Contributor

@rahulgurnani rahulgurnani commented Dec 11, 2020

Fixes DGRAPH-2832
This PR updates the raft wal migrate tool. We recently updated raft proposal format for alpha and zero, see #7059 .
The tool now can migrate raftwal from old proposal format to new proposal format.
This change will help to migrate raftwal from RC-2 to RC-4 onwards.


This change is Reviewable

rahulgurnani added 4 commits December 11, 2020 18:38
Earlier we had Key in message Proposal, now it's present outside the Proposal object as first 8 bytes
Earlier we used to store raftwal in badger, so can be deprecated
@all-seeing-code
Copy link
Contributor

This is to unable migrating from 20.11-rc3 to 20.11.-rc4 without backup restore.

This change is Reviewable

This is to enable migration or disable migration without backup restore? The comment is unclear without context.

Copy link
Contributor Author

@rahulgurnani rahulgurnani left a comment

Choose a reason for hiding this comment

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

Updated the comment, hope it makes sense now.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jarifibrahim, @manishrjain, @NamanJain8, and @vvbalaji-dgraph)

Copy link
Contributor

@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: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @NamanJain8, @rahulgurnani, and @vvbalaji-dgraph)


dgraph/cmd/raft-migrate/generated.go, line 31 at r3 (raw file):

)

type Proposal struct {

Don't need this file. Protobufs are backwards compatible.


dgraph/cmd/raft-migrate/run.go, line 82 at r3 (raw file):

	newKey := parseAndConvertKey("%02d-%d", oldProposal.Key)
	var newProposal pb.Proposal
	newProposal.Mutations = oldProposal.Mutations

Don't need this either. Protobufs are compatible.

Copy link
Contributor

@NamanJain8 NamanJain8 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: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @NamanJain8, and @vvbalaji-dgraph)


dgraph/cmd/raft-migrate/generated.go, line 31 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't need this file. Protobufs are backwards compatible.

We don't have Key in the new Proposal. Hence, we need the old types(and their unmarshaller) to unmarshal into.


dgraph/cmd/raft-migrate/run.go, line 82 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't need this either. Protobufs are compatible.

We need all this because of Key.

Copy link
Contributor

@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: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @NamanJain8, @rahulgurnani, and @vvbalaji-dgraph)


dgraph/cmd/debug/wal.go, line 51 at r4 (raw file):

	var err error
	if isZero {
		var zpr pb.ZeroProposal

The previous code was fine. No need to change this, or add isZero.


dgraph/cmd/debug/wal.go, line 125 at r4 (raw file):

}

func printRaft(db *badger.DB, store *raftmigrate.OldDiskStorage, groupId uint32) {

No need to change.


dgraph/cmd/raft-migrate/generated.go, line 31 at r3 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

We don't have Key in the new Proposal. Hence, we need the old types(and their unmarshaller) to unmarshal into.

But, that id is reserved still. It should work. Did you try?

Copy link
Contributor

@ahsanbarkati ahsanbarkati 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: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @NamanJain8, @rahulgurnani, and @vvbalaji-dgraph)


dgraph/cmd/raft-migrate/run.go, line 162 at r4 (raw file):

	fmt.Printf("Fetching entries from low: %d to high: %d\n", firstIndex, lastIndex)
	// Should we batch this up?
	oldEntries, err := oldWal.Entries(1, lastIndex+1, math.MaxUint64)

should be oldWal.Entries(firstIndex, lastIndex+1, math.MaxUint64)


dgraph/cmd/raft-migrate/run.go, line 196 at r4 (raw file):

	newWal, err := raftwal.InitEncrypted(newDir, encKey)
	x.Check(err)

defer newWal.Close()

Copy link
Contributor

@NamanJain8 NamanJain8 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: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @ahsanbarkati, @jarifibrahim, @manishrjain, @rahulgurnani, and @vvbalaji-dgraph)


dgraph/cmd/debug/wal.go, line 51 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

The previous code was fine. No need to change this, or add isZero.

We noticed that the previous code was not printing out the correct entries for ZeroProposal was able to get unmarshalled into Proposal. Hence, always the first statement was getting executed.


dgraph/cmd/raft-migrate/generated.go, line 31 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

But, that id is reserved still. It should work. Did you try?

Yes, I tried retrieving Key from the data. But I couldn't find a way unless we write our own unmarhsaller.


dgraph/cmd/raft-migrate/run.go, line 162 at r4 (raw file):

Previously, ahsanbarkati (Ahsan Barkati) wrote…

should be oldWal.Entries(firstIndex, lastIndex+1, math.MaxUint64)

Done.


dgraph/cmd/raft-migrate/run.go, line 196 at r4 (raw file):

Previously, ahsanbarkati (Ahsan Barkati) wrote…

defer newWal.Close()

Done.

Copy link
Contributor

@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: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @ahsanbarkati, @jarifibrahim, @manishrjain, @NamanJain8, @rahulgurnani, and @vvbalaji-dgraph)


dgraph/cmd/raft-migrate/generated.go, line 31 at r3 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

Yes, I tried retrieving Key from the data. But I couldn't find a way unless we write our own unmarhsaller.

You don't need to retrieve the key from the data. It's not that important. Does something not work without it?

Copy link
Contributor

@NamanJain8 NamanJain8 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: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @ahsanbarkati, @jarifibrahim, @manishrjain, @rahulgurnani, and @vvbalaji-dgraph)


dgraph/cmd/raft-migrate/generated.go, line 31 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

You don't need to retrieve the key from the data. It's not that important. Does something not work without it?

If we don't have a key, then every proposal in raftwal would be considered as invalid. That would mean that we are starting from a fresh state and ignoring the current raftwal

if key == 0 {
return key, errInvalidProposal
}

Copy link
Contributor

@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.

Do we still have the Badger code? We should remove it.

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @ahsanbarkati, @manishrjain, @NamanJain8, @rahulgurnani, and @vvbalaji-dgraph)


dgraph/cmd/raft-migrate/generated.go, line 31 at r3 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

If we don't have a key, then every proposal in raftwal would be considered as invalid. That would mean that we are starting from a fresh state and ignoring the current raftwal

if key == 0 {
return key, errInvalidProposal
}

You can remove that code from Zero.

Copy link
Contributor

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

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

Yes, the code in raft-migrate/storage.go should be removed. This code is currently used by dgraph debug tool for debugging old badger wal. @ajeetdsouza is working on it. This code removal can be part of separate PR.

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @ahsanbarkati, @manishrjain, @NamanJain8, @rahulgurnani, and @vvbalaji-dgraph)

@NamanJain8 NamanJain8 merged commit ab0f3ea into master Dec 16, 2020
@NamanJain8 NamanJain8 deleted the rahulgurnani/raft-migrate branch December 16, 2020 18:48
NamanJain8 pushed a commit that referenced this pull request Dec 17, 2020
This PR updates the raftmigrate tool. We recently updated raft proposal format for alpha and zero, see #7059 .
The tool now can migrate raftwal from old proposal format to new proposal format wherein we are encoding the proposal.Key within the entry.Data.
This change will help to migrate raftwal from [RC-2,RC-3] to RC-4 onwards.

(cherry picked from commit ab0f3ea)
NamanJain8 added a commit that referenced this pull request Dec 17, 2020
chore: Update Raft migrate tool to handle new proposal format (#7123)

This PR updates the raftmigrate tool. We recently updated raft proposal format for alpha and zero, see #7059 .
The tool now can migrate raftwal from old proposal format to new proposal format wherein we are encoding the proposal.Key within the entry.Data.
This change will help to migrate raftwal from [RC-2,RC-3] to RC-4 onwards.

(cherry picked from commit ab0f3ea)

Co-authored-by: Rahul Gurnani <rahulgurnani09@gmail.com>
@NamanJain8 NamanJain8 restored the rahulgurnani/raft-migrate branch December 24, 2020 08:15
@NamanJain8 NamanJain8 deleted the rahulgurnani/raft-migrate branch December 24, 2020 11:41
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.

None yet

6 participants