[GitHub] [pulsar] freeznet commented on a diff in pull request #16279: [refactor][function][PIP-166] Function add MANUAL delivery semantics

[GitBox <gi...@apache.org>]

freeznet commented on code in PR #16279:
URL: https://github.com/apache/pulsar/pull/16279#discussion_r932916868


##########
site2/docs/functions-concepts.md:
##########
@@ -167,6 +169,9 @@ Both processing time and event time are supported.
  * Processing time is defined based on the wall time when the function instance builds and processes a window. The judging of window completeness is straightforward and you don’t have to worry about data arrival disorder. 
  * Event time is defined based on the timestamps that come with the event record. It guarantees event time correctness but also offers more data buffering and a limited completeness guarantee.
 
+Delivery Semantic Guarantees.
+ * Currently, window function does not support `Effectively-once` delivery semantics.

Review Comment:
   Is `MANUAL` supported in Window Function?



##########
pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionConfigUtils.java:
##########
@@ -308,11 +308,20 @@ public static FunctionDetails convert(FunctionConfig functionConfig, ExtractedFu
         // windowing related
         WindowConfig windowConfig = functionConfig.getWindowConfig();
         if (windowConfig != null) {
+            // Windows Function not support EFFECTIVELY_ONCE
+            if (functionConfig.getProcessingGuarantees() == FunctionConfig.ProcessingGuarantees.EFFECTIVELY_ONCE) {
+                throw new IllegalArgumentException(
+                        "Windows Function not support EFFECTIVELY_ONCE delivery semantics.");
+            } else {
+                // Override functionConfig.getProcessingGuarantees to MANUAL, and set windowsFunction is guarantees
+                windowConfig.setProcessingGuarantees(WindowConfig.ProcessingGuarantees

Review Comment:
   `WindowConfig.ProcessingGuarantees` only support `ATMOST_ONCE` and `ATLEAST_ONCE`, but `functionDetails.ProcessingGuarantees` do have `MANUAL` and `EFFECTIVELY_ONCE` as well. This block do ignores `EFFECTIVELY_ONCE` case, but what if the `ProcessingGuarantees` is `MANUAL`? The `WindowConfig.ProcessingGuarantees.valueOf` may throw `IllegalArgumentException`, is it as expected? If yes, can we have some better logging here?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org