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

dgraphzero: lease flag #1583

Merged
merged 3 commits into from
Oct 4, 2017
Merged

dgraphzero: lease flag #1583

merged 3 commits into from
Oct 4, 2017

Conversation

peterstace
Copy link

@peterstace peterstace commented Oct 4, 2017

This change is Reviewable

@manishrjain
Copy link
Contributor

:lgtm: Some comments.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


cmd/dgraphzero/raft.go, line 243 at r1 (raw file):

		state.MaxLeaseId = p.MaxLeaseId
	}
	if p.MaxLeaseId == 0 {

if p.MaxLeaseId > 0 and < state.MaxLeaseId, we should return an error? Or, at lease log it.


cmd/dgraphzero/raft.go, line 245 at r1 (raw file):

	if p.MaxLeaseId == 0 {
		// Don't show lease proposals - they occur too frequently to be useful.
		x.Printf("Applied proposal: %+v\n", p)

Not sure why this printf is in the else. or in an if now. We can just print it directly, right?


Comments from Reviewable

@peterstace
Copy link
Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


cmd/dgraphzero/raft.go, line 243 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if p.MaxLeaseId > 0 and < state.MaxLeaseId, we should return an error? Or, at lease log it.

Done. Logging it out.


cmd/dgraphzero/raft.go, line 245 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Not sure why this printf is in the else. or in an if now. We can just print it directly, right?

@janardhan1993 mentioned to me it was occurring too frequently to be useful output. I'll defer to his judgement here.


Comments from Reviewable

@janardhan1993
Copy link
Contributor

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


cmd/dgraphzero/raft.go, line 245 at r1 (raw file):

Previously, peterstace (Peter Stace) wrote…

@janardhan1993 mentioned to me it was occurring too frequently to be useful output. I'll defer to his judgement here.

Lease changes happen too frequently i couldn't see tablet size update proposals in middle of them.
Probably better not to print lease proposals. Or may be print only the lease id for proposals which update lease.


cmd/dgraphzero/raft.go, line 242 at r2 (raw file):

	if p.MaxLeaseId > state.MaxLeaseId {
		state.MaxLeaseId = p.MaxLeaseId
	} else {

else if p.maxleaseid != 0
This proposal is used for other purposes also- for membership information and tablet sizes.


Comments from Reviewable

@peterstace
Copy link
Author

Review status: all files reviewed at latest revision, 3 unresolved discussions.


cmd/dgraphzero/raft.go, line 245 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Lease changes happen too frequently i couldn't see tablet size update proposals in middle of them.
Probably better not to print lease proposals. Or may be print only the lease id for proposals which update lease.

Hmm, okay I'll leave as is for now.


Comments from Reviewable

@peterstace peterstace merged commit 5468f77 into master Oct 4, 2017
@pawanrawal pawanrawal deleted the dgraphzero_lease_flag branch December 19, 2017 08:34
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.

3 participants