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

minor improvments in reactor code #1020

Merged
merged 3 commits into from
Apr 23, 2019
Merged

Conversation

ParthDesai
Copy link
Contributor

@ParthDesai ParthDesai commented Apr 22, 2019

  • I added unit tests for any code that added
  • I updated the CHANGELOG.md
  • All IP is original and not copied from another source
  • I assign all copyright to Loom Network for the code in the pull request

Copy link
Contributor

@enlight enlight left a comment

Choose a reason for hiding this comment

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

Just needs a small spelling fix

@@ -477,7 +472,7 @@ func (f *FnConsensusReactor) commit(fnID string) {
}

if !currentVoteSet.HasConverged(f.cfg.FnVoteSigningThreshold, currentValidators) {
f.Logger.Info("No consensus achived", "VoteSet", currentVoteSet)
f.Logger.Info("No consensus achived", "fnID", fnID, "VoteSet", currentVoteSet, "Payload", currentVoteSet.Payload, "Response", currentVoteSet.Payload.Response)
Copy link
Contributor

Choose a reason for hiding this comment

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

achived -> achieved

@@ -402,6 +402,7 @@ func (f *FnConsensusReactor) vote(fnID string, fn Fn, currentValidators *types.V
votesetPayload := NewFnVotePayload(executionRequest, executionResponse)

f.stateMtx.Lock()
defer f.stateMtx.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine. But what about the other mutexes you "manually" unlock like peerMapMtx, if there's a panic while trying to send to a peer it's never going to get unlocked.

Copy link
Contributor

@enlight enlight left a comment

Choose a reason for hiding this comment

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

LGTM

@ParthDesai ParthDesai merged commit ec9f8f1 into master Apr 23, 2019
@ParthDesai ParthDesai deleted the fix/reactor-improvments branch April 23, 2019 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants