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

Fix use of deprecated function grpc.WithTimeout() #3253

Merged
merged 11 commits into from
Apr 11, 2019

Conversation

codexnull
Copy link
Contributor

@codexnull codexnull commented Apr 3, 2019

Replace uses of grpc.WithTimeout() with context.WithTimeout(). Fix test error messages.


This change is Reviewable

@codexnull codexnull changed the title Show stderr from JSON load test Fix use of deprecated function grpc.WithTimeout() Apr 3, 2019
@codexnull codexnull requested a review from a team April 3, 2019 22:00
Copy link
Contributor

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

Reviewed 1 of 7 files at r2, 6 of 6 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @codexnull)


dgraph/cmd/zero/zero.go, line 563 at r3 (raw file):

	if x.IsReservedPredicate(tablet.Predicate) {
		// Force all the reserved predicates to be allocated to group 1.
		// This is to make it eaiser to stream ACL updates to all alpha servers

Why is this showing up here? This is one of my changes and it's already in master. Weird. Just merge from master again and check the diff is sane after that.


z/exec.go, line 32 at r3 (raw file):

	ShowOutput  bool = os.Getenv("DEBUG_SHOW_OUTPUT") != ""
	ShowError   bool = os.Getenv("DEBUG_SHOW_ERROR") != ""
	ShowCommand bool = os.Getenv("DEBUG_SHOW_COMMAND") != ""

are these variables used outside the z package? if not, keeping them as unexported is better.

Copy link

@gitlw gitlw 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, 2 unresolved discussions (waiting on @codexnull and @martinmr)


z/exec.go, line 32 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
	ShowOutput  bool = os.Getenv("DEBUG_SHOW_OUTPUT") != ""
	ShowError   bool = os.Getenv("DEBUG_SHOW_ERROR") != ""
	ShowCommand bool = os.Getenv("DEBUG_SHOW_COMMAND") != ""

are these variables used outside the z package? if not, keeping them as unexported is better.

agree with Martin

Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

:lgtm:

after making the variable private if they are not being used elsewhere.

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

Copy link
Contributor Author

@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, 2 unresolved discussions (waiting on @martinmr)


dgraph/cmd/zero/zero.go, line 563 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Why is this showing up here? This is one of my changes and it's already in master. Weird. Just merge from master again and check the diff is sane after that.

I guess because I merged master before. When I try to merge it again it just says, "Already up to date."


z/exec.go, line 32 at r3 (raw file):

Previously, gitlw (Lucas Wang) wrote…

agree with Martin

They're exported so the can be used outsize of the z package. For example so that a test running in package main can set z.ShowError directly instead of only through an environment variable.

Copy link
Contributor Author

@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: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @martinmr)


z/exec.go, line 32 at r3 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

They're exported so the can be used outsize of the z package. For example so that a test running in package main can set z.ShowError directly instead of only through an environment variable.

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.

Looks ok to me. Defer to @gitlw and @martinmr for final LGTM.

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @martinmr)

Copy link
Contributor

@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: 6 of 7 files reviewed, all discussions resolved (waiting on @martinmr)

Copy link
Contributor

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

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@codexnull codexnull merged commit a2fdf0a into master Apr 11, 2019
@codexnull codexnull deleted the javier/json_test_fixes branch April 24, 2019 18:10
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
Replace uses of grpc.WithTimeout() with context.WithTimeout(). Fix test error messages.
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