refactor: localstore transaction #4626
Conversation
istae
force-pushed
the
localstore-transaction
branch
2 times, most recently
from
db4ead4 to
c85ea95
Compare
There was a problem hiding this comment.
The changeset in this PR is quite large +3481 −5890 lines across 117 files. I am not able to validate all the changes for correctness as I would need more context on them. Some of the changes look to me not related to localstore transactions. I think that an interactive session on this PR would be more efficient than submitting comments. Esad, would you consider doing an interactive session?
| if err != nil { | ||
| logger.Error(err, "getting sleep value failed") | ||
| } | ||
| defer func() { time.Sleep(d) }() |
There was a problem hiding this comment.
What's the advantage of sleeping after the operation successfully completes?
There was a problem hiding this comment.
this is a infra requirement, to prevent the pod from restarting when it quits
There was a problem hiding this comment.
We should add a comment then.
There was a problem hiding this comment.
Nice approach on the transaction handling. It is an improvement, for sure.
The PR is rather large and it needs a lot of attention and with so many changes it is something harder to see if a change is related to the transaction or general code improvement. It would be good to separate code improvements from the new transaction into another PR, but that would require a lot of additional work. It is crucial to have do extensive correctness and load testing on integration.
| @@ -371,13 +371,10 @@ func (s *Service) retrieveChunk(ctx context.Context, quit chan struct{}, chunkAd | |||
|
|
|||
| func (s *Service) prepareCredit(ctx context.Context, peer, chunk swarm.Address, origin bool) (accounting.Action, error) { | |||
|
|
|||
| creditCtx, cancel := context.WithTimeout(ctx, time.Second) | |||
There was a problem hiding this comment.
it should be safe to wait as long as the context does not timeout, the one second is very arbitrary
Checklist
Description
An extensive cleanup of the db transaction.
Every localstore write operation now utilizes a transaction. Calls to the chunkstore are also part of the transaction.
also tackled: evict just enough chunks of a batch to fall below the reserve capacity
TODO:
revert reserve size change
revert reserve wake up duration
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
closes #4341 #4538 #4636 #4633
Screenshots (if appropriate):