Skip to content

Conversation

@vaibhavtiwari33
Copy link
Contributor

@vaibhavtiwari33 vaibhavtiwari33 commented Nov 4, 2025

Closes #206

As part of #3043, we're adding support for OnSuccess sink which allows users to write to a separate sink when writing to primary sink succeeds for example.
This PR aims to add this support to Java SDK as well, giving the users ability to interact with the onSuccess sink.

Local testing:

Brought up a pipeline with a UDSink using java SDK with latest changes:
Screenshot 2025-11-05 at 3 33 00 PM

Able to see messages in the onSuccess UDSink being routed from the primary UDSink:
Screenshot 2025-11-05 at 3 34 56 PM

Mix of messages being written to fallback and onsuccess sink:
Screenshot 2025-11-05 at 3 35 16 PM

Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
@vaibhavtiwari33 vaibhavtiwari33 marked this pull request as draft November 4, 2025 01:19
vtiwari5 added 3 commits November 4, 2025 11:26
Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
…lculation

Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
@codecov
Copy link

codecov bot commented Nov 4, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 14 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@38734c0). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...main/java/io/numaproj/numaflow/sinker/Service.java 70.00% 1 Missing and 8 partials ⚠️
...n/java/io/numaproj/numaflow/sinker/GRPCConfig.java 0.00% 2 Missing and 1 partial ⚠️
...ain/java/io/numaproj/numaflow/sinker/Response.java 94.59% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #207   +/-   ##
=======================================
  Coverage        ?   60.78%           
  Complexity      ?      517           
=======================================
  Files           ?      152           
  Lines           ?     3448           
  Branches        ?      242           
=======================================
  Hits            ?     2096           
  Misses          ?     1181           
  Partials        ?      171           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

vtiwari5 added 5 commits November 5, 2025 10:01
Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
Comment on lines +160 to +162
.setOnSuccessMsg(response.getOnSuccessMessage() == null
? SinkOuterClass.SinkResponse.Result.Message.getDefaultInstance()
: response.getOnSuccessMessage())
Copy link
Contributor Author

@vaibhavtiwari33 vaibhavtiwari33 Nov 5, 2025

Choose a reason for hiding this comment

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

The current numaflow-core implementation for accepting onSuccess response from server allows sending None in lieu of explicit OnSuccessMessage, but the generated java protobuf for OnSuccessMessage prohibits setting the OnSuccessMessage as null, so a default instance is being sent to the client currently.

This leads to discrepancy between behaviours across different SDKs.

Question: Should we add functionality to numaflow-core to interpret empty value ("") as an indication of sending original message to the onSuccess sink?

@vaibhavtiwari33 vaibhavtiwari33 marked this pull request as ready for review November 5, 2025 20:44
vtiwari5 added 2 commits November 6, 2025 08:46
…changes

Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
…changes

Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
@vaibhavtiwari33 vaibhavtiwari33 requested a review from yhl25 November 6, 2025 13:48
Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
@yhl25
Copy link
Contributor

yhl25 commented Nov 12, 2025

Please add java doc for public classes.

Signed-off-by: vtiwari5 <vaibhav_tiwari1@intuit.com>
@vaibhavtiwari33 vaibhavtiwari33 merged commit e616ba6 into main Nov 14, 2025
5 checks passed
@vaibhavtiwari33 vaibhavtiwari33 deleted the on-success-sink branch November 14, 2025 19:26
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.

Java SDK Changes to Support OnSuccess Sink

3 participants