Conversation
Farber98
left a comment
Farber98
left a comment
There was a problem hiding this comment.
Looks good to me. I'd wait Ilija's or Domino's reviews before approving
|
|
||
| r.dirty = true | ||
|
|
||
| r.filter.Addresses = append(r.filter.Addresses, address) |
There was a problem hiding this comment.
Seems like we should have some sort of check here to make sure the address isn't already in the list (or perhaps, use a set instead of a list?).
I'm not sure how often we unregister and re-register the same filter, but if it's a lol then we'll keep appending more and more copies of the same address onto the list.
There was a problem hiding this comment.
Good point, I noticed this is already handled by common and event readers (since they make precheck that address is bounded). So it won't keep appending copies at least in its current usage
There was a problem hiding this comment.
Prior to this PR yes, but with the introduction of this PR it becomes possible. I'll comment above on the place where the check is removed.
There was a problem hiding this comment.
That said, it doesn't pose any serious problem so if you want to leave it as-is I'm okay with that.
| } | ||
|
|
||
| cb.registrar.SetName(logpoller.FilterName(cb.name + "." + uuid.NewString())) | ||
| cb.registrar.AddAddress(binding) |
There was a problem hiding this comment.
Here is what I was referring to below. Previously, we were calling cb.Unregister which calls RemoveAddress before calling AddAddress, so it gets removed and then re-added--you can't add the same one twice. But now we skip the isBound so it can be added multiple times. Right?
There was a problem hiding this comment.
cb.Unregister() doesn't call RemoveAddress, we cant add address multiple times because of
if cb.bindingExists(binding) { continue }
|
4 participants
This PR fixes bug from this ticket