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/09/15 04:54:30 UTC
[GitHub] [pulsar] AnonHxy opened a new pull request, #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout
AnonHxy opened a new pull request, #17663:
URL: https://github.com/apache/pulsar/pull/17663
Fixes https://github.com/apache/pulsar/issues/17662
### Motivation
Fixes https://github.com/apache/pulsar/issues/17662
### Modifications
<!-- Describe the modifications you've done. -->
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on a diff in pull request #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #17663:
URL: https://github.com/apache/pulsar/pull/17663#discussion_r971605481
##########
pulsar-common/src/main/java/org/apache/pulsar/common/compression/CompressionCodecNone.java:
##########
@@ -28,8 +29,15 @@ public class CompressionCodecNone implements CompressionCodec {
@Override
public ByteBuf encode(ByteBuf raw) {
- // Provides an encoder that simply returns the same uncompressed buffer
- return raw.retain();
+ int readableBytes = raw.readableBytes();
+ if (readableBytes == raw.capacity()) {
+ return raw.retain();
+ }
+ raw.markReaderIndex();
+ ByteBuf target = PulsarByteBufAllocator.DEFAULT.buffer(readableBytes, readableBytes);
+ target.writeBytes(raw);
+ raw.resetReaderIndex();
+ return target;
}
Review Comment:
Do you mean: The `MemoryLimitController` manages memory according to the real size of message, but the message is allocated a bigger memory by ` BatchMessageContainerImpl`?
--
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] AnonHxy commented on pull request #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout using CompressionType.NONE
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #17663:
URL: https://github.com/apache/pulsar/pull/17663#issuecomment-1259446621
closed this pr becase it' not the best solution
--
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] AnonHxy commented on a diff in pull request #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17663:
URL: https://github.com/apache/pulsar/pull/17663#discussion_r971621312
##########
pulsar-common/src/main/java/org/apache/pulsar/common/compression/CompressionCodecNone.java:
##########
@@ -28,8 +29,15 @@ public class CompressionCodecNone implements CompressionCodec {
@Override
public ByteBuf encode(ByteBuf raw) {
- // Provides an encoder that simply returns the same uncompressed buffer
- return raw.retain();
+ int readableBytes = raw.readableBytes();
+ if (readableBytes == raw.capacity()) {
+ return raw.retain();
+ }
+ raw.markReaderIndex();
+ ByteBuf target = PulsarByteBufAllocator.DEFAULT.buffer(readableBytes, readableBytes);
+ target.writeBytes(raw);
+ raw.resetReaderIndex();
+ return target;
}
Review Comment:
Right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy closed pull request #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout using CompressionType.NONE
Posted by GitBox <gi...@apache.org>.
AnonHxy closed pull request #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout using CompressionType.NONE
URL: https://github.com/apache/pulsar/pull/17663
--
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] HQebupt commented on a diff in pull request #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout using CompressionType.NONE
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #17663:
URL: https://github.com/apache/pulsar/pull/17663#discussion_r971766275
##########
pulsar-common/src/main/java/org/apache/pulsar/common/compression/CompressionCodecNone.java:
##########
@@ -28,8 +29,15 @@ public class CompressionCodecNone implements CompressionCodec {
@Override
public ByteBuf encode(ByteBuf raw) {
- // Provides an encoder that simply returns the same uncompressed buffer
- return raw.retain();
+ int readableBytes = raw.readableBytes();
+ if (readableBytes == raw.capacity()) {
+ return raw.retain();
+ }
+ raw.markReaderIndex();
+ ByteBuf target = PulsarByteBufAllocator.DEFAULT.buffer(readableBytes, readableBytes);
+ target.writeBytes(raw);
+ raw.resetReaderIndex();
+ return target;
}
Review Comment:
LGTM
--
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 #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout using CompressionType.NONE
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #17663:
URL: https://github.com/apache/pulsar/pull/17663#issuecomment-1248984132
@AnonHxy Please take a look at this PR #15033. It tries to solve the memory waste problem.
--
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 #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout using CompressionType.NONE
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17663:
URL: https://github.com/apache/pulsar/pull/17663#discussion_r972663811
##########
pulsar-common/src/main/java/org/apache/pulsar/common/compression/CompressionCodecNone.java:
##########
@@ -28,8 +29,15 @@ public class CompressionCodecNone implements CompressionCodec {
@Override
public ByteBuf encode(ByteBuf raw) {
- // Provides an encoder that simply returns the same uncompressed buffer
- return raw.retain();
+ int readableBytes = raw.readableBytes();
+ if (readableBytes == raw.capacity()) {
+ return raw.retain();
+ }
+ raw.markReaderIndex();
+ ByteBuf target = PulsarByteBufAllocator.DEFAULT.buffer(readableBytes, readableBytes);
+ target.writeBytes(raw);
+ raw.resetReaderIndex();
+ return target;
Review Comment:
+1.
1. We should take the whole buffer size into account in `MemoryLimitController`. Because it's allocated by our producer.
2. There is a memory waste problem in producer batch container, as the max buffer size only grows.
--
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] AnonHxy commented on pull request #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout using CompressionType.NONE
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #17663:
URL: https://github.com/apache/pulsar/pull/17663#issuecomment-1247705871
> Good work.
>
> I just wonder if the title of the PR should mention that this applies to CompressionType.NONE .
Sure
--
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] merlimat commented on a diff in pull request #17663: [fix][client]Fix Producer exceeds PulsarClient memory limit when sending fail timeout using CompressionType.NONE
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #17663:
URL: https://github.com/apache/pulsar/pull/17663#discussion_r972135652
##########
pulsar-common/src/main/java/org/apache/pulsar/common/compression/CompressionCodecNone.java:
##########
@@ -28,8 +29,15 @@ public class CompressionCodecNone implements CompressionCodec {
@Override
public ByteBuf encode(ByteBuf raw) {
- // Provides an encoder that simply returns the same uncompressed buffer
- return raw.retain();
+ int readableBytes = raw.readableBytes();
+ if (readableBytes == raw.capacity()) {
+ return raw.retain();
+ }
+ raw.markReaderIndex();
+ ByteBuf target = PulsarByteBufAllocator.DEFAULT.buffer(readableBytes, readableBytes);
+ target.writeBytes(raw);
+ raw.resetReaderIndex();
+ return target;
Review Comment:
I don't think this is the right solution because it would cause a memory copy for all the batches.
--
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