You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/03/25 02:58:10 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #3048:
ARTEMIS-1975 Fixing LargeMessage encoding for replication
clebertsuconic opened a new pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3048:
ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#issuecomment-603936602
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r398090739
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessagePersister.java
##########
@@ -96,10 +91,11 @@ public void encode(ActiveMQBuffer buffer, Message record) {
properties.encode(buffer.byteBuf());
}
- ByteBuf savedEncodeBuffer = getSavedEncodeBuffer(record);
+ ByteBuf savedEncodeBuffer = msgEncode.getSavedEncodeBuffer();
buffer.writeBytes(savedEncodeBuffer, 0, savedEncodeBuffer.writerIndex());
- savedEncodeBuffer.release();
- msgEncode.temporaryBuffer = null;
+ msgEncode.releaseEncodedBuffer();
+ msgEncode.releaseEncodedBuffer(); // We need two releases here, one for the retain when the savedEncodeBuffer was called
Review comment:
I'm still fighting to find some water :(
I would like you to be happy about the naming anyway, impl-wire you can use https://netty.io/4.1/api/io/netty/util/ReferenceCounted.html#release-int- that would release the buffer the number of times you need (and can return a boolean if you need to do something right after too)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397978492
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
+ if (temporaryBuffer != null && temporaryBuffer.release()) {
+ temporaryBuffer = null;
+ }
+ }
+
+ public synchronized ByteBuf getSavedEncodeBuffer() {
+ if (temporaryBuffer == null) {
+ temporaryBuffer = PooledByteBufAllocator.DEFAULT.buffer(getEstimateSavedEncode());
+ saveEncoding(temporaryBuffer);
+ }
+ return temporaryBuffer.retain();
Review comment:
I renamed the method. This is not much my style of coding. I have seen other code you've written and you would create a new method. but I will submit and created the new method now.
pushed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397651623
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
Review comment:
Id rather this is sync. Dont need to be chasing such difficult issues again
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397925010
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
Review comment:
So, I will keep the synchronize 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397640655
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessagePersister.java
##########
@@ -96,10 +91,11 @@ public void encode(ActiveMQBuffer buffer, Message record) {
properties.encode(buffer.byteBuf());
}
- ByteBuf savedEncodeBuffer = getSavedEncodeBuffer(record);
+ ByteBuf savedEncodeBuffer = msgEncode.getSavedEncodeBuffer();
buffer.writeBytes(savedEncodeBuffer, 0, savedEncodeBuffer.writerIndex());
- savedEncodeBuffer.release();
- msgEncode.temporaryBuffer = null;
+ msgEncode.releaseEncodedBuffer();
+ msgEncode.releaseEncodedBuffer(); // We need two releases here, one for the retain when the savedEncodeBuffer was called
Review comment:
Having 2 separate releases is prone to future errors: I believe this can be simplified to make esplicit in the method names what's happening 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] asfgit closed pull request #3048: ARTEMIS-1975
Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r398090739
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessagePersister.java
##########
@@ -96,10 +91,11 @@ public void encode(ActiveMQBuffer buffer, Message record) {
properties.encode(buffer.byteBuf());
}
- ByteBuf savedEncodeBuffer = getSavedEncodeBuffer(record);
+ ByteBuf savedEncodeBuffer = msgEncode.getSavedEncodeBuffer();
buffer.writeBytes(savedEncodeBuffer, 0, savedEncodeBuffer.writerIndex());
- savedEncodeBuffer.release();
- msgEncode.temporaryBuffer = null;
+ msgEncode.releaseEncodedBuffer();
+ msgEncode.releaseEncodedBuffer(); // We need two releases here, one for the retain when the savedEncodeBuffer was called
Review comment:
I'm still fighting to find some water :(
I would like you to be happy about the naming anyway, impl-wire you can use https://netty.io/4.1/api/io/netty/util/ReferenceCounted.html#release-int- that would release the buffer the number of times you need (and can return a boolean if you need to do something right after too): release is quite costy so doing it in one call is better
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397945983
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessagePersister.java
##########
@@ -96,10 +91,11 @@ public void encode(ActiveMQBuffer buffer, Message record) {
properties.encode(buffer.byteBuf());
}
- ByteBuf savedEncodeBuffer = getSavedEncodeBuffer(record);
+ ByteBuf savedEncodeBuffer = msgEncode.getSavedEncodeBuffer();
buffer.writeBytes(savedEncodeBuffer, 0, savedEncodeBuffer.writerIndex());
- savedEncodeBuffer.release();
- msgEncode.temporaryBuffer = null;
+ msgEncode.releaseEncodedBuffer();
+ msgEncode.releaseEncodedBuffer(); // We need two releases here, one for the retain when the savedEncodeBuffer was called
Review comment:
Just to write here an example: https://netty.io/4.1/api/io/netty/buffer/ByteBuf.html#readRetainedSlice-int-
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397925776
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
+ if (temporaryBuffer != null && temporaryBuffer.release()) {
+ temporaryBuffer = null;
+ }
+ }
+
+ public synchronized ByteBuf getSavedEncodeBuffer() {
+ if (temporaryBuffer == null) {
+ temporaryBuffer = PooledByteBufAllocator.DEFAULT.buffer(getEstimateSavedEncode());
+ saveEncoding(temporaryBuffer);
+ }
+ return temporaryBuffer.retain();
Review comment:
I know but looking at the naming used on Netty I think is a good idea to communicate when something is to be relased after usage and when not, just to be less error prone
eg Just to write here an example: https://netty.io/4.1/api/io/netty/buffer/ByteBuf.html#readRetainedSlice-int-
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397639669
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
+ if (temporaryBuffer != null && temporaryBuffer.release()) {
+ temporaryBuffer = null;
+ }
+ }
+
+ public synchronized ByteBuf getSavedEncodeBuffer() {
+ if (temporaryBuffer == null) {
+ temporaryBuffer = PooledByteBufAllocator.DEFAULT.buffer(getEstimateSavedEncode());
+ saveEncoding(temporaryBuffer);
+ }
+ return temporaryBuffer.retain();
Review comment:
retain is a costly operation, is it necessary? Or we just document that the saved encode buffer cannot be conserved but just immediately used?
I would document it anyway ( eg retainedSavedEncodeBuffer) to save a user to forget to release it and cause memory leaks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397843241
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
+ if (temporaryBuffer != null && temporaryBuffer.release()) {
+ temporaryBuffer = null;
+ }
+ }
+
+ public synchronized ByteBuf getSavedEncodeBuffer() {
+ if (temporaryBuffer == null) {
+ temporaryBuffer = PooledByteBufAllocator.DEFAULT.buffer(getEstimateSavedEncode());
+ saveEncoding(temporaryBuffer);
+ }
+ return temporaryBuffer.retain();
Review comment:
Retain is just a ref counter increment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397925776
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
+ if (temporaryBuffer != null && temporaryBuffer.release()) {
+ temporaryBuffer = null;
+ }
+ }
+
+ public synchronized ByteBuf getSavedEncodeBuffer() {
+ if (temporaryBuffer == null) {
+ temporaryBuffer = PooledByteBufAllocator.DEFAULT.buffer(getEstimateSavedEncode());
+ saveEncoding(temporaryBuffer);
+ }
+ return temporaryBuffer.retain();
Review comment:
I know but looking at the naming used on Netty I think is a good idea to communicate when something is to be relased after usage and when not, just to be less error prone...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3048:
ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#issuecomment-603914033
I'm just doing some testing.. please don't merge it yet.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397639669
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
+ if (temporaryBuffer != null && temporaryBuffer.release()) {
+ temporaryBuffer = null;
+ }
+ }
+
+ public synchronized ByteBuf getSavedEncodeBuffer() {
+ if (temporaryBuffer == null) {
+ temporaryBuffer = PooledByteBufAllocator.DEFAULT.buffer(getEstimateSavedEncode());
+ saveEncoding(temporaryBuffer);
+ }
+ return temporaryBuffer.retain();
Review comment:
retain is a costly operation, is it necessary? Or we just document that the saved encode buffer cannot be conserved but just immediately used?
I would document it anyway to save a user to forget to release it and cause memory leaks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3048:
ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#issuecomment-603922906
the issue I found is totally not related.. I'm sending a separate 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3048:
ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#issuecomment-603931840
do not merge again.. as I'm dealing with the method rename.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397696354
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
Review comment:
Yep, agree I just wanted be sure was a correctness requirement :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397842608
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
Review comment:
This is about persistence. Which will not happen concurrently unless it’s replication.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #3048:
ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#issuecomment-603613295
today I have sent two commits directly into master, as I thought they were obvious change that didn't require peer review.
in the process I broke org.apache.activemq.artemis.tests.integration.amqp.largemessages.AmqpReplicatedLargeMessageTest
this is fixing AmqpReplicatedLargeMessageTest
Instead of using a Thread local for the cache of the saved buffer, I'm storing the record on the Message.
However for replication, this may be broken due to multi-threading.
This is fixing that by adding reference counting.
I did some manual testing to make sure there are no leaks. I couldn't find a way to add a permanent test without being invasive in performance.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397843569
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessagePersister.java
##########
@@ -96,10 +91,11 @@ public void encode(ActiveMQBuffer buffer, Message record) {
properties.encode(buffer.byteBuf());
}
- ByteBuf savedEncodeBuffer = getSavedEncodeBuffer(record);
+ ByteBuf savedEncodeBuffer = msgEncode.getSavedEncodeBuffer();
buffer.writeBytes(savedEncodeBuffer, 0, savedEncodeBuffer.writerIndex());
- savedEncodeBuffer.release();
- msgEncode.temporaryBuffer = null;
+ msgEncode.releaseEncodedBuffer();
+ msgEncode.releaseEncodedBuffer(); // We need two releases here, one for the retain when the savedEncodeBuffer was called
Review comment:
I will make a new method to do a release(2)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397651002
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
Review comment:
Largemessage can be concurrent accessed and used. In case of topic with multiple queues.
Remember how we had to fix loads of concurrent access issue on core message the other year
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397638589
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessage.java
##########
@@ -126,6 +127,20 @@ public void closeLargeMessage() throws Exception {
parsingData = null;
}
+ public synchronized void releaseEncodedBuffer() {
Review comment:
Why synchronized? We perform these operations concurrently?
If the semantic of accesses is as-single-threaded, but is happening on different Threads one-by-one, is ok to remove the synchronization (and better, because better document that this class shouldn't be used for concurrent accesses)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3048: ARTEMIS-1975 Fixing LargeMessage encoding for replication
URL: https://github.com/apache/activemq-artemis/pull/3048#discussion_r397945983
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessagePersister.java
##########
@@ -96,10 +91,11 @@ public void encode(ActiveMQBuffer buffer, Message record) {
properties.encode(buffer.byteBuf());
}
- ByteBuf savedEncodeBuffer = getSavedEncodeBuffer(record);
+ ByteBuf savedEncodeBuffer = msgEncode.getSavedEncodeBuffer();
buffer.writeBytes(savedEncodeBuffer, 0, savedEncodeBuffer.writerIndex());
- savedEncodeBuffer.release();
- msgEncode.temporaryBuffer = null;
+ msgEncode.releaseEncodedBuffer();
+ msgEncode.releaseEncodedBuffer(); // We need two releases here, one for the retain when the savedEncodeBuffer was called
Review comment:
Just to write here an example: https://netty.io/4.1/api/io/netty/buffer/ByteBuf.html#readRetainedSlice-int-
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services