Ch.Part.API: Allow retry of failed removals

[stephyee]

Type of change

• Improvement

Description

Allow retrying of failed channel removals per comment

[yacovm]

Can't we simply make the pendingRemoval map, a map from string to struct{} ?

I see it this way:

1. If we are in the process of removing, then we have the element in the map, so we return a pending error.
2. If we succeeded the removal, then great, we return that it doesn't exist so we can't remove it.
3. Else, we failed the removal, so we just re-try and it's equivalent to the first removal attempt, no?

Why do we need to keep a boolean map value if we have 3 possible states, and 2 maps (existing channels and pending channels) which can encode 4 states?

[wlahti]

Agreed. Definitely makes sense.

[tock-ibm]

I disagree. The remove is asynch, so first the request thread stops the chain/follower, moves the chain/follower to the pendingRemove map, spawns a go-routine to remove it, and returns. You need some way of signaling to a potential second remove request thread that the remove is on-going in the background (with the go-routine spawned by the first). This is exactly the boolean in the third map - true signals an ongoing remove, false a failed remove, and the key-value is deleted when the remove succeeds. If we had a struct{} in there, there was no way of detecting that background removal is in progress or when it ended, for good or bad. Note that the chains/followers/pendingRemove maps are mutually exclusive. A map[string]boolean can encodes these 3 states, whereas a map[string]struct{} cannot.

In addition, see this comment here: #2027 (comment) which advocated for a

struct {
  cRel     types.ClusterRelation
  status  types.Status
}

in the pendingRemove map instead of a boolean, and a new additional status types.StatusFailure to make diagnosing failures even easier.

[wlahti]

Hmm, I'll admit to not being as familiar with the flow here and will defer to Yoav. :)

[yacovm]

You need some way of signaling to a potential second remove request thread that the remove is on-going in the background (with the go-routine spawned by the first).

The fact that the channel is in the pending map conveys that. As long as the mapping exists, there is a goroutine active, and whenever the mapping doesn't exist, there is no such goroutine. This implies that the goroutine deletes the mapping once it is done - whether it succeeded or failed.

[tock-ibm]

That is not a good idea.
The go-routine should set the mapping channelID->false when it fails and terminates. This way:

1. a List (GET request) of the channel can reveal that the removal had failed
2. subsequent Join requests are rejected (the ledger may be in an inconsistent state)
3. but remove can be retried safely.
   If we don't keep it in the map, the Registrar has no record that the removal had failed, or that the channel exists, since it is not in either channels/followers/pendingRemove.

[yacovm]

Under the assumption that we want to reject joins, then let's go ahead with what you propose

[tock-ibm]

Looks good to me!
Good suggestion by @yacovm .