Remove dead client producer from pub sub #1429
Conversation
|
There is no consensus that this is the right approach to this issue. Please don't merge until an agreement has been noted. |
|
Agree, lets keep this as is without merging, until we investigate other options. |
|
Regarding the ClientNotAvailableException The original exception was a KeyNotFoundException, so the ClientNotAvailableException was added as a specialization of KeyNotFoundException to preserve any error handling that may be in place for this error. I'm not opposed to the exception being a specialization of OrleansExceptions, which it should be, but we should be aware that this is a change to the public behavior, which a specialization of KeyNotFoundException would not be. |
|
Yes, I understand. But I would not worry about |
|
@gabikliot Agreed. |
|
The plan on this subject is:
|
|
re 2) is there is a way to force a failure of a client in |
|
One way is to host the client in a separate app domain and unload it without calling Client.Unitialize. Another is to add support to "KillClient" with some test hook. I wander what @jdom thinks. |
|
IMO, I think the AppDomain unloading should be a closer-to-reality test. Regarding the exception, I agree that it's pretty unlikely users are already catch KeyNotFoundException, so breaking it by specializing OrleansException should be OK. Nevertheless, these kind of sneaky breaking changes in behavior MUST be captured in the changelog... I think we should not way for the release date to start capturing things in the changelog, as we will most likely forget about them by that time. |
|
Thumbs up on the changelog. Great idea! |
gabikliot
changed the title
jason-bragg
force-pushed
the
RemoveClientProducerFromPubSub
branch
from
b60ea0f
to
bc73477
Compare
jason-bragg
changed the title
|
Updated and tested with "Client Streaming Tests #1637" |
| @@ -29,6 +29,14 @@ protected OrleansException(SerializationInfo info, StreamingContext context) | |||
| } | |||
| } | |||
|
|
|||
| public static class OrleansExceptionExtensions | |||
| { | |||
| public static bool IsClientNotAvailableException(this OrleansException orleansException) | |||
There was a problem hiding this comment.
?? Why not check exception type? even better, use standard catch block?
There was a problem hiding this comment.
Yeah, this sucks. I had planned to discuss this after this PR, but we can do this now. :)
The problem is that when the catalog encounters this condition, it rejects the message and rejected messages always come back as OrleansException. The original exception becomes the message of the Orleans exception. I was not sure what depended on this behavior so I chose not to change it. This extension class allowed me to avoid addressing what was potentially a breaking change (how we handle rejection responses) in this PR. :/
I think the behavior we want would be:
If original exception is of type OrleansException (which all should be), we pass it through. If it is not, we create an OrleansException with the message and inner exception coming from the oringial exception.
Questions:
Should we address the rejection exception change in this PR?
If so, is the above behavior desirable, or is there better alternative?
There was a problem hiding this comment.
Dug into this with Sergey. We've an approach I'm going to attempt. Pls see next iteration.
|
Looks good overall, some small comments. CI is stuck for some reason. |
|
@dotnet-bot test this please |
jason-bragg
force-pushed
the
RemoveClientProducerFromPubSub
branch
from
bc73477
to
ab493db
Compare
|
Not ready for merge. I'm modifying rejected message error handling to preserve original exception type. Will note when complete. |
…or grains no longer acting as producers.
jason-bragg
force-pushed
the
RemoveClientProducerFromPubSub
branch
from
ab493db
to
b869d0c
Compare
|
Rejected message error handling now preserves original exception type if original exception was of type OrleansException. PubSub grain error handling adjusted accordingly. CI build seems to be stuck, but assuming that gets resolved, I think this is ready to go, unless there are more comments. |
|
I hope you know that is was not me who added that "up-for-grabs" label. |
The pubsub system was unable to detect (and remove) dead producers on unavailable clients. This would put the pubsub system in a non-recoverable state where all further subscriptions on a stream would fail.