You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/08/02 08:56:48 UTC
[GitHub] [pulsar] Nicklee007 opened a new pull request, #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Nicklee007 opened a new pull request, #16915:
URL: https://github.com/apache/pulsar/pull/16915
### Motivation
Release memory usage when invalid message, the related PR #16835 only release the memory usage, but the semaphore also need release in here.
The fixed unit test can reproduce it, which only `enableBatching` can invoke the `createOpSendMsg` in `BatchMessageContainerImpl`.
### Modifications
1. add semaphore release;
2. fix unit test;
### Documentation
- [X] `doc-not-needed`
(Please explain why)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1206282326
Move `release/2.7.5` to https://github.com/apache/pulsar/pull/16957
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Nicklee007 commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1206242921
> @Nicklee007 Please help open another PR to merge this to branch 2.7
@Jason918 branch 2.7 cherry-pick PR #16957 , PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1206759681
@Nicklee007 Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#discussion_r935429100
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerSemaphoreTest.java:
##########
@@ -66,7 +66,7 @@ public void testProducerSemaphoreInvalidMessage() throws Exception {
ProducerImpl<byte[]> producer = (ProducerImpl<byte[]>) pulsarClient.newProducer()
.topic("testProducerSemaphoreAcquire")
.maxPendingMessages(pendingQueueSize)
- .enableBatching(false)
+ .enableBatching(true)
Review Comment:
Should we also cover the case when batching is disabled? Maybe add a parameter in the test method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Nicklee007 commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1206241466
> Please also help to cherry-pick to branch 2.9
@mattisonchao ok, I'll cherry-pick it to branch 2.9.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1205026319
Please also help to cherry-pick to branch 2.9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a diff in pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on code in PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#discussion_r936217583
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerSemaphoreTest.java:
##########
@@ -79,6 +79,17 @@ public void testProducerSemaphoreInvalidMessage() throws Exception {
} catch (PulsarClientException.InvalidMessageException ex) {
Assert.assertEquals(producer.getSemaphore().get().availablePermits(), pendingQueueSize);
}
+
+ producer.conf.setBatchingEnabled(false);
+ try {
+ try (MockedStatic<ClientCnx> mockedStatic = Mockito.mockStatic(ClientCnx.class)) {
+ mockedStatic.when(ClientCnx::getMaxMessageSize).thenReturn(2);
+ producer.send("semaphore-test".getBytes(StandardCharsets.UTF_8));
+ }
+ throw new IllegalStateException("can not reach here");
Review Comment:
You can use `fail()` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- merged pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #16915:
URL: https://github.com/apache/pulsar/pull/16915
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1204849099
There is a conflict when direct merge this to branch 2.7
@Nicklee007 Please help open another PR to merge this to branch 2.7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1206282880
@Nicklee007 Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Nicklee007 commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1206298746
> Please also help to cherry-pick to branch 2.9
@mattisonchao branch 2.9 cherry-pick PR #16958, PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1210825437
@Nicklee007 Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#issuecomment-1210824120
Move `release/2.9.4` to https://github.com/apache/pulsar/pull/16958
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Nicklee007 commented on a diff in pull request #16915: [fix][client]Fix MaxQueueSize semaphore release leak in createOpSendMsg
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on code in PR #16915:
URL: https://github.com/apache/pulsar/pull/16915#discussion_r935462681
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerSemaphoreTest.java:
##########
@@ -66,7 +66,7 @@ public void testProducerSemaphoreInvalidMessage() throws Exception {
ProducerImpl<byte[]> producer = (ProducerImpl<byte[]>) pulsarClient.newProducer()
.topic("testProducerSemaphoreAcquire")
.maxPendingMessages(pendingQueueSize)
- .enableBatching(false)
+ .enableBatching(true)
Review Comment:
@Jason918 Add test to cover the batching is disabled case, PTAL 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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org