-
Notifications
You must be signed in to change notification settings - Fork 112
chunk, pushsync: do not increment synced tags in pushsync #1880
Conversation
// set chunk status to synced, insert to db GC index | ||
if err := p.store.Set(ctx, chunk.ModeSetSyncPush, syncedAddrs...); err != nil { | ||
log.Error("pushsync: error setting chunks to synced", "err", err) | ||
} | ||
|
||
// delete from pushed items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we now increment synced tags in the localstore, we can only check if the root span is to be closed after we have performed the set operations.
tag := item.tag | ||
if tag != nil { | ||
tag.Inc(chunk.StateSynced) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer increment synced tags on receive of receipts, but we still close the spans for individual chunks as soon as we get a receipt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pushsync/pusher.go
Outdated
if found { | ||
tag := item.tag | ||
if tag != nil { | ||
if tag.Done(chunk.StateSynced) { | ||
p.logger.Debug("closing root span for tag", "taguid", tag.Uid, "tagname", tag.Name) | ||
tag.FinishRootSpan() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Pyramid of Doom can be simplified:
if found && item.tag != nil && item.tag.Done(chunk.StateSynced) {
tag := item.tag
p.logger.Debug("closing root span for tag", "taguid", tag.Uid, "tagname", tag.Name)
tag.FinishRootSpan()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix, thanks! I have to see why the tests are failing, not sure if they are flaky, or the change to upload.go
does indeed have an effect.
This PR is updating the pushsync event loop to not increment tags for synced chunks on receive of receipts (as we are already doing that as part of the localstore mode set).