PKI PR Feedback.

[winder]

Summary

• misc nits, clarifications, typos, organization
• Close() now blocks (with timeout) until write queue completes.
• Flush() now has a timeout.
• REST API returns 404 for no-op deletes and missing keys

Test Plan

[tsachiherman]

duration -> timeout

[tsachiherman]

Timeout -> flushTimeout ( it doesn't look like there is a reason to make it public, right ? )

[winder]

It's not used, but I was thinking it might make sense to have it be configurable. I don't mind making it private.

[tsachiherman]

the writing of db.writeQueue <- partDBWriteRecord which is done under mutex lock, could deadlock with the flush thread trying to take the lock (db.mutex.Lock()). In fact, if you'll try to Flush very quickly many time, it's very likely to happen.

[winder]

Great catch, I think there were a few opportunities for this to happen with other functions as well.

I reproduced one of the deadlocks with a unit test and added a separate mutex for the specific variable used by the flush thread.

[codecov-commenter]

Codecov Report

Merging #3191 (b924a84) into feature/partkey (0ca20bd) will decrease coverage by 0.01%.
The diff coverage is 45.78%.

@@                 Coverage Diff                 @@
##           feature/partkey    #3191      +/-   ##
===================================================
- Coverage            47.70%   47.68%   -0.02%     
===================================================
  Files                  367      367              
  Lines                59079    59082       +3     
===================================================
- Hits                 28184    28176       -8     
- Misses               27650    27663      +13     
+ Partials              3245     3243       -2     

Impacted Files	Coverage Δ
agreement/abstractions.go	50.00% <ø> (ø)
agreement/agreementtest/keyManager.go	100.00% <ø> (ø)
daemon/algod/api/server/v2/handlers.go	0.00% <0.00%> (ø)
daemon/algod/api/server/v2/test/helpers.go	74.81% <ø> (ø)
data/accountManager.go	0.00% <0.00%> (ø)
node/node.go	23.89% <20.00%> (ø)
data/account/participationRegistry.go	81.29% <59.32%> (-0.84%)	⬇️
agreement/pseudonode.go	72.15% <100.00%> (ø)
network/wsPeer.go	65.83% <0.00%> (-2.50%)	⬇️
network/requestTracker.go	70.25% <0.00%> (-0.87%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ca20bd...b924a84. Read the comment docs.

[tsachiherman]

I'd like to suggest a slightly different approach to the participationDB.Flush implementation that, I believe, would make it much simpler and more robust; the pseudocode goes like that:

func (db *participationDB) Flush(timeout time.Duration) error {
        resultChannel := make(chan error, 1)
        timeoutCh := time.After(timeout)
	select {
        case db.writeQueue <- partDBWriteRecord{
			flush: true,
                        writeError: resultChannel,
		}
	case <-db.writeQueue:
		return err
	case <-timeoutCh:
		return fmt.Errorf("timeout while flushing changes, check results manually")
	}

	select {
        case err := <- resultChannel:
               // todo : log error here if we want to.
               return err
	case <-db.writeQueue:
		return err
	case <-timeoutCh:
		return fmt.Errorf("timeout while flushing changes, check results manually")
	}
}

This model doesn't really require any Cond variable, and given that the output channel is allocated per Flush call, there is no risk of the channel being full.

[tsachiherman]

it would be nice to have the 500 * time.Millisecond as a constant, but we don't need this to be done in this PR.

[tsachiherman]

looks good. thanks for the changes.