feat: [OCISDEV-855] receivedsharecache: retry on transient storage errors in CAS loop#657
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
1a0308f to
4fe0540
Compare
a6710ac to
58e4274
Compare
mklos-kw
changed the title
jvillafanez
left a comment
jvillafanez
left a comment
There was a problem hiding this comment.
Some minor comments and ideas, but it looks fine.
| ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Grab lock") | ||
| unlock := c.lockUser(userID) | ||
| span.End() | ||
| span.SetAttributes(attribute.String("cs3.userid", userID)) |
There was a problem hiding this comment.
Since this has been touched....
I think we decided against this pattern. Either you time the whole List function, or you time inside the lockUser function (which will also time all the calls going through that function). In this particular you case, you're only measuring this particular lockUser call, which might be confusing because other calls won't appear.
In addition, the SetAttributes might not have any effect because the span has ended.
| span.SetAttributes(attribute.String("cs3.userid", userID)) | ||
| defer unlock() | ||
|
|
||
| err := c.syncWithLock(ctx, userID) |
There was a problem hiding this comment.
This might need some name change or a comment. I thought it had to do something with locking or it might be a potential blocking call.
There was a problem hiding this comment.
Agreed. Renamed syncWithLock() to syncIfStale() + lock comment - to follows convention from Designing Data Intensive Applications (DDIA) ch. 5 Replication.
| return isTooEarly || isInternal | ||
| } | ||
|
|
||
| func (c *Cache) retryPersist(ctx context.Context, span trace.Span, userID, spaceID string, persistFunc func() error) error { |
There was a problem hiding this comment.
Any advantage of passing the span as parameter?
In theory, if you need access to the span, you should get the span from the context. In general, I think that whoever creates a new span is responsible for filling it and close it, so span shouldn't leave the function that has created it.
|
|
||
| // CS3 represents a metadata storage with a cs3 storage backend | ||
| type CS3 struct { | ||
| log zerolog.Logger |
There was a problem hiding this comment.
Any chance to get the logger from the context? In theory, it could provide additional information from the context (usually associated to the request).
7936a29 to
8ee685d
Compare
jvillafanez
left a comment
jvillafanez
left a comment
There was a problem hiding this comment.
Just a comment for future PRs. It's a bit out of scope for this one.
| if !isSyncTransient(serr) { | ||
| span.RecordError(serr) | ||
| span.SetStatus(codes.Error, serr.Error()) | ||
| log.Error().Int("attempt", attempt).Err(serr).Msg("lost update: re-read failed, aborting") | ||
| return serr |
There was a problem hiding this comment.
I think it's weird that we only set the error and status code of the span here. If the caller creates the span, it should be the caller the one closing it AND setting the appropriate status code. I think all the errors returned by the function should be registered, not just this one.
|
7 participants
No description provided.