Re: [PR] [fix] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
iosdev747 commented on PR #23094: URL: https://github.com/apache/pulsar/pull/23094#issuecomment-2257609391 Yes, agreed on this part, concurrency should be limited. And along with that we can have this flag that will prevent unloading in cases where there are too many namespaces to unload and/or unload bundles which are required (not on primary/secondary broker group). For the bug fix, I'll move it out in a separate PR so it can be merged quickly. -- 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] [cleanup][broker] Remove PersistentSubscription.getStats [pulsar]
nodece merged PR #23095: URL: https://github.com/apache/pulsar/pull/23095 -- 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] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
lhotari commented on code in PR #23094:
URL: https://github.com/apache/pulsar/pull/23094#discussion_r1696367524
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##
@@ -771,8 +782,13 @@ private CompletableFuture
filterAndUnloadMatchedNamespaceAsync(NamespaceIs
.map(tenant ->
adminClient.namespaces().getNamespacesAsync(tenant));
return FutureUtil.waitForAll(completableFutureStream)
.thenApply(namespaces -> {
-// if namespace match any policy regex, add it
to ns list to be unload.
+// Filter namespaces that have current cluster
in their replication_clusters
+// if namespace match any policy regex, add it
to ns list to be unloaded.
return namespaces.stream()
+.filter(namespaceName ->
adminClient.namespaces()
+
.getPoliciesAsync(namespaceName)
+.thenApply(policies ->
policies.replication_clusters.contains(cluster))
+.join())
Review Comment:
> Can you suggest how I can make it async?
There's a real problem already with the existing code, even without
unloading calls. I've explained some of that in the previous comment in
https://github.com/apache/pulsar/pull/23094#issuecomment-2257496114 .
One of the problems is that all tenants and namespaces will be listed
concurrently at once, without any concurrency limits. That alone will cause
problems.
To fix the problem, the solution for making asynchronous calls will need
concurrency limits. I'd suggest introducing a dependency to
```
com.spotify
completable-futures
0.3.6
```
and using the
https://github.com/spotify/completable-futures/blob/master/src/main/java/com/spotify/futures/ConcurrencyReducer.java
class for controlling the concurrency.
This challenge is that this is a systemic problem at a higher level and
solving this problem in this PR might feel overly complex. However, it's
possible to handle it incrementally and refactor later.
For making the code asynchronous without blocking calls, composition is
needed by using `thenCompose`/`thenApply`.
In this case, it's not trivial, so it requires a bit more thought than usual
since the unlimited concurrency problem needs to also be solved.
--
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] [improve][broker]Reuse method getAvailableBrokersAsync [pulsar]
crossoverJie commented on PR #23099: URL: https://github.com/apache/pulsar/pull/23099#issuecomment-2257577348 /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][io] Upgrade Debezium connector version to 2.6.1.Final [pulsar]
mukesh154 commented on code in PR #23078: URL: https://github.com/apache/pulsar/pull/23078#discussion_r1696373258 ## pom.xml: ## @@ -201,7 +201,7 @@ flexible messaging model and an intuitive client API. 2.4.10 1.2.4 8.12.1 -1.9.7.Final +2.6.1.Final Review Comment: From 2.6.2.Final onwards the postgres tester is not working. The consumer is expecting a message event just after the insertion into the table but the source is taking some time to process the event. I have tested this locally as well. I think it is due to PostgreSQL offset flush race condition. More details are here: 1. [Release Notes](https://debezium.io/blog/2024/05/30/debezium-2-6-2-final-released/) 2. [DBZ-7816](https://issues.redhat.com/browse/DBZ-7816) -- 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] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
lhotari commented on code in PR #23094:
URL: https://github.com/apache/pulsar/pull/23094#discussion_r1696368579
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##
@@ -771,8 +781,13 @@ private CompletableFuture
filterAndUnloadMatchedNamespaceAsync(NamespaceIs
.map(tenant ->
adminClient.namespaces().getNamespacesAsync(tenant));
return FutureUtil.waitForAll(completableFutureStream)
.thenApply(namespaces -> {
-// if namespace match any policy regex, add it
to ns list to be unload.
+// Filter namespaces that have current cluster
in their replication_clusters
+// if namespace match any policy regex, add it
to ns list to be unloaded.
return namespaces.stream()
+.filter(namespaceName ->
adminClient.namespaces()
+
.getPoliciesAsync(namespaceName)
+.thenApply(policies ->
policies.replication_clusters.contains(cluster))
+.join())
.filter(namespaceName ->
policyData.getNamespaces().stream().anyMatch(namespaceName::matches))
Review Comment:
this should be the first filter so that policies aren't retrieved
unnecessarily
--
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] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
lhotari commented on code in PR #23094:
URL: https://github.com/apache/pulsar/pull/23094#discussion_r1696367524
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##
@@ -771,8 +782,13 @@ private CompletableFuture
filterAndUnloadMatchedNamespaceAsync(NamespaceIs
.map(tenant ->
adminClient.namespaces().getNamespacesAsync(tenant));
return FutureUtil.waitForAll(completableFutureStream)
.thenApply(namespaces -> {
-// if namespace match any policy regex, add it
to ns list to be unload.
+// Filter namespaces that have current cluster
in their replication_clusters
+// if namespace match any policy regex, add it
to ns list to be unloaded.
return namespaces.stream()
+.filter(namespaceName ->
adminClient.namespaces()
+
.getPoliciesAsync(namespaceName)
+.thenApply(policies ->
policies.replication_clusters.contains(cluster))
+.join())
Review Comment:
> Can you suggest how I can make it async?
There's a real problem already with the existing code, even without
unloading calls. I've explained some of that in the previous comment in
https://github.com/apache/pulsar/pull/23094#issuecomment-2257496114 .
One of the problems is that all tenants and namespaces will be listed
concurrently at once, without any concurrency limits. That alone will cause
problems.
To fix the problem, the solution for making asynchronous calls will need
concurrency limits. I'd suggest introducing a dependency to
```
com.spotify
completable-futures
0.3.6
```
and using the
https://github.com/spotify/completable-futures/blob/master/src/main/java/com/spotify/futures/ConcurrencyReducer.java
class for controlling the concurrency.
This challenge is that this is a systemic problem at a higher level and
solving this problem in this PR might feel overly complex. However, it's
possible to handle it incrementally and refactor later.
For making the code asynchronous without blocking calls, composition is
needed by using `thenCompose`.
In this case, it's not trivial, so it requires a bit more thought than usual
since the unlimited concurrency problem needs to also be solved.
--
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][io] Upgrade Debezium connector version to 2.6.1.Final [pulsar]
mukesh154 commented on code in PR #23078: URL: https://github.com/apache/pulsar/pull/23078#discussion_r1696367462 ## tests/integration/src/test/java/org/apache/pulsar/tests/integration/containers/DebeziumMongoDbContainer.java: ## @@ -25,7 +25,7 @@ public class DebeziumMongoDbContainer extends ChaosContainer
Re: [PR] [fix] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
lhotari commented on PR #23094: URL: https://github.com/apache/pulsar/pull/23094#issuecomment-2257496114 > 2\. When there are too many namespaces in a cluster and adding a single namespace to the ns isolation policy data leads to too many bundle unload calls. This can exhaust broker resources (open connections). This should be controlled by a flag if bundle unload can be done post setPolicy call Good catch! This problem is related to the migration from synchronous calls to asynchronous calls in the Pulsar code base (/cc @mattisonchao). I have discussed the problem with @mattisonchao and there's some description [in this comment](https://github.com/apache/pulsar/pull/22541#issuecomment-2071568113). The main problem is that with asynchronous calls there aren't concurrency limits in place. One of the possible solutions would be to use https://github.com/spotify/completable-futures/blob/master/src/main/java/com/spotify/futures/ConcurrencyReducer.java to limit the concurrency of asynchronous calls. I wonder if we would need to also fix this problem although it seems like a useful solution to prevent unloading when policies are modified to reduce load on the system. -- 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: [I] [Bug] org.apache.pulsar:pulsar-package-bookkeeper-storage:jar:3.4.0-SNAPSHOT was not found [pulsar]
lhotari commented on issue #23096: URL: https://github.com/apache/pulsar/issues/23096#issuecomment-2257475790 This seems to be a reported about an issue with IntelliJ, however you didn't mention the IntelliJ version. I'd recommend ensuring that you are using the latest IntelliJ version with updates since there have been issues in the past where the user has been using an outdated version of IntelliJ. If that doesn't resolve the issue, I believe that this type of issues can be resolved by running `mvn clean install -DskipTests`. (for faster execution, I typically run `mvn -Pcore-modules,-main -T 1C clean install -DskipTests -Dspotbugs.skip=true -DnarPluginPhase=none`) We have IDE instructions at https://pulsar.apache.org/contribute/setup-ide/ . It's possible that the instructions are slightly outdated. -- 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] [improve] [test] add end to end deduplication test. [pulsar]
BewareMyPower commented on code in PR #23071:
URL: https://github.com/apache/pulsar/pull/23071#discussion_r1696283481
##
pulsar-broker/src/test/java/org/apache/pulsar/client/api/DeduplicationEndToEndTest.java:
##
@@ -0,0 +1,672 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.api;
+
+import io.netty.util.TimerTask;
+import org.apache.pulsar.broker.service.PulsarCommandSender;
+import org.apache.pulsar.broker.service.ServerCnx;
+import org.apache.pulsar.broker.service.persistent.PersistentTopic;
+import org.apache.pulsar.client.impl.ClientCnx;
+import org.apache.pulsar.client.impl.ConnectionHandler;
+import org.apache.pulsar.client.impl.MultiTopicsConsumerImpl;
+import org.apache.pulsar.client.impl.PartitionedProducerImpl;
+import org.apache.pulsar.client.impl.ProducerImpl;
+import org.apache.pulsar.client.impl.PulsarClientImpl;
+import org.awaitility.Awaitility;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+
+/**
+ * This test class is used to point out cases where message duplication can
occur,
+ * producer idempotency features can be used to solve which cases and can't
solve which cases.
+ * There are mainly two kinds of message duplication:
+ * 1. Producer sends the message but doesn't receive the ack due to network
issue, broker issue, etc.
+ * User receives the exception and sends the same message again.
+ * This resend operation is executed by the user, so the user can control the
sequence id of the message or not.
+ * Without message deduplication, the message is duplicated definitely.
+ * With message deduplication, the cases vary:
+ * - If user don't control the sequence id, the message is duplicated, as the
message is resent with a different sequence id.
+ * - If user control the sequence id, there are chances that the message is
duplicate or not, depending on whether
+ *the topic is single partitioned or multi partitioned, whether the
sequence id is used as the key of the message,
+ *whether the partition number is updated between the two messages.
+ *
+ * 2. The connection between the producer and the broker is broken after the
producer sends the message,
+ * but before the producer receives the ack. The producer reconnects to the
broker and sends the same message again internally.
+ * In this case, the producer can't control the sequence id of the message.
The resent message remains the same as the original message.
+ * - Without message deduplication, the message is duplicated.
+ * - With message deduplication, the message is not duplicated.
+ */
+@Test(groups = "broker-api")
+public class DeduplicationEndToEndTest extends ProducerConsumerBase {
+
+@BeforeClass
+@Override
+protected void setup() throws Exception {
+super.internalSetup();
+super.producerBaseSetup();
+}
+
+@AfterClass(alwaysRun = true)
+@Override
+protected void cleanup() throws Exception {
+super.internalCleanup();
+}
+
+private List> assertDuplicate(Consumer consumer,
byte[] data) throws PulsarClientException {
+// consume the message, there are at least two messages in the topic
+List> messages = new ArrayList<>(2);
+Message message = consumer.receive(1, TimeUnit.SECONDS);
+assertNotNull(message);
+assertEquals(message.getData(), data);
+messages.add(message);
+message = consumer.receive(1, TimeUnit.SECONDS);
+assertNotNull(message);
+assertEquals(message.getData(), data);
+messages.add(message);
+return messages;
+}
+
+private Message assertNotDuplicate(Consumer consumer,
byte[] data) throws PulsarClientException {
+// consume the
Re: [PR] [improve][txn] Take first snapshot before persisting the first transactional message [pulsar]
BewareMyPower commented on code in PR #21406:
URL: https://github.com/apache/pulsar/pull/21406#discussion_r1696281854
##
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/buffer/utils/TransactionBufferTestImpl.java:
##
@@ -0,0 +1,36 @@
+package org.apache.pulsar.broker.transaction.buffer.utils;
+
+import lombok.Setter;
+import org.apache.bookkeeper.mledger.Position;
+import org.apache.pulsar.broker.service.persistent.PersistentTopic;
+import org.apache.pulsar.broker.transaction.buffer.impl.TopicTransactionBuffer;
+
+import java.util.concurrent.CompletableFuture;
+
+public class TransactionBufferTestImpl extends TopicTransactionBuffer {
+@Setter
+public CompletableFuture transactionBufferFuture = null;
+@Setter
+public State state = null;
+@Setter
+public CompletableFuture publishFuture = null;
+
+public TransactionBufferTestImpl(PersistentTopic topic) {
+super(topic);
+}
+
+@Override
+public CompletableFuture getTransactionBufferFuture() {
Review Comment:
You can use `Mockito.spy` to customize the behavior of `getXxx()` methods.
--
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
[PR] [improve][broker]Reuse method getAvailableBrokersAsync [pulsar]
crossoverJie opened a new pull request, #23099: URL: https://github.com/apache/pulsar/pull/23099 ### Modifications ModularLoadManagerImpl resuse method getAvailableBrokersAsync ### Verifying this change - [x] 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: *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 - [ ] `doc` - [ ] `doc-required` - [x] `doc-not-needed` - [ ] `doc-complete` ### Matching PR in forked repository PR in forked repository: -- 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] Remove blocking calls from BookieRackAffinityMapping [pulsar]
AlexStocks commented on PR #22846: URL: https://github.com/apache/pulsar/pull/22846#issuecomment-2257374380 related stack [1267792802456608768.md](https://github.com/user-attachments/files/16421530/1267792802456608768.md) -- 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] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
iosdev747 commented on PR #23094: URL: https://github.com/apache/pulsar/pull/23094#issuecomment-2256705908 Hey @lhotari, while going through the tests, I realized that it will need more changes to add this flag in the admin client. Is it better to add new methods in `org.apache.pulsar.client.admin.Clusters` to keep things backward compatible? Or modify existing methods for cleaner 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] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
iosdev747 commented on code in PR #23094:
URL: https://github.com/apache/pulsar/pull/23094#discussion_r1695702384
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##
@@ -771,8 +782,13 @@ private CompletableFuture
filterAndUnloadMatchedNamespaceAsync(NamespaceIs
.map(tenant ->
adminClient.namespaces().getNamespacesAsync(tenant));
return FutureUtil.waitForAll(completableFutureStream)
.thenApply(namespaces -> {
-// if namespace match any policy regex, add it
to ns list to be unload.
+// Filter namespaces that have current cluster
in their replication_clusters
+// if namespace match any policy regex, add it
to ns list to be unloaded.
return namespaces.stream()
+.filter(namespaceName ->
adminClient.namespaces()
+
.getPoliciesAsync(namespaceName)
+.thenApply(policies ->
policies.replication_clusters.contains(cluster))
+.join())
Review Comment:
For the filter to work, I need to call join to wait for the future
completion. The reason for doing it this way is that policy is required to
decide whether to remove the namespace or not. Can you suggest how I can make
it async? Nothing better comes to my mind that doesn't need too much code
refactoring...
--
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
[PR] [fix][build] Generate Sources and Update Folders For All Projects Error in IDEA [pulsar]
four1er opened a new pull request, #23097: URL: https://github.com/apache/pulsar/pull/23097 Fixes Build in IDEA Main Issue: PIP: ### Motivation ### Modifications ### Verifying this change - [x] 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. ### Does this pull request potentially affect one of the following parts: *If the box was checked, please highlight the changes* - [x] **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 - [ ] `doc` - [ ] `doc-required` - [x] `doc-not-needed` - [ ] `doc-complete` ### Matching PR in forked repository PR in forked repository: -- 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
[I] [Bug] org.apache.pulsar:pulsar-package-bookkeeper-storage:jar:3.4.0-SNAPSHOT was not found [pulsar]
four1er opened a new issue, #23096:
URL: https://github.com/apache/pulsar/issues/23096
### Search before asking
- [X] I searched in the [issues](https://github.com/apache/pulsar/issues)
and found nothing similar.
### Read release policy
- [X] I understand that unsupported versions don't get bug fixes. I will
attempt to reproduce the issue on a supported version of Pulsar client and
Pulsar broker.
### Version
OS: Mac OS 14.5 M1 Pro
Java version: jdk-17.0.4.1
Pulsar version: master(47d35a0af8ef72062288866f0c875312b5684906)
### Minimal reproduce step
Click the "Generate Sources and Update Folders For All Projects" button in
the Maven UI toolbar.
https://github.com/user-attachments/assets/f332d58e-3126-4025-83b2-6e3979520098;>
### What did you expect to see?
Generate Source success.
https://github.com/user-attachments/assets/b00b16a3-9b1d-47f1-9407-ca1ecbd51b08;>
### What did you see instead?
org.apache.pulsar:pulsar-package-bookkeeper-storage:jar:3.4.0-SNAPSHOT was
not found.
### Anything else?
This is because the pom.xml use 3.4.0 as the dependency version, but the
newest version is 2.10.7.2-SNAPSHOT-86382c9.
```
org.apache.pulsar
pulsar-package-bookkeeper-storage
${project.version}
```
### Are you willing to submit a PR?
- [X] I'm willing to submit a 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.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
lhotari commented on code in PR #23094:
URL: https://github.com/apache/pulsar/pull/23094#discussion_r1695596846
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##
@@ -771,8 +782,13 @@ private CompletableFuture
filterAndUnloadMatchedNamespaceAsync(NamespaceIs
.map(tenant ->
adminClient.namespaces().getNamespacesAsync(tenant));
return FutureUtil.waitForAll(completableFutureStream)
.thenApply(namespaces -> {
-// if namespace match any policy regex, add it
to ns list to be unload.
+// Filter namespaces that have current cluster
in their replication_clusters
+// if namespace match any policy regex, add it
to ns list to be unloaded.
return namespaces.stream()
+.filter(namespaceName ->
adminClient.namespaces()
+
.getPoliciesAsync(namespaceName)
+.thenApply(policies ->
policies.replication_clusters.contains(cluster))
+.join())
Review Comment:
this would be a blocking operation. it would be better to make it
asynchronous.
--
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
[PR] Remove PersistentSubscription.getStats [pulsar]
nodece opened a new pull request, #23095: URL: https://github.com/apache/pulsar/pull/23095 ### Motivation Clean up `org.apache.pulsar.broker.service.persistent.PersistentSubscription#getStats` method. ### Modifications - org.apache.pulsar.broker.service.persistent.PersistentTopic#removeSubscription returns `CompletableFuture` - Remove `PersistentSubscription#getStats` method and call ### Documentation - [ ] `doc` - [ ] `doc-required` - [x] `doc-not-needed` - [ ] `doc-complete` -- 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: [I] [Security] 3.3.0 has several fixable security vulnerabilities. [pulsar]
lhotari commented on issue #23083: URL: https://github.com/apache/pulsar/issues/23083#issuecomment-2256479938 The release vote is out for 3.3.1 release: https://lists.apache.org/thread/6o1d37jx8ztcs54d8x120gl4491n4502 . Please help validate the release candidate! @adrian-tarau Does it fix the security vulnerabilities that you have detected? -- 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: [I] [Bug] Checkstyle check is not applied to all tests [pulsar]
four1er commented on issue #23079: URL: https://github.com/apache/pulsar/issues/23079#issuecomment-2256446729 I want to solve it, pls assign it to me, thx! -- 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
[PR] [fix] [broker] fix how ns-isolation-policy API works for replicated namespaces [pulsar]
iosdev747 opened a new pull request, #23094: URL: https://github.com/apache/pulsar/pull/23094 Fixes #23092 ### Motivation 1. Problem is namespace unload operation happens on all the namespaces that matches the policy regex. Check should be done only on namespaces in current cluster. 2. When there are too many namespaces in a cluster and adding a single namespace to the ns isolation policy data leads to too many bundle unload calls. This can exhaust broker resources (open connections). This should be controlled by a flag if bundle unload can be done post setPolicy call. ### Modifications 1. Fix namespace filter so only namespaces in current cluster should be used for unload operation. 2. Adding a unload flag in set namespaceIsolationPolicies API so that applying a isolation policy on live cluster doesn't lead to too many unload calls. ### Verifying this change - [ ] Make sure that the change passes the CI checks. *(Please pick either of the following options)* This change added tests and can be verified as follows: - *Multi cluster setup where namespace is not present in one cluster but is there in other.* ### Does this pull request potentially affect one of the following parts: *If the box was checked, please highlight the changes* - [ ] Dependencies (add or upgrade a dependency) - [x] The public API - [ ] The schema - [ ] The default values of configurations - [ ] The threading model - [ ] The binary protocol - [ ] The REST endpoints - [x] The admin CLI options - [ ] The metrics - [ ] Anything that affects deployment ### Documentation - [ ] `doc` - [x] `doc-required` - [ ] `doc-not-needed` - [ ] `doc-complete` ### Matching PR in forked repository PR in forked repository: https://github.com/iosdev747/pulsar/pull/1 -- 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] [improve][build] Move docker-push profile to submodule [pulsar]
nodece merged PR #23093: URL: https://github.com/apache/pulsar/pull/23093 -- 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] [improve][build] Move docker-push profile to submodule [pulsar]
codecov-commenter commented on PR #23093: URL: https://github.com/apache/pulsar/pull/23093#issuecomment-2256386735 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23093?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 73.44%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`6022c9e`)](https://app.codecov.io/gh/apache/pulsar/commit/6022c9e8bddf94fa5243caa8f6d4a8d83b707736?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 479 commits behind head on master. Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/23093?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) ```diff @@ Coverage Diff @@ ## master #23093 +/- ## - Coverage 73.57% 73.44% -0.14% - Complexity3262433524 +900 Files 1877 1919 +42 Lines139502 144087+4585 Branches 1529915745 +446 + Hits 102638 105824+3186 - Misses2890830145+1237 - Partials 7956 8118 +162 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23093/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23093/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `27.58% <ø> (+2.99%)` | :arrow_up: | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23093/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `24.76% <ø> (+0.43%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23093/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `72.51% <ø> (-0.34%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [see 516 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23093/indirect-changes?src=pr=tree-more_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) -- 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] [broker] local metadata sync topic contains configuration events causing all operations stuck [pulsar]
poorbarcode commented on PR #22695: URL: https://github.com/apache/pulsar/pull/22695#issuecomment-2256358780 @lhotari > @poorbarcode please resolve the merge conflict Done -- 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] [broker] local metadata sync topic contains configuration events causing all operations stuck [pulsar]
lhotari commented on PR #22695: URL: https://github.com/apache/pulsar/pull/22695#issuecomment-2256330984 @poorbarcode please resolve the merge conflict -- 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] [improve][build] Upgrade docker-maven-plugin to 0.45.0 [pulsar]
nodece commented on PR #23091: URL: https://github.com/apache/pulsar/pull/23091#issuecomment-2256205907 @lhotari https://github.com/apache/pulsar/pull/23093 will fix that. -- 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] [improve][build] Move docker-push profile to submodule [pulsar]
nodece closed pull request #23093: [improve][build] Move docker-push profile to submodule URL: https://github.com/apache/pulsar/pull/23093 -- 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] [broker] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]
Technoboy- merged PR #23018: URL: https://github.com/apache/pulsar/pull/23018 -- 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] [improve][build] Upgrade docker-maven-plugin to 0.45.0 [pulsar]
lhotari commented on PR #23091: URL: https://github.com/apache/pulsar/pull/23091#issuecomment-2256167768 > If we push the image in the docker/pulsar or docker/pulsar-all directory, this PR will break the release process, otherwise, it works fine in the docker directory. > > Usually, we will push images in the docker directory, what do you think? well, the release process uses `-pl docker/pulsar,docker/pulsar-all`. It will break. Many others might have copied the similar command that is used in https://pulsar.apache.org/contribute/release-process/#release-pulsar-30-and-later . -- 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] [improve][build] Upgrade docker-maven-plugin to 0.45.0 [pulsar]
nodece commented on PR #23091: URL: https://github.com/apache/pulsar/pull/23091#issuecomment-2256133414 If we push the image in the docker/pulsar or docker/pulsar-all directory, this PR will break the release process, otherwise, it works fine in the docker directory. Usually, we will push images in the docker directory, what do you think? -- 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] [improve][broker] Support to specify auth-plugin, auth-parameters and tls-enable arguments when init cluster metadata [pulsar]
Demogorgon314 merged PR #23087: URL: https://github.com/apache/pulsar/pull/23087 -- 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] [improve][build] Upgrade docker-maven-plugin to 0.45.0 [pulsar]
lhotari commented on PR #23091: URL: https://github.com/apache/pulsar/pull/23091#issuecomment-2256090707 @nodece One of the problems with the profile refactoring is that it breaks the current release process documented at https://pulsar.apache.org/contribute/release-process/#release-pulsar-30-and-later. I think that it would be better to revert the changes related to moving the profile to docker/pom.xml since many might have internal releases that break as a result. -- 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][client] TransactionCoordinatorClient support retry [pulsar]
codecov-commenter commented on PR #23081: URL: https://github.com/apache/pulsar/pull/23081#issuecomment-2256085159 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23081?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `31.25000%` with `11 lines` in your changes missing coverage. Please review. > Project coverage is 73.41%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`c6b3c47`)](https://app.codecov.io/gh/apache/pulsar/commit/c6b3c4740f64e009b2f90a7e0d5e246375c625a3?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 477 commits behind head on master. Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/23081?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) ```diff @@ Coverage Diff @@ ## master #23081 +/- ## - Coverage 73.57% 73.41% -0.16% - Complexity3262433206 +582 Files 1877 1917 +40 Lines139502 144077+4575 Branches 1529915744 +445 + Hits 102638 105774+3136 - Misses2890830179+1271 - Partials 7956 8124 +168 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23081/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23081/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `27.44% <25.00%> (+2.86%)` | :arrow_up: | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23081/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `24.78% <0.00%> (+0.45%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23081/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `72.47% <31.25%> (-0.37%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/apache/pulsar/pull/23081?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [.../transaction/TransactionCoordinatorClientImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23081?src=pr=tree=pulsar-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fimpl%2Ftransaction%2FTransactionCoordinatorClientImpl.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL3RyYW5zYWN0aW9uL1RyYW5zYWN0aW9uQ29vcmRpbmF0b3JDbGllbnRJbXBsLmphdmE=) | `66.07% <100.00%> (+0.30%)` | :arrow_up: | | [...ulsar/client/impl/TransactionMetaStoreHandler.java](https://app.codecov.io/gh/apache/pulsar/pull/23081?src=pr=tree=pulsar-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fimpl%2FTransactionMetaStoreHandler.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1RyYW5zYWN0aW9uTWV0YVN0b3JlSGFuZGxlci5qYXZh) | `67.04% <21.42%> (-0.86%)` | :arrow_down: | ... and [510 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23081/indirect-changes?src=pr=tree-more_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) -- 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] [improve][broker] Optimize the performance of individual acknowledgments [pulsar]
shibd merged PR #23072: URL: https://github.com/apache/pulsar/pull/23072 -- 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][client] TransactionCoordinatorClient support retry [pulsar]
chenhongSZ commented on PR #23081: URL: https://github.com/apache/pulsar/pull/23081#issuecomment-2256014580 /pulsarbot run-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] [improve][broker] Reduce the CPU pressure from the transaction buffer in rolling restarts [pulsar]
BewareMyPower merged PR #23062: URL: https://github.com/apache/pulsar/pull/23062 -- 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] [improve][broker] Reduce the CPU pressure from the transaction buffer in rolling restarts [pulsar]
BewareMyPower commented on PR #23062: URL: https://github.com/apache/pulsar/pull/23062#issuecomment-2255843175 Merge it first. -- 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][io] Upgrade Debezium connector version to 2.6.1.Final [pulsar]
lhotari commented on code in PR #23078: URL: https://github.com/apache/pulsar/pull/23078#discussion_r1695123309 ## pom.xml: ## @@ -201,7 +201,7 @@ flexible messaging model and an intuitive client API. 2.4.10 1.2.4 8.12.1 -1.9.7.Final +2.6.1.Final Review Comment: ```suggestion 2.7.0.Final ``` -- 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] [improve][broker] Reduce the CPU pressure from the transaction buffer in rolling restarts [pulsar]
codecov-commenter commented on PR #23062: URL: https://github.com/apache/pulsar/pull/23062#issuecomment-2255793312 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23062?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `83.78378%` with `30 lines` in your changes missing coverage. Please review. > Project coverage is 73.47%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`acc95de`)](https://app.codecov.io/gh/apache/pulsar/commit/acc95dedac9aa81a87acf469fa87e17b4f2d2835?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 475 commits behind head on master. Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/23062?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) ```diff @@ Coverage Diff @@ ## master #23062 +/- ## - Coverage 73.57% 73.47% -0.11% - Complexity3262433530 +906 Files 1877 1919 +42 Lines139502 144086+4584 Branches 1529915741 +442 + Hits 102638 105860+3222 - Misses2890830101+1193 - Partials 7956 8125 +169 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23062/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23062/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `27.52% <36.75%> (+2.94%)` | :arrow_up: | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23062/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `24.72% <0.00%> (+0.40%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23062/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `72.54% <83.78%> (-0.31%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/apache/pulsar/pull/23062?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [...n/java/org/apache/pulsar/broker/PulsarService.java](https://app.codecov.io/gh/apache/pulsar/pull/23062?src=pr=tree=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2FPulsarService.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9QdWxzYXJTZXJ2aWNlLmphdmE=) | `84.25% <100.00%> (+1.88%)` | :arrow_up: | | [...r/service/SystemTopicTxnBufferSnapshotService.java](https://app.codecov.io/gh/apache/pulsar/pull/23062?src=pr=tree=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FSystemTopicTxnBufferSnapshotService.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1N5c3RlbVRvcGljVHhuQnVmZmVyU25hcHNob3RTZXJ2aWNlLmphdmE=) | `80.55% <100.00%> (-2.47%)` | :arrow_down: | | [...rvice/TransactionBufferSnapshotServiceFactory.java](https://app.codecov.io/gh/apache/pulsar/pull/23062?src=pr=tree=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FTransactionBufferSnapshotServiceFactory.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1RyYW5zYWN0aW9uQnVmZmVyU25hcHNob3RTZXJ2aWNlRmFjdG9yeS5qYXZh) | `83.33% <100.00%> (ø)` | | |
Re: [PR] [fix][io] Upgrade Debezium connector version to 2.6.1.Final [pulsar]
lhotari commented on code in PR #23078: URL: https://github.com/apache/pulsar/pull/23078#discussion_r1695116720 ## tests/integration/src/test/java/org/apache/pulsar/tests/integration/containers/DebeziumMongoDbContainer.java: ## @@ -25,7 +25,7 @@ public class DebeziumMongoDbContainer extends ChaosContainer 2.4.10 1.2.4 8.12.1 -1.9.7.Final +2.6.1.Final Review Comment: According to https://debezium.io/ , latest stable Debezium release series is 2.7.x . Could we upgrade directly to 2.7.x instead? ## tests/integration/src/test/java/org/apache/pulsar/tests/integration/containers/DebeziumPostgreSqlContainer.java: ## @@ -26,7 +26,7 @@ public class DebeziumPostgreSqlContainer extends ChaosContainer
Re: [PR] [improve][ci] Switch to use DEVELOCITY_ACCESS_KEY from GRADLE_ENTERPRISE_ACCESS_KEY [pulsar]
lhotari merged PR #23090: URL: https://github.com/apache/pulsar/pull/23090 -- 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: [I] Incorrect sequence of messages while reading dataChan after closing [pulsar-client-go]
gunli commented on issue #1257: URL: https://github.com/apache/pulsar-client-go/issues/1257#issuecomment-2255699499 @Gilthoniel Good catch, could pls check if #1249 can fix this? -- 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] [improve][txn] Add admin api getOwnedTransactions [pulsar]
liangyepianzhou commented on PR #22980: URL: https://github.com/apache/pulsar/pull/22980#issuecomment-2255679917 > Thank you for your reply, I have already created a PIP before. > PIP: #22982 > discussion thread: https://lists.apache.org/thread/wc4yw1m5g4zywc3c628p5zz4bdkh5x7x OK, please put the PIP link in the description of the 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
[I] [Bug] Breaking contract b/w 2.9 / 2.10 and 3.0 for ns-isolation-policy [pulsar]
iosdev747 opened a new issue, #23092: URL: https://github.com/apache/pulsar/issues/23092 ### Search before asking - [X] I searched in the [issues](https://github.com/apache/pulsar/issues) and found nothing similar. ### Read release policy - [X] I understand that unsupported versions don't get bug fixes. I will attempt to reproduce the issue on a supported version of Pulsar client and Pulsar broker. ### Version Pulsar: 3.0.5 Admin Client: 3.0.5 ### Minimal reproduce step - 2 clusters (cluster-1, cluster-2) - Tenant present in all clusters. (tenant1) - Namespace (tenant1/ns1) present in only one cluster (cluster-1) - When updating the ns-isolation-policy for cluster-2 with policy data as `tenant1/.*`, `setNamespaceIsolationPolicy()` fails. This serves usecases where other namespaces under this tenant should be a part of certain broker group. This used to work in 2.9.x and 2.10.x. ### What did you expect to see? 200 returned and setNamespaceIsolationPolicy() setting up the ns-isolation-policy correctly. This should not try to unload namespaces which are not present in current cluster. ### What did you see instead? PR that introduced this change: https://github.com/apache/pulsar/pull/15527 [Here](https://github.com/apache/pulsar/blob/fdeeba597d1689f858a0eec072441872ad33c0ed/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java#L737), set policy call tries to unload all matching namespaces (all bundles). There should be a check that namespace should be present in the current cluster. ### Anything else? Apart from the above issue, this way of unloading all the namespace bundles is very impactful (That will definitely lead to some amount of downtime). If some namespaces are already present in the correct broker group (as per ns isolation policy), then those namespace bundles should not be unloaded. For a broker group with many small namespaces, this will lead to timeouts and or even 5xx (when there are too many bundles unloaded). One of such scenario: ``` java.util.concurrent.CompletionException: org.apache.pulsar.client.admin.PulsarAdminException$ServerSideErrorException: --- An unexpected error occurred in the server --- Message: Namespace bundle tenant1/ns1/0x1af286bc_0x21af286b is being unloaded Stacktrace: java.lang.IllegalStateException: Namespace bundle tenant1/ns1/0x1af286bc_0x21af286b is being unloaded at org.apache.pulsar.broker.namespace.NamespaceService.lambda$findBrokerServiceUrl$11(NamespaceService.java:463) at java.base/java.util.concurrent.CompletableFuture.uniAcceptNow(CompletableFuture.java:757) at java.base/java.util.concurrent.CompletableFuture.uniAcceptStage(CompletableFuture.java:735) at java.base/java.util.concurrent.CompletableFuture.thenAccept(CompletableFuture.java:2182) at org.apache.pulsar.broker.namespace.NamespaceService.lambda$findBrokerServiceUrl$15(NamespaceService.java:448) at org.apache.pulsar.common.util.collections.ConcurrentOpenHashMap$Section.put(ConcurrentOpenHashMap.java:418) at org.apache.pulsar.common.util.collections.ConcurrentOpenHashMap.computeIfAbsent(ConcurrentOpenHashMap.java:243) at org.apache.pulsar.broker.namespace.NamespaceService.findBrokerServiceUrl(NamespaceService.java:444) at org.apache.pulsar.broker.namespace.NamespaceService.lambda$internalGetWebServiceUrl$9(NamespaceService.java:314) at java.base/java.util.concurrent.CompletableFuture.uniComposeStage(CompletableFuture.java:1187) at java.base/java.util.concurrent.CompletableFuture.thenCompose(CompletableFuture.java:2309) at org.apache.pulsar.broker.namespace.NamespaceService.internalGetWebServiceUrl(NamespaceService.java:296) at org.apache.pulsar.broker.namespace.NamespaceService.getWebServiceUrlAsync(NamespaceService.java:277) at org.apache.pulsar.broker.web.PulsarWebResource.isBundleOwnedByAnyBroker(PulsarWebResource.java:623) at org.apache.pulsar.broker.admin.impl.NamespacesBase.lambda$internalUnloadNamespaceBundleAsync$125(NamespacesBase.java:1052) at java.base/java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:1150) at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510) at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2147) at org.apache.pulsar.metadata.impl.ZKMetadataStore.lambda$existsFromStore$11(ZKMetadataStore.java:362) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
Re: [PR] [improve][txn] Add admin api getOwnedTransactions [pulsar]
hr commented on PR #22980: URL: https://github.com/apache/pulsar/pull/22980#issuecomment-2255641697 > @hr Adding a user-facing interface is a breaking change and requires a PIP. Do you mind to write a PIP about this? You can write the PIP following [this](https://pulsar.apache.org/contribute/#pulsar-improvement-proposal-pip). Once you have finished the PIP, we would be like to review it. Thank you for your reply, I have already created a PIP before. PIP: https://github.com/apache/pulsar/pull/22982 discussion thread: https://lists.apache.org/thread/wc4yw1m5g4zywc3c628p5zz4bdkh5x7x -- 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] [improve][build] Upgrade docker-maven-plugin to 0.45.0 [pulsar]
lhotari merged PR #23091: URL: https://github.com/apache/pulsar/pull/23091 -- 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] [improve] [broker] Check max producers/consumers limitation first before other ops to save resources [pulsar]
poorbarcode merged PR #23074: URL: https://github.com/apache/pulsar/pull/23074 -- 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] [broker] Remove blocking calls from Subscription.getStats [pulsar]
nodece merged PR #23088: URL: https://github.com/apache/pulsar/pull/23088 -- 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][build] Upgrade docker-maven-plugin to 0.43.4 [pulsar]
nodece closed pull request #22609: [fix][build] Upgrade docker-maven-plugin to 0.43.4 URL: https://github.com/apache/pulsar/pull/22609 -- 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] [improve][build] Upgrade docker-maven-plugin to 0.45.0 [pulsar]
nodece commented on PR #23091: URL: https://github.com/apache/pulsar/pull/23091#issuecomment-2255600550 @lhotari Thanks, Done. -- 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] [improve][build] Remove tag goal from the docker-maven-plugin [pulsar]
nodece closed pull request #22754: [improve][build] Remove tag goal from the docker-maven-plugin URL: https://github.com/apache/pulsar/pull/22754 -- 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] [improve] [broker] Check max producers/consumers limitation first before other ops to save resources [pulsar]
codecov-commenter commented on PR #23074: URL: https://github.com/apache/pulsar/pull/23074#issuecomment-2255572263 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23074?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `92.85714%` with `1 line` in your changes missing coverage. Please review. > Project coverage is 73.45%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`a20ff14`)](https://app.codecov.io/gh/apache/pulsar/commit/a20ff1cc16cd013ed0a5b37d6524b3af611d?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 471 commits behind head on master. Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/23074?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) ```diff @@ Coverage Diff @@ ## master #23074 +/- ## - Coverage 73.57% 73.45% -0.13% - Complexity3262433212 +588 Files 1877 1917 +40 Lines139502 144077+4575 Branches 1529915741 +442 + Hits 102638 105828+3190 - Misses2890830133+1225 - Partials 7956 8116 +160 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23074/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23074/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `27.55% <28.57%> (+2.97%)` | :arrow_up: | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23074/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `24.76% <21.42%> (+0.43%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23074/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `72.50% <92.85%> (-0.35%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/apache/pulsar/pull/23074?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [...va/org/apache/pulsar/broker/service/ServerCnx.java](https://app.codecov.io/gh/apache/pulsar/pull/23074?src=pr=tree=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FServerCnx.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1NlcnZlckNueC5qYXZh) | `72.28% <100.00%> (+0.13%)` | :arrow_up: | | [...rg/apache/pulsar/broker/service/AbstractTopic.java](https://app.codecov.io/gh/apache/pulsar/pull/23074?src=pr=tree=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FAbstractTopic.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0VG9waWMuamF2YQ==) | `88.30% <80.00%> (+0.31%)` | :arrow_up: | ... and [509 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23074/indirect-changes?src=pr=tree-more_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) -- 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] [broker] Fix wrong logic of method TopicName.getPartition(int index) [pulsar]
lhotari commented on PR #19841: URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255560317 > @lhotari > > > > This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed > > > @poorbarcode In which case would this apply? > > See the section 4 in the Motivation @poorbarcode Since #22705 has been cherry-picked to branch-3.0, does it prevent the problem from occuring? -- 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] [improve][build] Upgrade docker-maven-plugin to 0.45.0 [pulsar]
codecov-commenter commented on PR #23091: URL: https://github.com/apache/pulsar/pull/23091#issuecomment-2255547264 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23091?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 73.45%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`202a701`)](https://app.codecov.io/gh/apache/pulsar/commit/202a701c84e046952096bb7849cd644007ede025?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 471 commits behind head on master. Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/23091?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) ```diff @@ Coverage Diff @@ ## master #23091 +/- ## - Coverage 73.57% 73.45% -0.13% - Complexity3262433511 +887 Files 1877 1917 +40 Lines139502 144067+4565 Branches 1529915741 +442 + Hits 102638 105818+3180 - Misses2890830125+1217 - Partials 7956 8124 +168 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23091/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23091/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `27.51% <ø> (+2.92%)` | :arrow_up: | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23091/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `24.80% <ø> (+0.47%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23091/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `72.51% <ø> (-0.33%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [see 513 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23091/indirect-changes?src=pr=tree-more_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) -- 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] [improve][build] Upgrade docker-maven-plugin to 0.45.0 [pulsar]
lhotari commented on PR #23091: URL: https://github.com/apache/pulsar/pull/23091#issuecomment-2255536372 @nodece does this make #22754 and #22609 obsolete? Please close them in that case. I forgot the reason why the tag goal is removed. please add a description. -- 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] [improve][broker] Optimize the performance of individual acknowledgments [pulsar]
gaoran10 commented on code in PR #23072:
URL: https://github.com/apache/pulsar/pull/23072#discussion_r1694963654
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##
@@ -531,14 +532,15 @@ public CompletableFuture messageAcked(CommandAck
ack) {
//this method is for individual ack not carry the transaction
private CompletableFuture individualAckNormal(CommandAck ack,
Map properties) {
-List positionsAcked = new ArrayList<>();
+List> positionsAcked = new ArrayList<>();
long totalAckCount = 0;
for (int i = 0; i < ack.getMessageIdsCount(); i++) {
MessageIdData msgId = ack.getMessageIdAt(i);
Position position;
-long ackedCount = 0;
-long batchSize = getBatchSize(msgId);
-Consumer ackOwnerConsumer =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
+Pair ackOwnerConsumerPair =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
Review Comment:
ok
--
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] [broker] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]
poorbarcode commented on code in PR #23018:
URL: https://github.com/apache/pulsar/pull/23018#discussion_r1694961292
##
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##
@@ -498,7 +498,7 @@ protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
// Clear cache after session expired.
-if (event == SessionEvent.SessionLost || event ==
SessionEvent.ConnectionLost) {
+if (event == SessionEvent.SessionReestablished) {
Review Comment:
Sure, 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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [improve][broker] Optimize the performance of individual acknowledgments [pulsar]
shibd commented on code in PR #23072:
URL: https://github.com/apache/pulsar/pull/23072#discussion_r1694947373
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##
@@ -531,14 +532,15 @@ public CompletableFuture messageAcked(CommandAck
ack) {
//this method is for individual ack not carry the transaction
private CompletableFuture individualAckNormal(CommandAck ack,
Map properties) {
-List positionsAcked = new ArrayList<>();
+List> positionsAcked = new ArrayList<>();
long totalAckCount = 0;
for (int i = 0; i < ack.getMessageIdsCount(); i++) {
MessageIdData msgId = ack.getMessageIdAt(i);
Position position;
-long ackedCount = 0;
-long batchSize = getBatchSize(msgId);
-Consumer ackOwnerConsumer =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
+Pair ackOwnerConsumerPair =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
Review Comment:
Actually, I threw an exception at first code, however, it caused the test to
fail.
https://github.com/apache/pulsar/blob/51202a6889582117bf790e9ff2325b9f3119510f/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BatchMessageTest.java#L279
So, I kept the same behavior as the current logic.
I think we can discuss the behavior here in detail later and optimize it. In
short, that's not the point of 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] [broker] Fix wrong logic of method TopicName.getPartition(int index) [pulsar]
poorbarcode commented on PR #19841: URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255518969 @lhotari >> This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed > @poorbarcode In which case would this apply? See the section 4 in the Motivation https://github.com/user-attachments/assets/4d20a0cf-2854-4440-9b47-45ae420d5c81;> -- 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] [improve][broker] Optimize the performance of individual acknowledgments [pulsar]
shibd commented on code in PR #23072:
URL: https://github.com/apache/pulsar/pull/23072#discussion_r1694947373
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##
@@ -531,14 +532,15 @@ public CompletableFuture messageAcked(CommandAck
ack) {
//this method is for individual ack not carry the transaction
private CompletableFuture individualAckNormal(CommandAck ack,
Map properties) {
-List positionsAcked = new ArrayList<>();
+List> positionsAcked = new ArrayList<>();
long totalAckCount = 0;
for (int i = 0; i < ack.getMessageIdsCount(); i++) {
MessageIdData msgId = ack.getMessageIdAt(i);
Position position;
-long ackedCount = 0;
-long batchSize = getBatchSize(msgId);
-Consumer ackOwnerConsumer =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
+Pair ackOwnerConsumerPair =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
Review Comment:
Actually, I threw an exception at first code, however, it caused the test to
fail.
https://github.com/apache/pulsar/blob/411f6973e85b0a6213e992386e1704f93d0aae42/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BatchMessageWithBatchIndexLevelTest.java#L227
It was introduced by #17003
So, I kept the same behavior as the current logic.
I think we can discuss the behavior here in detail later and optimize it. In
short, that's not the point of 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] [broker] Fix wrong logic of method TopicName.getPartition(int index) [pulsar]
lhotari commented on PR #19841: URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255509613 > This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed @poorbarcode In which case would this apply? -- 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] [improve][broker] Optimize the performance of individual acknowledgments [pulsar]
gaoran10 commented on code in PR #23072:
URL: https://github.com/apache/pulsar/pull/23072#discussion_r1694935489
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##
@@ -531,14 +532,15 @@ public CompletableFuture messageAcked(CommandAck
ack) {
//this method is for individual ack not carry the transaction
private CompletableFuture individualAckNormal(CommandAck ack,
Map properties) {
-List positionsAcked = new ArrayList<>();
+List> positionsAcked = new ArrayList<>();
long totalAckCount = 0;
for (int i = 0; i < ack.getMessageIdsCount(); i++) {
MessageIdData msgId = ack.getMessageIdAt(i);
Position position;
-long ackedCount = 0;
-long batchSize = getBatchSize(msgId);
-Consumer ackOwnerConsumer =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
+Pair ackOwnerConsumerPair =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
Review Comment:
Maybe we shouldn't use `this` as the owner broker if we can't find the ack
position in the consumer pending ack cache. For example:
1. Someone didn't consume messages and ack a position directly.
2. The broker restart.
--
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] [improve][broker] Optimize the performance of individual acknowledgments [pulsar]
shibd commented on code in PR #23072:
URL: https://github.com/apache/pulsar/pull/23072#discussion_r1694934170
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##
@@ -531,14 +532,15 @@ public CompletableFuture messageAcked(CommandAck
ack) {
//this method is for individual ack not carry the transaction
private CompletableFuture individualAckNormal(CommandAck ack,
Map properties) {
-List positionsAcked = new ArrayList<>();
+List> positionsAcked = new ArrayList<>();
long totalAckCount = 0;
for (int i = 0; i < ack.getMessageIdsCount(); i++) {
MessageIdData msgId = ack.getMessageIdAt(i);
Position position;
-long ackedCount = 0;
-long batchSize = getBatchSize(msgId);
-Consumer ackOwnerConsumer =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
+Pair ackOwnerConsumerPair =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
Review Comment:
Thanks for review, Done.
--
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] [broker] Fix wrong logic of method TopicName.getPartition(int index) [pulsar]
poorbarcode commented on PR #19841: URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255484816 > @poorbarcode The test PartitionKeywordCompatibilityTest is broken in branch-3.0 . Cherry-picking this PR fixes the issue. Please let me know if you have concerns about cherry-picking this PR to branch-3.0. https://github.com/apache/pulsar/pull/19841/files#diff-93feae220f18ea80cb01ec7f2cefeab410aa0f3f181eed1206da4a294db0f701R233-R234 This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed -- 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] [broker] [branch-3.0] Fast fix infinite HTTP call createSubscriptions caused by wrong topicName [pulsar]
lhotari commented on PR #21997: URL: https://github.com/apache/pulsar/pull/21997#issuecomment-2255462725 > The master branch has fixed the issue by #19841 Since it will makes users can not receive the messages which created in mistake, we did not cherry-pick #19841 into other branches, see detail #19841) @poorbarcode I didn't find the reason why #19841 wasn't cherry-picked to branch-3.0 . Please provide the details here. Thanks. -- 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] [broker] Fix wrong logic of method TopicName.getPartition(int index) [pulsar]
lhotari commented on PR #19841: URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255454810 @poorbarcode The test PartitionKeywordCompatibilityTest is broken in branch-3.0 . Cherry-picking this PR fixes the issue. Please let me know if you have concerns about cherry-picking this PR to branch-3.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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [broker] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]
lhotari commented on code in PR #23018:
URL: https://github.com/apache/pulsar/pull/23018#discussion_r1694796159
##
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##
@@ -498,7 +498,7 @@ protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
// Clear cache after session expired.
-if (event == SessionEvent.SessionLost || event ==
SessionEvent.ConnectionLost) {
+if (event == SessionEvent.SessionReestablished) {
Review Comment:
please also add `SessionEvent.Reconnected`
--
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] [broker] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]
poorbarcode commented on code in PR #23018:
URL: https://github.com/apache/pulsar/pull/23018#discussion_r1694793372
##
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer
listener) {
protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
+
+// Clear cache after session expired.
Review Comment:
@lhotari @merlimat
Sorry, I missunderstood before, it has been fixed now
--
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] [broker] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]
lhotari commented on code in PR #23018:
URL: https://github.com/apache/pulsar/pull/23018#discussion_r1694783563
##
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer
listener) {
protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
+
+// Clear cache after session expired.
Review Comment:
> Good pointer, it has been fixed. Thanks
@poorbarcode how was @merlimat's comment addressed? It might be better to
postpone the flushing of caches until reconnection has finished.
--
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] [improve][txn] Add admin api getOwnedTransactions [pulsar]
liangyepianzhou commented on PR #22980: URL: https://github.com/apache/pulsar/pull/22980#issuecomment-2255260596 @hr Adding a user-facing interface is a breaking change and requires a PIP. Do you mind to write a PIP about this? You can write the PIP following [this](https://pulsar.apache.org/contribute/#pulsar-improvement-proposal-pip). Once you have finished the PIP, we would be like to review 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] [improve][pip] Propose a Contributor Repository for Pulsar [pulsar]
liangyepianzhou commented on PR #23061: URL: https://github.com/apache/pulsar/pull/23061#issuecomment-2255242198 @dave2wave @eolivelli Enrico and I had some discussions in emails. These discussions led me to refine my ideas and create a prototype of the project in the new repository. [0] This project prototype will show what we want to do better than the documentation. [0] - https://github.com/StevenLuMT/pulsar-java-contrib -- 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] [improve][broker] Optimize the performance of individual acknowledgments [pulsar]
lhotari commented on code in PR #23072:
URL: https://github.com/apache/pulsar/pull/23072#discussion_r1694749342
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##
@@ -531,14 +532,15 @@ public CompletableFuture messageAcked(CommandAck
ack) {
//this method is for individual ack not carry the transaction
private CompletableFuture individualAckNormal(CommandAck ack,
Map properties) {
-List positionsAcked = new ArrayList<>();
+List> positionsAcked = new ArrayList<>();
long totalAckCount = 0;
for (int i = 0; i < ack.getMessageIdsCount(); i++) {
MessageIdData msgId = ack.getMessageIdAt(i);
Position position;
-long ackedCount = 0;
-long batchSize = getBatchSize(msgId);
-Consumer ackOwnerConsumer =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
+Pair ackOwnerConsumerPair =
getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());
Review Comment:
It's better to rename the method from `getAckOwnerConsumer` to
`getAckOwnerConsumerAndBatchSize`. Similarly, rename `ackOwnerConsumer` to
`ackOwnerConsumerAndBatchSize` so that it's possible to understand what the
`Pair` is holding.
--
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] [improve][txn] Add admin api getOwnedTransactions [pulsar]
poorbarcode commented on PR #22980: URL: https://github.com/apache/pulsar/pull/22980#issuecomment-2255235049 @liangyepianzhou @hr Does it relates a PIP? -- 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] [broker] Remove blocking calls from Subscription.getStats [pulsar]
poorbarcode commented on code in PR #23088:
URL: https://github.com/apache/pulsar/pull/23088#discussion_r1694746685
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##
@@ -1201,6 +1203,21 @@ public long estimateBacklogSize() {
}
public SubscriptionStatsImpl getStats(GetStatsOptions getStatsOptions) {
+// So far, there is no case hits this check.
+if (getStatsOptions.isGetEarliestTimeInBacklog()) {
+throw new IllegalArgumentException("Calling the sync method
subscription.getStats with"
Review Comment:
> @Deprecated has been added, why add this check, we will clean up this
method in the future.
> IDE can help us to check the @Depercated.
It prevents PR containing incorrect usage from being merged without adequate
review, so it is needed.
--
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] [broker] Remove blocking calls from Subscription.getStats [pulsar]
poorbarcode commented on code in PR #23088:
URL: https://github.com/apache/pulsar/pull/23088#discussion_r1694743828
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##
@@ -1201,6 +1203,21 @@ public long estimateBacklogSize() {
}
public SubscriptionStatsImpl getStats(GetStatsOptions getStatsOptions) {
+// So far, there is no case hits this check.
+if (getStatsOptions.isGetEarliestTimeInBacklog()) {
+throw new IllegalArgumentException("Calling the sync method
subscription.getStats with"
++ " getEarliestTimeInBacklog, it may encountered a
deadlock error.");
+}
+// The method "getStatsAsync" will be a sync method if the param
"isGetEarliestTimeInBacklog" is false.
+try {
+return getStatsAsync(getStatsOptions).get(5, TimeUnit.SECONDS);
Review Comment:
> My point is that `get(5, TimeUnit.SECONDS)` doesn't make sense.
This is defensive programming, which can be removed, but it is better than
removing it. After we modified the method `persistentTopic.removeSubscription`,
we can remove the method `Subscription.getStat`
--
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][client] TransactionCoordinatorClient support retry [pulsar]
chenhongSZ commented on PR #23081: URL: https://github.com/apache/pulsar/pull/23081#issuecomment-2255215856 > Great work @chenhongSZ! Would it be possible to add a test case? This pr contains correlation test cases in `TransactionCoordinatorClientTest.java` -- 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][client] TransactionCoordinatorClient support retry [pulsar]
lhotari commented on PR #23081: URL: https://github.com/apache/pulsar/pull/23081#issuecomment-2255200463 Great work @chenhongSZ! Would it be possible to add a test case? -- 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][client] Fix negative acknowledgement by messageId [pulsar]
lhotari merged PR #23060: URL: https://github.com/apache/pulsar/pull/23060 -- 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][client] Fix negative acknowledgement by messageId [pulsar]
lhotari commented on PR #23060: URL: https://github.com/apache/pulsar/pull/23060#issuecomment-2255155903 Great catch @izumo27 ! > This will cause both negative ack redeliver and ack timeout redeliver. Would it be possible to add a test case for this scenario? -- 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] [improve][broker] Support to specify auth-plugin, auth-parameters and tls-enable arguments when init cluster metadata [pulsar]
Technoboy- closed pull request #23087: [improve][broker] Support to specify auth-plugin, auth-parameters and tls-enable arguments when init cluster metadata URL: https://github.com/apache/pulsar/pull/23087 -- 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] [broker] Remove blocking calls from Subscription.getStats [pulsar]
nodece commented on code in PR #23088:
URL: https://github.com/apache/pulsar/pull/23088#discussion_r1694711859
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##
@@ -1201,6 +1203,21 @@ public long estimateBacklogSize() {
}
public SubscriptionStatsImpl getStats(GetStatsOptions getStatsOptions) {
+// So far, there is no case hits this check.
+if (getStatsOptions.isGetEarliestTimeInBacklog()) {
+throw new IllegalArgumentException("Calling the sync method
subscription.getStats with"
Review Comment:
`@Deprecated` has been added, why add this check, we will clean up this
method in the future.
IDE can help us to check the `@Depercated`.
--
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] [broker] Remove blocking calls from Subscription.getStats [pulsar]
nodece commented on code in PR #23088:
URL: https://github.com/apache/pulsar/pull/23088#discussion_r1694711859
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##
@@ -1201,6 +1203,21 @@ public long estimateBacklogSize() {
}
public SubscriptionStatsImpl getStats(GetStatsOptions getStatsOptions) {
+// So far, there is no case hits this check.
+if (getStatsOptions.isGetEarliestTimeInBacklog()) {
+throw new IllegalArgumentException("Calling the sync method
subscription.getStats with"
Review Comment:
`@Deprecated` has been added, why add this check, we will clean up this
method in the future.
--
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][broker] Fix MessageDeduplication replay timeout cause topic loading stuck [pulsar]
lhotari commented on PR #23004: URL: https://github.com/apache/pulsar/pull/23004#issuecomment-2255135819 @TakaHiR07 Cherry-picking PR 22860 fixed the issue. Thanks for the guidance. -- 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] [broker] Remove blocking calls from Subscription.getStats [pulsar]
nodece commented on code in PR #23088:
URL: https://github.com/apache/pulsar/pull/23088#discussion_r1694708640
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##
@@ -1201,6 +1203,21 @@ public long estimateBacklogSize() {
}
public SubscriptionStatsImpl getStats(GetStatsOptions getStatsOptions) {
+// So far, there is no case hits this check.
+if (getStatsOptions.isGetEarliestTimeInBacklog()) {
+throw new IllegalArgumentException("Calling the sync method
subscription.getStats with"
++ " getEarliestTimeInBacklog, it may encountered a
deadlock error.");
+}
+// The method "getStatsAsync" will be a sync method if the param
"isGetEarliestTimeInBacklog" is false.
+try {
+return getStatsAsync(getStatsOptions).get(5, TimeUnit.SECONDS);
Review Comment:
My point is that `get(5, TimeUnit.SECONDS)` doesn't make sense.
--
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][broker] Fix MessageDeduplication replay timeout cause topic loading stuck [pulsar]
lhotari commented on PR #23004: URL: https://github.com/apache/pulsar/pull/23004#issuecomment-2255111865 > @lhotari The reason is pr-22860 not in branch-3.2. Do I change the test "testFinishTakeSnapshotWhenTopicLoading" in branch-3.2, or wait until cherry-pick pr-22860? Thanks for checking that @TakaHiR07 . I'll cherry-pick PR 22860 to branch-3.2 and check if things pass after that. -- 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
[PR] [improve][ci] Switch to use DEVELOCITY_ACCESS_KEY from GRADLE_ENTERPRISE_ACCESS_KEY [pulsar]
lhotari opened a new pull request, #23090: URL: https://github.com/apache/pulsar/pull/23090 ### Motivation - fixes deprecation warning: `The deprecated "GRADLE_ENTERPRISE_ACCESS_KEY" environment variable has been replaced by "DEVELOCITY_ACCESS_KEY"` - this is used to access [ASF Gradle Develocity server ge.apache.org](https://ge.apache.org/scans?search.rootProjectNames=Pulsar) ### Modifications - rename environment variable name `GRADLE_ENTERPRISE_ACCESS_KEY` to `DEVELOCITY_ACCESS_KEY` ### Documentation - [ ] `doc` - [ ] `doc-required` - [x] `doc-not-needed` - [ ] `doc-complete` -- 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] [improve] [broker] Check max producers/consumers limitation first before other ops to save resources [pulsar]
Technoboy- commented on code in PR #23074:
URL: https://github.com/apache/pulsar/pull/23074#discussion_r1694691826
##
pulsar-broker/src/test/java/org/apache/pulsar/client/api/MaxProducerTest.java:
##
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.api;
+
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+import java.util.ArrayList;
+import java.util.List;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.BrokerTestUtil;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+@Slf4j
+@Test(groups = "broker-api")
+public class MaxProducerTest extends ProducerConsumerBase {
+
+@BeforeClass(alwaysRun = true)
+@Override
+protected void setup() throws Exception {
+super.internalSetup();
+super.producerBaseSetup();
+}
+
+@AfterClass(alwaysRun = true)
+@Override
+protected void cleanup() throws Exception {
+super.internalCleanup();
+}
+
+@Override
+protected void doInitConf() throws Exception {
+super.doInitConf();
+conf.setMaxProducersPerTopic(2);
+}
+
+@Test
+public void testMaxProducersForBroker() throws Exception {
+testMaxProducers(2);
+}
+
+@Test
+public void testMaxProducersForNamespace() throws Exception {
+// set max clients
+admin.namespaces().setMaxProducersPerTopic("public/default", 3);
+testMaxProducers(3);
+}
+
+private void testMaxProducers(int maxProducerExpected) throws Exception {
+final String topicName =
BrokerTestUtil.newUniqueName("persistent://public/default/tp");
+admin.topics().createNonPartitionedTopic(topicName);
+
+List> producers = new
ArrayList<>();
Review Comment:
need to close these created producers
--
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] [broker] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]
poorbarcode commented on code in PR #23018:
URL: https://github.com/apache/pulsar/pull/23018#discussion_r1694688117
##
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer
listener) {
protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
+
+// Clear cache after session expired.
+if (event == SessionEvent.SessionLost || event ==
SessionEvent.ConnectionLost) {
+for (MetadataCacheImpl metadataCache : metadataCaches) {
+metadataCache.invalidateAll();
+}
+childrenCache.synchronous().invalidateAll();
+existsCache.synchronous().invalidateAll();
Review Comment:
Fixed
--
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] [broker] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]
Technoboy- commented on code in PR #23018:
URL: https://github.com/apache/pulsar/pull/23018#discussion_r1694682995
##
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer
listener) {
protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
+
+// Clear cache after session expired.
+if (event == SessionEvent.SessionLost || event ==
SessionEvent.ConnectionLost) {
+for (MetadataCacheImpl metadataCache : metadataCaches) {
+metadataCache.invalidateAll();
+}
+childrenCache.synchronous().invalidateAll();
+existsCache.synchronous().invalidateAll();
Review Comment:
call `invalidateAll` instead
--
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: [I] [Security] 3.3.0 has several fixable security vulnerabilities. [pulsar]
lhotari commented on issue #23083:
URL: https://github.com/apache/pulsar/issues/23083#issuecomment-2255069534
I'm about to start the release process for Apache Pulsar 3.3.1, so all
already fixed issues will be addressed with this release.
A lot of security vulnerability scanner alerts are false positives or don't
apply to Apache Pulsar. It would be useful to mention what scanner you are
using and what your requirements are ("Several Major issues were reported."
isn't addressable).
Apache Pulsar is maintained by volunteers. Active contributions to Apache
Pulsar are more than welcome for this type of issues. Releases are typically
made every 1-2 months [for actively supported
versions](https://pulsar.apache.org/contribute/release-policy/#supported-versions).
If you need quicker response to security vulnerabilities, I'd recommend
subscribing to a commercial distribution of Apache Pulsar (I work for
StreamNative which offers such services).
--
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] [broker] Remove blocking calls from Subscription.getStats [pulsar]
poorbarcode commented on code in PR #23088:
URL: https://github.com/apache/pulsar/pull/23088#discussion_r1694663444
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##
@@ -1201,6 +1203,21 @@ public long estimateBacklogSize() {
}
public SubscriptionStatsImpl getStats(GetStatsOptions getStatsOptions) {
+// So far, there is no case hits this check.
+if (getStatsOptions.isGetEarliestTimeInBacklog()) {
+throw new IllegalArgumentException("Calling the sync method
subscription.getStats with"
++ " getEarliestTimeInBacklog, it may encountered a
deadlock error.");
+}
+// The method "getStatsAsync" will be a sync method if the param
"isGetEarliestTimeInBacklog" is false.
+try {
+return getStatsAsync(getStatsOptions).get(5, TimeUnit.SECONDS);
Review Comment:
> This thread will be released by
org.apache.pulsar.metadata.impl.ZKSessionWatcher.
It is not related to the thread which working on `ping & pong`, and it will
not trigger a timeout. You can do a test for 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] [broker] Remove blocking calls from Subscription.getStats [pulsar]
nodece commented on code in PR #23088:
URL: https://github.com/apache/pulsar/pull/23088#discussion_r1694659187
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##
@@ -1201,6 +1203,21 @@ public long estimateBacklogSize() {
}
public SubscriptionStatsImpl getStats(GetStatsOptions getStatsOptions) {
+// So far, there is no case hits this check.
+if (getStatsOptions.isGetEarliestTimeInBacklog()) {
+throw new IllegalArgumentException("Calling the sync method
subscription.getStats with"
++ " getEarliestTimeInBacklog, it may encountered a
deadlock error.");
+}
+// The method "getStatsAsync" will be a sync method if the param
"isGetEarliestTimeInBacklog" is false.
+try {
+return getStatsAsync(getStatsOptions).get(5, TimeUnit.SECONDS);
Review Comment:
> The metadata-store thread will be released
This thread will be released by
`org.apache.pulsar.metadata.impl.ZKSessionWatcher`.
--
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] [improve] [test] add end to end deduplication test. [pulsar]
thetumbled commented on code in PR #23071:
URL: https://github.com/apache/pulsar/pull/23071#discussion_r1694656154
##
pulsar-broker/src/test/java/org/apache/pulsar/client/api/DeduplicationEndToEndTest.java:
##
@@ -0,0 +1,672 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.api;
+
+import io.netty.util.TimerTask;
+import org.apache.pulsar.broker.service.PulsarCommandSender;
+import org.apache.pulsar.broker.service.ServerCnx;
+import org.apache.pulsar.broker.service.persistent.PersistentTopic;
+import org.apache.pulsar.client.impl.ClientCnx;
+import org.apache.pulsar.client.impl.ConnectionHandler;
+import org.apache.pulsar.client.impl.MultiTopicsConsumerImpl;
+import org.apache.pulsar.client.impl.PartitionedProducerImpl;
+import org.apache.pulsar.client.impl.ProducerImpl;
+import org.apache.pulsar.client.impl.PulsarClientImpl;
+import org.awaitility.Awaitility;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+
+/**
+ * This test class is used to point out cases where message duplication can
occur,
+ * producer idempotency features can be used to solve which cases and can't
solve which cases.
+ * There are mainly two kinds of message duplication:
+ * 1. Producer sends the message but doesn't receive the ack due to network
issue, broker issue, etc.
+ * User receives the exception and sends the same message again.
+ * This resend operation is executed by the user, so the user can control the
sequence id of the message or not.
+ * Without message deduplication, the message is duplicated definitely.
+ * With message deduplication, the cases vary:
+ * - If user don't control the sequence id, the message is duplicated, as the
message is resent with a different sequence id.
+ * - If user control the sequence id, there are chances that the message is
duplicate or not, depending on whether
+ *the topic is single partitioned or multi partitioned, whether the
sequence id is used as the key of the message,
+ *whether the partition number is updated between the two messages.
+ *
+ * 2. The connection between the producer and the broker is broken after the
producer sends the message,
+ * but before the producer receives the ack. The producer reconnects to the
broker and sends the same message again internally.
+ * In this case, the producer can't control the sequence id of the message.
The resent message remains the same as the original message.
+ * - Without message deduplication, the message is duplicated.
+ * - With message deduplication, the message is not duplicated.
+ */
+@Test(groups = "broker-api")
+public class DeduplicationEndToEndTest extends ProducerConsumerBase {
+
+@BeforeClass
+@Override
+protected void setup() throws Exception {
+super.internalSetup();
+super.producerBaseSetup();
+}
+
+@AfterClass(alwaysRun = true)
+@Override
+protected void cleanup() throws Exception {
+super.internalCleanup();
+}
+
+private List> assertDuplicate(Consumer consumer,
byte[] data) throws PulsarClientException {
+// consume the message, there are at least two messages in the topic
+List> messages = new ArrayList<>(2);
+Message message = consumer.receive(1, TimeUnit.SECONDS);
+assertNotNull(message);
+assertEquals(message.getData(), data);
+messages.add(message);
+message = consumer.receive(1, TimeUnit.SECONDS);
+assertNotNull(message);
+assertEquals(message.getData(), data);
+messages.add(message);
+return messages;
+}
+
+private Message assertNotDuplicate(Consumer consumer,
byte[] data) throws PulsarClientException {
+// consume the message,
Re: [PR] [fix] [broker] Remove blocking calls from Subscription.getStats [pulsar]
nodece commented on code in PR #23088:
URL: https://github.com/apache/pulsar/pull/23088#discussion_r1694510897
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##
@@ -1201,6 +1203,21 @@ public long estimateBacklogSize() {
}
public SubscriptionStatsImpl getStats(GetStatsOptions getStatsOptions) {
+// So far, there is no case hits this check.
+if (getStatsOptions.isGetEarliestTimeInBacklog()) {
+throw new IllegalArgumentException("Calling the sync method
subscription.getStats with"
++ " getEarliestTimeInBacklog, it may encountered a
deadlock error.");
+}
+// The method "getStatsAsync" will be a sync method if the param
"isGetEarliestTimeInBacklog" is false.
+try {
+return getStatsAsync(getStatsOptions).get(5, TimeUnit.SECONDS);
Review Comment:
The `metadata-store` thread will be released after 30 seconds, because the
zk client will be reconnected(I remember).
~So I say, your way just reduce the lock time to 5s.~, use `get()` with 5s
is meaningless, because this thread was blocked, don't be released.
The sync method has been deprecated, which is used for the unit test and
removeSubscription.
https://github.com/user-attachments/assets/faaeb106-205c-4b7d-9c21-976e670e9cfa;>
--
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][broker] Fix MessageDeduplication replay timeout cause topic loading stuck [pulsar]
TakaHiR07 commented on PR #23004: URL: https://github.com/apache/pulsar/pull/23004#issuecomment-2255040654 > @TakaHiR07 would you mind backporting this change to branch-3.2? The PR can be applied to branch-3.2, but the test TopicDuplicationTest.testFinishTakeSnapshotWhenTopicLoading fails consistently. ok. I take some time to see the test. -- 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] [Issue 1259][producer] Prevent panic when calling Flush on closed producer [pulsar-client-go]
crossoverJie merged PR #1260: URL: https://github.com/apache/pulsar-client-go/pull/1260 -- 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: [I] Panic when calling Flush after Close [pulsar-client-go]
crossoverJie closed issue #1259: Panic when calling Flush after Close URL: https://github.com/apache/pulsar-client-go/issues/1259 -- 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] [improve][misc] Sync commits from apache into 3.1_ds [pulsar]
nikhil-ctds closed pull request #23089: [improve][misc] Sync commits from apache into 3.1_ds URL: https://github.com/apache/pulsar/pull/23089 -- 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
