-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Check for n.Raft() to be nil #4084
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ A review job has been created and sent to the PullRequest network.
@mangalaman93 you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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 r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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 @balajijinnah and @mangalaman93)
worker/snapshot.go, line 168 at r1 (raw file):
} raftNode := n.Raft()
I like to generally avoid one-time use variables. A good way to do this would be to put an OR next to n == nil.
if n == nil || n.Raft() == nil { ... }
There was a problem hiding this 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 @balajijinnah and @manishrjain)
worker/snapshot.go, line 168 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
I like to generally avoid one-time use variables. A good way to do this would be to put an OR next to n == nil.
if n == nil || n.Raft() == nil { ... }
I was trying to avoid calling n.Raft()
twice because it internally acquires a lock. Does it still seem okay to do that?
There was a problem hiding this 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 @balajijinnah and @mangalaman93)
worker/snapshot.go, line 168 at r1 (raw file):
Previously, mangalaman93 (Aman Mangal) wrote…
I was trying to avoid calling
n.Raft()
twice because it internally acquires a lock. Does it still seem okay to do that?
Really not a big deal. This func is super expensive already.
09e4f1e
to
a0793b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Warning
PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More
fixes #4053
This change is