Repository navigation
Conversation
| // the analyzed batch's. It fails closed: any signature it cannot obtain fails | ||
| // the analysis. | ||
| func New(params Params) conflict.Analyzer { | ||
| if params.Logger == nil { |
There was a problem hiding this comment.
i would skip nil check here, logger not being passed, we should just panic
| for _, l := range labels { | ||
| err := a.Dictionary.Create(ctx, TargetLabel{Queue: queue, ID: l.id, Label: l.label, CreatedAt: createdAt}) | ||
| if err != nil && !errors.Is(err, storage.ErrAlreadyExists) { | ||
| a.Logger.Warnw("failed to record target label", "queue", queue, "id", l.id, "label", l.label, "error", err) |
There was a problem hiding this comment.
i think @sbalabanov wanted it to not be best-effort but always fail if fail here?
There was a problem hiding this comment.
This is the target dictionary not the signature store.
There was a problem hiding this comment.
i think even for labels, we should not skip, because then what's the point of storing it if we can live without one. So having a store which is source of truth for this data, we should ensure we have it recorded post this stage, so when it's required in later stages say conflict relaxation or any other then it's there and not missing. Because if missing, we will end-up adding another fallback in those stages again
This branch has not been deployed
2 participants
Why
Signatures hold only 64-bit target IDs, so an overlap can't be traced back to targets. The dictionary store exists but nothing writes to it, and labels are only available while a batch is signed.
What
ErrAlreadyExistsis success, and on a hash collision the first label stays.ParamsgainsDictionaryandLogger.Test plan
go test -race ./submitqueue/extension/conflict/tango/...: labels are written once on creation, not when the signature already exists, and failures are logged without failing the analysis.