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>
OlegDokuka
added
type/enhancement
| @@ -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.
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.
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.
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.
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.
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>
OlegDokuka
added
the
status/on-hold
This PR introduces the global
onDiscardhookAlso, it improves the existing operators to use
onDiscardin places whereonNextDroppedis used as wellSigned-off-by: Oleh Dokuka odokuka@vmware.com