Add syncMu to AlgorandFullNode OnNewBlock #1623
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsachiherman
suggested changes
Oct 14, 2020
node/node.go
Outdated
| @@ -578,7 +581,9 @@ func (node *AlgorandFullNode) GetPendingTransaction(txID transactions.Txid) (res | |||
| // Status returns a StatusReport structure reporting our status as Active and with our ledger's LastRound | |||
| func (node *AlgorandFullNode) Status() (s StatusReport, err error) { | |||
| node.mu.Lock() | |||
There was a problem hiding this comment.
It would be sufficient to take the new lock during the setup of these two output variables, i.e.
node.syncMu.Lock()
s.LastRoundTimestamp = node.lastRoundTimestamp
s.HasSyncedSinceStartup = node.hasSyncedSinceStartup
node.mu.Unlock()
node.mu.Lock()
defer node.syncMu.Unlock()
...
node/node.go
Outdated
Comment on lines
118
to
122
| // use syncMu for locking lastRoundTimestamp and hasSyncedSinceStartup | ||
| // syncMu added so OnNewBlock wouldn't be blocked by oldKeyDeletionThread during catchup | ||
| syncMu deadlock.Mutex | ||
| lastRoundTimestamp time.Time | ||
| hasSyncedSinceStartup bool |
There was a problem hiding this comment.
When writing a variable description in go, try to always start with the variable name; go-vet would complain about that for public functions.
node/node.go
Outdated
| @@ -115,6 +115,9 @@ type AlgorandFullNode struct { | |||
|
|
|||
| log logging.Logger | |||
|
|
|||
| // use syncMu for locking lastRoundTimestamp and hasSyncedSinceStartup | |||
| // syncMu added so OnNewBlock wouldn't be blocked by oldKeyDeletionThread during catchup | |||
| syncMu deadlock.Mutex | |||
There was a problem hiding this comment.
could you change the variable name to something more concrete. ( since all mutexes are meant for syncing.. )
maybe lastRoundTimeMu ?
There was a problem hiding this comment.
the two variables together might be 'sync status', so this could be the 'sync status mutex'
tsachiherman
approved these changes
Oct 14, 2020
tsachiherman
pushed a commit
to tsachiherman/go-algorand
that referenced
this pull request
Jul 7, 2021
separate synchronization primitive for updating the latest round reached timestamp to improve catchup performance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Adding a new mutex to AlgorandFullNode so that OnNewBlock wouldn't be blocked by oldKeyDeletionThread during catchup.
Test Plan