-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
adds global onDiscard
hook
#3240
Conversation
this commit also improves the existing operators to use `onDiscard` in places where `onNextDropped` is used to drop violating values Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@@ -135,6 +135,7 @@ public void onSubscribe(Subscription s) { | |||
public void onNext(T t) { | |||
if (done) { | |||
Operators.onNextDropped(t, actual.currentContext()); | |||
Operators.onDiscard(t, actual.currentContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, unlike the more recent Operators.onDiscard
, Operators.onNextDropped
doesn't catch exceptions. maybe this could be added as an improvement alongside this PR?
otherwise, a throwing onNextDropped
hook would prevent the onDiscard
.
Reversing the order of the calls would also avoid this issue (and I think discarding is more important than signalling the malformed signal)
@@ -135,6 +135,7 @@ public void onSubscribe(Subscription s) { | |||
public void onNext(T t) { | |||
if (done) { | |||
Operators.onNextDropped(t, actual.currentContext()); | |||
Operators.onDiscard(t, actual.currentContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one pattern that could be applied everywhere is to call actual.currentContext()
only once, assigning the result to a local Context ctx
variable and using ctx
in both Operators
calls.
* | ||
* @param c the {@link Consumer} to apply to data (onNext) that is discarded | ||
*/ | ||
public static void onDiscard(Consumer<Object> c) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be better to offer a signature with a key, similar to eg. onEachOperator
.
In these keyed hooks, the variant adding without a key is generally legacy, so we wouldn't even add one for onDiscard
. So the signatures would become something like:
public static void onDiscard(String key, Consumer<Object>);
public static void resetOnDiscard(String key);
public static void resetOnDiscard();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it is better to use class as a key. E.g onDiscard(Class<T> clazz, Consumer<T> consumer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh couldn't there be legitimate cases where multiple libraries would want to add a hook for the same class with slightly different behaviors (or complementary ones)? e.g. for ByteBuf
we could have one hook by Reactor-Netty which releases the buffer, while (Micrometer/Sleuth/something) installs a hook that tracks a "netty.bytebuf.discarded" metric.
Adds try-catch to onNextDropped Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Closing as per #2103 (comment) |
This PR introduces the global
onDiscard
hookAlso, it improves the existing operators to use
onDiscard
in places whereonNextDropped
is used as wellSigned-off-by: Oleh Dokuka odokuka@vmware.com