[GitHub] [pulsar] sijie commented on a change in pull request #4127: [Issue#4110] [component/functions] Adding getActualMessage() method

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

sijie commented on a change in pull request #4127: [Issue#4110] [component/functions] Adding getActualMessage() method
URL: https://github.com/apache/pulsar/pull/4127#discussion_r279141314
 
 

 ##########
 File path: pulsar-functions/api-java/src/main/java/org/apache/pulsar/functions/api/Context.java
 ##########
 @@ -33,6 +33,12 @@
  * executing function
  */
 public interface Context {
+
+    /**
+     * Access the message associated with current input value.
+     */
+    Object getCurrentMessage();
 
 Review comment:
   @ConcurrencyPractitioner 
   
   what @jerrypeng proposed in #4110 "we could also add a getActualMessage() method to PulsarRecord" is the right direction. I guess what he means there is "add getAcutalMessage() to Record". because PulsarRecord is an internal class which is not publicly available to the developers who develop a function.
   
   I asked the question "can you add an example function on how people can use this method?", is to help you think from a function developer perspective. You have to be a *pulsar function user* before you know how to make a good API to pulsar functions users. If you write a pulsar function example to use the method you proposed, you will know whether the method is good or not.
   
   You can find the examples under https://github.com/apache/pulsar/tree/master/pulsar-functions/java-examples, which demonstrate how users can use the function API to develop functions. My suggestion to you is to write a pulsar function example first. It would help you a lot on understanding why we need this change and how to provide the right api to the developers.
   
   Now back to the discussion on the interface itself.
   
   1) I don't think we should add another `getCurrent*` interface in `Context`. It makes the interface very confused because there is already a `getCurrentRecord`. Hence it should be `getActualMessage` in `Context` as what @jerrypeng proposed.
   
   2) Returning `Object` doesn't help resolving the problem. The function-api dependency doesn't include `pulsar-client-api`, so developers don't know what type is this object and they can not cast it back to `Message<T>`.
   
   That means in order to implement the proposal that @jerrypeng proposed in #4110 , you have to do followings:
   
   - add `Message<T> getActualMessage()` to `Record<T>` interface.
   - introduce `pulsar-client-api` dependency to `pulsar-function-api`.
   
   If @srkukarni and @jerrypeng agree on introducing `pulsar-client-api` dependency to `pulsar-function-api`, then you can implement that as what I point out, and #4110 is done.
   
   However if there are concerns about the `pulsar-client-api` dependency in `pulsar-function-api`, then we have to go back and check my original proposal to add the support of using `Message<T>` as input type. Hence we don't need to include pulsar-client-api as pulsar-function-api, and it also supports people writing functions using java native `Function` interface.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services