Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default implements for IAsyncObserver and IAsyncBatchObserver #8783

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bluexo
Copy link
Contributor

@bluexo bluexo commented Dec 18, 2023

This commit adds default implementations for two methods, OnCompletedAsync and OnErrorAsync, in the IAsyncObserver interface . typically, these two methods are not mandatory to implement.

Microsoft Reviewers: Open in CodeFlow

@@ -56,6 +56,6 @@ public interface IAsyncObserver<in T>
/// </summary>
/// <param name="ex">An Exception that describes the error that occurred on the stream.</param>
/// <returns>A Task that is completed when the close has been accepted.</returns>
Task OnErrorAsync(Exception ex);
Task OnErrorAsync(Exception ex) => throw ex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to add something to this.

I have seen that the Orleans' audience point-of-view for streams is not very well aligned with how it's actually implemented and/or indented to be used. I would argue that a lot of people use it more for decoupling purposes as opposed to a throughput pipeline (which IMO there is nothing wrong with it).

That being said: typically this method will be raised on QueueCacheMissException which happens on read from the cache and it doesn't mean anything is wrong other than your cache window being too small, or processing is taking long(er). I would argue that most people (including me) are fine with swallowing those exceptions.

Why I argue against throw ex?

Having this default implementation, typically means people will not bother overriding the behavior.
I see 3 types of people/usages for this:

  1. Those who don't care about these errors:
    OnErrorAsync(Exception ex) => Task.CompletedTask

  2. Those who care about errors but don't care about cache misses:

OnErrorAsync(Exception ex)
{
    if (ex is not QueueCacheMissException) 
    {
         throw ex;
    }
}
  1. Those who care about every kind of error:
    OnErrorAsync(Exception ex) => throw ex;

I have a really hard time beliving that most of the audience belongs to number 3, therefor I'd suggest to have the default implementation Task.CompletedTask, and the audiences 2 and 3 can override it accordingly to the needs.

p.s: the same holds for IAsyncBatchObserver<T>

Copy link
Contributor Author

@bluexo bluexo Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback. Your opinions and analysis make sense. We may need to consider developers who are not particularly familiar with Orleans Streaming. Explicitly handling errors in streaming can avoid misunderstandings, especially for beginners. Therefore, I will adopt your suggestion and remove the default implementation for OnErrorAsync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants