search

mailgun / groupcache

Clone of golang/groupcache with TTL and Item Removal support
Apache License 2.0
484 stars 72 forks source link

Make LocalRemove public #36

Closed whs closed 2 years ago

whs commented 2 years ago

When implementing groupcache.ProtoGetter, calling Group.Remove is not an option as it would recursively resend the removal request to other peers. In the HTTP transport the private method localRemove is used to prevent such issue, but this would make out of package alternative transport (we're using gRPC) implementation impossible.

Arguably, making this private API public would make it misuse-prone, but I believe other than shipping all possible transports, there's no alternative. Unlike Get, Remove needs to be fired on all nodes so the terminating cause wouldn't be the same as Get.

I'm open to open sourcing our gRPC peer picker as well, if this upstream would accept it.

thrawn01 commented 2 years ago

I'm confused why Group.Remove() would cause an recursive issue. As the code is currently written no recursive issue is known of. Can you please explain the conditions where this would occur?

whs commented 2 years ago

The recursion scenario would occur in out of tree code:

  1. User call Remove
  2. Remove call removeFromPeer for all other peers
  3. If this PR is not merged, ProtoGetter implementor has to call Remove once removeFromPeer request has been received (HttpPool use private API so it will only call localRemove and not removeFromPeer)
  4. Which recurse into step 1

I did some more work and turns out more API needs to be public to accommodate for out-of-tree ProtoGetter so I already moved to HTTP2 based ProtoGetter.