You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "jiangpengcheng (via GitHub)" <gi...@apache.org> on 2024/11/22 08:21:11 UTC
[PR] [fix][fn] Align WindowContext with Context [pulsar]
jiangpengcheng opened a new pull request, #23628:
URL: https://github.com/apache/pulsar/pull/23628
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes an issue, -->
Fixes #xyz
<!-- or this PR is one task of an issue -->
Main Issue: #xyz
<!-- If the PR belongs to a PIP, please add the PIP link here -->
PIP: #xyz
<!-- Details of when a PIP is required and how the PIP process work, please see: https://github.com/apache/pulsar/blob/master/pip/README.md -->
### Motivation
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
### Modifications
<!-- Describe the modifications you've done. -->
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `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 -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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
Re: [PR] [fix][fn] Align WindowContext with Context [pulsar]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23628:
URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2493149443
@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 -->
```
--
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
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "jiangpengcheng (via GitHub)" <gi...@apache.org>.
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<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;
}
}
```
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/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
```
--
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
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with Context [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #23628:
URL: https://github.com/apache/pulsar/pull/23628#discussion_r1856000809
##########
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:
I think it's best to keep the approach of defining a separate interface for WindowsContext. For example, in the semantics of Windows functions, which record does `getCurrentRecord` refer to? We cannot provide this method, right?
--
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
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "jiangpengcheng (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with Context [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23628:
URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2496531482
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23628?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `15 lines` in your changes missing coverage. Please review.
> Project coverage is 74.33%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`552f538`)](https://app.codecov.io/gh/apache/pulsar/commit/552f538d7fb21cd5998cdfb1a41579b764e602a3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 743 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23628?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../pulsar/functions/windowing/WindowContextImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23628?src=pr&el=tree&filepath=pulsar-functions%2Finstance%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Ffunctions%2Fwindowing%2FWindowContextImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9pbnN0YW5jZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Z1bmN0aW9ucy93aW5kb3dpbmcvV2luZG93Q29udGV4dEltcGwuamF2YQ==) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/23628?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/23628?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #23628 +/- ##
============================================
+ Coverage 73.57% 74.33% +0.76%
- Complexity 32624 34981 +2357
============================================
Files 1877 1944 +67
Lines 139502 147201 +7699
Branches 15299 16239 +940
============================================
+ Hits 102638 109429 +6791
- Misses 28908 29305 +397
- Partials 7956 8467 +511
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23628/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.28% <0.00%> (+2.70%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/23628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.37% <0.00%> (+0.04%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.72% <0.00%> (+0.87%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23628?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../pulsar/functions/windowing/WindowContextImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23628?src=pr&el=tree&filepath=pulsar-functions%2Finstance%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Ffunctions%2Fwindowing%2FWindowContextImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9pbnN0YW5jZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Z1bmN0aW9ucy93aW5kb3dpbmcvV2luZG93Q29udGV4dEltcGwuamF2YQ==) | `7.14% <0.00%> (-3.97%)` | :arrow_down: |
... and [662 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23628/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
----
🚨 Try these New Features:
- [Flaky Tests Detection](https://docs.codecov.com/docs/test-result-ingestion-beta) - Detect and resolve failed and flaky tests
- [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis) - Avoid shipping oversized bundles
--
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
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "jiangpengcheng (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "jiangpengcheng (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "jiangpengcheng (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with Context [pulsar]
Posted by "jiangpengcheng (via GitHub)" <gi...@apache.org>.
jiangpengcheng closed pull request #23628: [fix][fn] Align WindowContext with Context
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "jiangpengcheng (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][fn] Align WindowContext with BaseContext [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23628:
URL: https://github.com/apache/pulsar/pull/23628#issuecomment-2498320335
@shibd This is a breaking change and shouldn't be cherry-picked. Existing compiled classes will have to be recompiled.
--
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