Add TxWithRetries and test cases #109
Conversation
| return cmdErr.Code == 112 | ||
| } | ||
|
|
||
| log.Println("Error does not conform to CommandError") |
There was a problem hiding this comment.
we using "github.com/rs/zerolog/log" in the service. i'm not sure if using log package directly would be a good idea.
There was a problem hiding this comment.
just adding on top of this. i don't think we actually need any logs in this file. it's the calling methods responsibility to log errors.
That means u can simply remove all logs in this file
There was a problem hiding this comment.
all the methods here looks very similar, they all try to check the cmdErr code.
shall we have a private generic method that does that and compare the code instead? then this generic method can be called by IsWriteConflictError etc by passing in the 122 code into the method
| dbTransactionClient DBTransactionClient, | ||
| txnFunc func(sessCtx mongo.SessionContext) (interface{}, error), | ||
| ) (interface{}, error) { | ||
| maxAttempts := DefaultMaxAttempts |
There was a problem hiding this comment.
maxAttempts is not overwritten anywhere in this method, why not use the pre-defined variable directly in this case?
same as initialBackoff and backoffFactor
There was a problem hiding this comment.
yes I can remove that
| @@ -54,3 +56,10 @@ type DBClient interface { | |||
| ) error | |||
| FindTopStakersByTvl(ctx context.Context, paginationToken string) (*DbResultMap[model.StakerStatsDocument], error) | |||
| } | |||
| type DBSession interface { | |||
There was a problem hiding this comment.
Could you remind me why we need to define this here?
I'm thinking twice around this, feel like we shall not expose internal implementation of the db client here as the interface here are used for defining API contract, it's not mongodb specific.
There was a problem hiding this comment.
was trying to make it an interface so that the function could consume both test db and mongo db
There was a problem hiding this comment.
I think in go, unit tests are placed under the same file directory as the methods you are trying to test with. this would avoid u making those internal implementation methods being public
There was a problem hiding this comment.
ok agree
There was a problem hiding this comment.
question: why do we need this implementation instead of using the time.sleep method directly?
There was a problem hiding this comment.
im abstracted the sleep func to mainly use it in tests for mock sleep
| backoffFactor := DefaultBackoffFactor | ||
|
|
||
| var ( | ||
| result interface{} |
There was a problem hiding this comment.
I think there might be too many tabs here
| for attempt := 1; attempt <= maxAttempts; attempt++ { | ||
| session, sessionErr := dbTransactionClient.StartSession(); | ||
|
|
||
|
|
| if mongo.IsNetworkError(err) { | ||
| return true | ||
| } | ||
| if mongo.IsTimeout(err) { |
There was a problem hiding this comment.
i think the network error and timeout error from mongo is already retry handled by the mongo driver implementation of WithTransaction
#63
Features:
Tests: