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

Force users to explicitly tell restore command to run without zero. #5206

Merged
merged 3 commits into from
Apr 14, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Apr 14, 2020

The zero value passed to the command is optional, which can lead to
users forgetting about it and being surprised that the cluster does
not work (because the uid lease and the timestamp need to be updated
first).

This change makes the zero flag required unless the --force_zero
flag is explicitly passed. Not passing a zero value can still be useful
when only the p directories need to be generated.

Fixes DGRAPH-1251


This change is Reviewable

Docs Preview: Dgraph Preview

The zero value passed to the command is optional, which can lead to
users forgetting about it and being surprised that the cluster does
not work (because the uid lease and the timestamp need to be updated
first).

This change makes the option not optional, unless the --force_no_zero
flag is explicitly passed. Not passing a zero value can still be useful
when only the p directories need to be generated.
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.

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @danielmai, @manishrjain, @martinmr, and @MichaelJCompton)


ee/backup/run.go, line 111 at r1 (raw file):

force_no_zero

We should call this force_zero and then make the default true.

--force_zero=false would relax the requirement to connect to Zero.


ee/backup/run.go, line 178 at r1 (raw file):

		return errors.Errorf("No zero address passed. Use the --force-no-zero option if you " +
			"meant to do this")
	}

"No Dgraph Zero address passed. Use the --force_zero option if you meant to do this"


wiki/content/enterprise-features/index.md, line 181 at r1 (raw file):

optional

We shouldn't say it's optional anymore.

We should say that the --zero option is required unless the other flag is set (--force_zero=false).

Copy link
Contributor Author

@martinmr martinmr 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 @danielmai, @manishrjain, and @MichaelJCompton)


ee/backup/run.go, line 111 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…
force_no_zero

We should call this force_zero and then make the default true.

--force_zero=false would relax the requirement to connect to Zero.

Done.


ee/backup/run.go, line 178 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…
		return errors.Errorf("No zero address passed. Use the --force-no-zero option if you " +
			"meant to do this")
	}

"No Dgraph Zero address passed. Use the --force_zero option if you meant to do this"

Done.


wiki/content/enterprise-features/index.md, line 181 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…
optional

We shouldn't say it's optional anymore.

We should say that the --zero option is required unless the other flag is set (--force_zero=false).

Done.

@martinmr martinmr requested a review from danielmai April 14, 2020 21:21
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 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai, @manishrjain, and @MichaelJCompton)

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @manishrjain and @MichaelJCompton)

@martinmr martinmr merged commit ef2adf6 into master Apr 14, 2020
@martinmr martinmr deleted the martinmr/no-zero-restore branch April 14, 2020 23:24
martinmr added a commit that referenced this pull request Apr 14, 2020
…5206)

The zero value passed to the command is optional, which can lead to
users forgetting about it and being surprised that the cluster does
not work (because the uid lease and the timestamp need to be updated
first).

This change makes the option not optional, unless the --force_zero
flag is set to false . Not passing a zero value can still be useful when only
the p directories need to be generated.
martinmr added a commit that referenced this pull request Apr 14, 2020
…5206)

The zero value passed to the command is optional, which can lead to
users forgetting about it and being surprised that the cluster does
not work (because the uid lease and the timestamp need to be updated
first).

This change makes the option not optional, unless the --force_zero
flag is set to false . Not passing a zero value can still be useful when only
the p directories need to be generated.
@martinmr
Copy link
Contributor Author

Cherry-picked to the 1.2 and 20.03 branches.

dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
…graph-io#5206)

The zero value passed to the command is optional, which can lead to
users forgetting about it and being surprised that the cluster does
not work (because the uid lease and the timestamp need to be updated
first).

This change makes the option not optional, unless the --force_zero
flag is set to false . Not passing a zero value can still be useful when only
the p directories need to be generated.
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.

2 participants