Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
jiangpengcheng commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2506971100 Hi @lhotari, here is the follow-up PIP: https://github.com/apache/pulsar/pull/23659, PTAL when you have time -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
jiangpengcheng commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2502361011 @lhotari many thx for the explain! I will create a PIP for. this change -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
lhotari commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2501057772 The reasons why this is binary compatible seems to be related to these points described in [Java Language Specification about binary compatibility](https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html): * Inserting new class or interface types in the type hierarchy. (`BaseContext` was inserted) * Moving a method upward in the class hierarchy. (methods were moved from `WindowContext` to `BaseContext`) -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
lhotari commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2501041145 > I just verified it with below steps: @jiangpengcheng thanks for checking. It seems that my concerns about breaking binary compatibility don't apply. We don't need to revert this change. It would be useful to have a PIP to document that this change was made in the public 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
jiangpengcheng commented on PR #23628:
URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2500747511
@lhotari ok, I thought we need to add integration tests to the CI
I just verified it with below steps:
1. checkout pulsar to previous version
(eddf395631811a731fe9c0284b44fd2f6efd2026)
2. update the `LoggingWindowFunction` like below:
```
package org.apache.pulsar.functions.api.examples.window;
import java.util.Collection;
import org.apache.pulsar.functions.api.Record;
import org.apache.pulsar.functions.api.WindowContext;
import org.apache.pulsar.functions.api.WindowFunction;
import org.slf4j.Logger;
/**
* A function that demonstrates how to redirect logging to a topic.
* In this particular example, for every input string, the function
* does some logging. If --logTopic topic is specified, these log statements
* end up in that specified pulsar topic.
*/
public class LoggingWindowFunction implements WindowFunction {
@Override
public Void process(Collection> inputs, WindowContext
context) throws Exception {
Logger log = context.getLogger();
log.info("tenant: {}, namespace: {}, instanceId: {}, replicas: {}",
context.getTenant(), context.getNamespace(),
context.getInstanceId(), context.getNumInstances());
for (Record record : inputs) {
log.info(record + "-window-log");
}
return null;
}
}
```
3. build the `pulsar-functions/java-examples` project, it generated a `--jar
pulsar-functions/java-examples/target/pulsar-functions-api-examples.jar`
4. checkout pulsar to master (7909d2dfdb4aad8053c133ce6a00d5dddf0b9db8)
5. start the pulsar locally:
```
./bin/pulsar standalone
```
6. create the window function
```
./bin/pulsar-admin functions create --tenant public --namespace default
--name window-function --className
org.apache.pulsar.functions.api.examples.window.LoggingWindowFunction --inputs
persistent://public/default/test-window-input --output
persistent://public/default/test-window-output --log-topic
persistent://public/default/test-window-logs --jar
pulsar-functions/java-examples/target/pulsar-functions-api-examples.jar --cpu
0.1 --window-length-count 10 --sliding-interval-count 5
```
7. produce 5 messages to the input topic:
```
./bin/pulsar-client produce -m "test-message" --value-schema string -n 5
persistent://public/default/test-window-input
```
8. consume from the log topic:
```
./bin/pulsar-client consume -n 0 -s mysub --subscription-position Earliest
persistent://public/default/test-window-logs
```
9. the function works correctly and I got expected messages from the log
topic:
```
- got message -
key:[null], properties:[fqn=public/default/window-function, instance=0,
loglevel=INFO], content:Window Config: WindowConfig(windowLengthCount=10,
windowLengthDurationMs=null, slidingIntervalCount=5,
slidingIntervalDurationMs=null, lateDataTopic=null, maxLagMs=null,
watermarkEmitIntervalMs=null, timestampExtractorClassName=null,
actualWindowFunctionClassName=org.apache.pulsar.functions.api.examples.window.LoggingWindowFunction,
processingGuarantees=ATLEAST_ONCE)
- got message -
key:[null], properties:[fqn=public/default/window-function, instance=0,
loglevel=INFO], content:tenant: public, namespace: default, instanceId: 0,
replicas: 1
- got message -
key:[null], properties:[fqn=public/default/window-function, instance=0,
loglevel=INFO],
content:org.apache.pulsar.functions.source.PulsarFunctionRecord@32afdbf6-window-log
- got message -
key:[null], properties:[fqn=public/default/window-function, instance=0,
loglevel=INFO],
content:org.apache.pulsar.functions.source.PulsarFunctionRecord@467cda10-window-log
- got message -
key:[null], properties:[fqn=public/default/window-function, instance=0,
loglevel=INFO],
content:org.apache.pulsar.functions.source.PulsarFunctionRecord@3e25cfb6-window-log
- got message -
key:[null], properties:[fqn=public/default/window-function, instance=0,
loglevel=INFO],
content:org.apache.pulsar.functions.source.PulsarFunctionRecord@3997d10c-window-log
- got message -
key:[null], properties:[fqn=public/default/window-function, instance=0,
loglevel=INFO],
content:org.apache.pulsar.functions.source.PulsarFunctionRecord@752bcda3-window-log
- got message -
key:[null], properties:[fqn=public/default/window-function, instance=0,
loglevel=INFO], content:[persistent://public/default/test-window-input]
[public/default/window-function] [ef1db] Prefetched messages: 0 --- Consume
throughput received: 0.08 msgs/s --- 0.00 Mbit/s --- Ack sent rate: 0.08 ack/s
--- Failed messages: 0 --- batch messages: 0 ---Failed acks: 0
- got message -
key:[null], properties:[fqn=public/default/win
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
lhotari commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2500662024 > Validating compatibility seems like a significant effort. What if this PR doesn’t need to be cherry-picked to older branches and is only merged into the master branch? Would creating a PIP and resubmitting the PR suffice? @jiangpengcheng It's not a significant effort. You simply have a function jar using one of the moved methods in WindowContext and have it compiled with a previous version of Pulsar. You then run this with the master branch version of Pulsar. The first step of this is to do this test manually. The required effort is around 10 to 20 minutes. I don't think it's justified to break binary compatibility when there's a chance to preserve it. -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
jiangpengcheng commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2500437083 Validating compatibility seems like a significant effort. What if this PR doesn’t need to be cherry-picked to older branches and is only merged into the master branch? Would creating a PIP and resubmitting the PR suffice? -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
lhotari commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2500344766 > @lhotari sorry for the trouble, so now should I create a PIP for this one or: > > 1. revert his PR > 2. create a PIP, discuss and vote @jiangpengcheng Makes sense. The need for the PIP is due to the change in a public interface. However creating a PIP isn't useful alone since in this case, it will require experimentation to find an optimal solution. An optimal solution would be one which doesn't introduce breaking changes in binary compatibility or source compatibility. > 3. and then re-submit this PR ? Most likely we need a slightly different solution than this PR. A solution should be sought where binary compatibility could be preserved. For checking binary compatibility, it would be useful to have an integration test where a function compiled for a previous version is run with the master branch version. Since adding the test could be a lot of work, the first step would be to perform this checking manually and having a test case available at least in some branch to find a solution where binary compatibility would be preserved. It's possible that the solution for preserving binary compatibility is simply one where methods don't get removed from the WindowContext interface and it's simply modified to extend `BaseContext` interface. I might also be wrong that binary compatibility is broken in the changes that this PR introduces. That needs to be validated in any case, since it's hard to know the corner cases of binary compatibility just by analyzing source code. -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
jiangpengcheng commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2500268204 @lhotari sorry for the trouble, so now should I create a PIP for this one or: 1. revert his PR 2. create a PIP, discuss and vote 3. and then re-submit this PR ? -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
lhotari commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2500145298 > @shibd @jiangpengcheng @Technoboy- This is a breaking change and shouldn't be cherry-picked. Existing compiled classes will have to be recompiled. This change breaks binary compatibility although source compatibility is retained. @jiangpengcheng Please respond. Since this is a breaking change, I think that we need a PIP for this so that we have properly documented a breaking change. There could also be ways to make the change in a way that it doesn't break binary compatibility. -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Technoboy- merged PR #23628: URL: https://github.com/apache/pulsar/pull/23628 -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
shibd commented on PR #23628: URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2497176190 /pulsarbot rerun-failure-checks -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
jiangpengcheng commented on code in PR #23628:
URL: https://github.com/apache/pulsar/pull/23628#discussion_r1856025480
##
pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/windowing/WindowContextImpl.java:
##
@@ -80,6 +87,11 @@ public String getOutputTopic() {
return this.context.getOutputTopic();
}
+@Override
+public Record getCurrentRecord() {
Review Comment:
changed to extend `BaseContext`, this is more reasonable
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
