[fix][fn] Align WindowContext with BaseContext #23628

jiangpengcheng · 2024-11-22T08:21:09Z

Fixes #23629

Main Issue: #xyz

PIP: #xyz

Motivation

Modifications

Verifying this change

Make sure that the change passes the CI checks.
This change is already covered by existing tests, such as (please describe tests).
- org.apache.pulsar.tests.integration.functions.java.testTumblingCountWindowTest
- org.apache.pulsar.tests.integration.functions.java.testSlidingCountWindowTest

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: jiangpengcheng#35

github-actions · 2024-11-22T08:21:39Z

@jiangpengcheng Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

codecov-commenter · 2024-11-25T02:09:27Z

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 74.29%. Comparing base (bbc6224) to head (aced084).
Report is 746 commits behind head on master.

Files with missing lines	Patch %	Lines
.../pulsar/functions/windowing/WindowContextImpl.java	0.00%	10 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #23628      +/-   ##
============================================
+ Coverage     73.57%   74.29%   +0.72%     
- Complexity    32624    34456    +1832     
============================================
  Files          1877     1944      +67     
  Lines        139502   147197    +7695     
  Branches      15299    16239     +940     
============================================
+ Hits         102638   109366    +6728     
- Misses        28908    29384     +476     
- Partials       7956     8447     +491

Flag	Coverage Δ
inttests	`27.36% <0.00%> (+2.77%)`	⬆️
systests	`24.38% <0.00%> (+0.05%)`	⬆️
unittests	`73.68% <0.00%> (+0.84%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../pulsar/functions/windowing/WindowContextImpl.java	`8.10% <0.00%> (-3.01%)`	⬇️

... and 658 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

...unctions/instance/src/main/java/org/apache/pulsar/functions/windowing/WindowContextImpl.java

shibd · 2024-11-25T08:09:01Z

/pulsarbot rerun-failure-checks

lhotari · 2024-11-25T15:24:10Z

@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.

lhotari · 2024-11-26T09:47:31Z

@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.

jiangpengcheng · 2024-11-26T10:42:00Z

@lhotari sorry for the trouble, so now should I create a PIP for this one or:

revert his PR
create a PIP, discuss and vote
and then re-submit this PR ?

lhotari · 2024-11-26T11:15:16Z

@lhotari sorry for the trouble, so now should I create a PIP for this one or:

revert his PR

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.

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.

jiangpengcheng · 2024-11-26T11:38:17Z

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?

lhotari · 2024-11-26T12:25:23Z

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.

jiangpengcheng · 2024-11-26T13:02:24Z

@lhotari ok, I thought we need to add integration tests to the CI

I just verified it with below steps:

checkout pulsar to previous version (eddf395)
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<String, Void> {

    @Override
    public Void process(Collection<Record<String>> 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<String> record : inputs) {
            log.info(record + "-window-log");
        }
        return null;
    }
}

build the pulsar-functions/java-examples project, it generated a --jar pulsar-functions/java-examples/target/pulsar-functions-api-examples.jar
checkout pulsar to master (7909d2d)
start the pulsar locally:

./bin/pulsar standalone

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

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

consume from the log topic:

./bin/pulsar-client consume -n 0 -s mysub --subscription-position Earliest persistent://public/default/test-window-logs

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/window-function, instance=0, loglevel=INFO], content:[persistent://public/default/test-window-logs] [standalone-23-1] --- Publish throughput: 0.12 msg/s --- 0.00 Mbit/s --- Latency: med: 117.000 ms - 95pct: 117.000 ms - 99pct: 117.000 ms - 99.9pct: 117.000 ms - max: 117.000 ms --- BatchSize: med: 6.000 - 95pct: 6.000 - 99pct: 6.000 - 99.9pct: 6.000 - max: 6.000 --- MsgSize: med: 437.000 bytes - 95pct: 437.000 bytes - 99pct: 437.000 bytes - 99.9pct: 437.000 bytes - max: 437.000 bytes --- Ack received rate: 0.12 ack/s --- Failed messages: 0 --- Pending messages: 1
2024-11-26T12:54:54,980+0000 [pulsar-timer-16-1] INFO  org.apache.pulsar.client.impl.ConsumerStatsRecorderImpl - [persistent://public/default/test-window-logs] [mysub] [9415c] Prefetched messages: 0 --- Consume throughput received: 0.15 msgs/s --- 0.00 Mbit/s --- Ack sent rate: 0.15 ack/s --- Failed messages: 0 --- batch messages: 0 ---Failed acks: 0

lhotari · 2024-11-26T14:58:29Z

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.

lhotari · 2024-11-26T15:04:34Z

The reasons why this is binary compatible seems to be related to these points described in Java Language Specification about binary compatibility:

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)

jiangpengcheng · 2024-11-27T00:49:14Z

@lhotari many thx for the explain! I will create a PIP for. this change

jiangpengcheng · 2024-11-29T02:20:37Z

Hi @lhotari, here is the follow-up PIP: #23659, PTAL when you have time

[fix][fn] Align WindowContext with Context

848a56d

github-actions bot added the doc-label-missing label Nov 22, 2024

jiangpengcheng closed this Nov 22, 2024

github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Nov 22, 2024

jiangpengcheng reopened this Nov 22, 2024

freeznet approved these changes Nov 22, 2024

View reviewed changes

update

552f538

mattisonchao approved these changes Nov 25, 2024

View reviewed changes

shibd requested changes Nov 25, 2024

View reviewed changes

...unctions/instance/src/main/java/org/apache/pulsar/functions/windowing/WindowContextImpl.java Outdated Show resolved Hide resolved

jiangpengcheng added 2 commits November 25, 2024 16:00

extends to BaseContext

f6ac5cb

revert comment

0171f1d

jiangpengcheng changed the title ~~[fix][fn] Align WindowContext with Context~~ [fix][fn] Align WindowContext with BaseContext Nov 25, 2024

shibd approved these changes Nov 25, 2024

View reviewed changes

shibd assigned jiangpengcheng Nov 25, 2024

shibd added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages release/3.0.9 release/3.3.4 release/4.0.2 ready-to-test labels Nov 25, 2024

fix style

aced084

Technoboy- added this to the 4.1.0 milestone Nov 25, 2024

Technoboy- merged commit 7909d2d into apache:master Nov 25, 2024
52 checks passed

lhotari removed release/3.0.9 release/3.3.4 release/4.0.2 labels Nov 25, 2024

jiangpengcheng mentioned this pull request Nov 29, 2024

[improve][pip] PIP-396: Align WindowFunction's WindowContext with BaseContext #23659

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][fn] Align WindowContext with BaseContext #23628

[fix][fn] Align WindowContext with BaseContext #23628

jiangpengcheng commented Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024

codecov-commenter commented Nov 25, 2024 •

edited

Loading

shibd commented Nov 25, 2024

lhotari commented Nov 25, 2024 •

edited

Loading

lhotari commented Nov 26, 2024

jiangpengcheng commented Nov 26, 2024

lhotari commented Nov 26, 2024

jiangpengcheng commented Nov 26, 2024

lhotari commented Nov 26, 2024

jiangpengcheng commented Nov 26, 2024

lhotari commented Nov 26, 2024

lhotari commented Nov 26, 2024

jiangpengcheng commented Nov 27, 2024

jiangpengcheng commented Nov 29, 2024

[fix][fn] Align WindowContext with BaseContext #23628

[fix][fn] Align WindowContext with BaseContext #23628

Conversation

jiangpengcheng commented Nov 22, 2024 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

github-actions bot commented Nov 22, 2024

codecov-commenter commented Nov 25, 2024 • edited Loading

Codecov Report

shibd commented Nov 25, 2024

lhotari commented Nov 25, 2024 • edited Loading

lhotari commented Nov 26, 2024

jiangpengcheng commented Nov 26, 2024

lhotari commented Nov 26, 2024

jiangpengcheng commented Nov 26, 2024

lhotari commented Nov 26, 2024

jiangpengcheng commented Nov 26, 2024

lhotari commented Nov 26, 2024

lhotari commented Nov 26, 2024

jiangpengcheng commented Nov 27, 2024

jiangpengcheng commented Nov 29, 2024

jiangpengcheng commented Nov 22, 2024 •

edited

Loading

codecov-commenter commented Nov 25, 2024 •

edited

Loading

lhotari commented Nov 25, 2024 •

edited

Loading