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.

Sign up for GitHub

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
Open

Add default implements for IAsyncObserver and IAsyncBatchObserver #8783

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
122f273
Add default implements for IAsyncObserver and IAsyncBatchObserver
bluexo Dec 18, 2023
6bc9faf
Update IAsyncBatchObserver.cs
bluexo Dec 22, 2023
bfe3f20
Update IAsyncObserver.cs
bluexo Dec 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Orleans.Streaming/Core/IAsyncBatchObserver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public interface IAsyncBatchObserver<T>
/// </para>
/// </summary>
/// <returns>A Task that is completed when the stream-complete operation has been accepted.</returns>
Task OnCompletedAsync();
Task OnCompletedAsync() => Task.CompletedTask;

/// <summary>
/// Notifies the consumer that the stream had an error.
Expand All @@ -84,6 +84,6 @@ public interface IAsyncBatchObserver<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;
}
}
4 changes: 2 additions & 2 deletions src/Orleans.Streaming/Core/IAsyncObserver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public interface IAsyncObserver<in T>
/// </para>
/// </summary>
/// <returns>A Task that is completed when the stream-complete operation has been accepted.</returns>
Task OnCompletedAsync();
Task OnCompletedAsync() => Task.CompletedTask;

/// <summary>
/// Notifies the consumer that the stream had an error.
Expand All @@ -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 •
edited
Loading

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.

}
}
Loading