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/02/19 02:46:46 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #2986:
ARTEMIS-1975 Real Large Message support into AMQP
clebertsuconic opened a new pull request #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986
This is Large commit, where I am refactoring largeMessage Body out of CoreMessage
which is now reused with AMQP.
I had also to fix Reference Counting to fix how Large Messages are Acked
And I also had to make sure Large Messages are transversing correctly when in cluster.
----------------------------------------------------------------
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 #2986:
ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#issuecomment-589697754
I rebased.. and I think this is now ready to be merged.
----------------------------------------------------------------
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] tabish121 commented on a change in pull request
#2986: ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
tabish121 commented on a change in pull request #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#discussion_r382104032
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java
##########
@@ -278,39 +283,66 @@ public void onMessage(Delivery delivery) throws ActiveMQAMQPException {
return;
}
- if (delivery.isAborted()) {
- // Aborting implicitly remotely settles, so advance
- // receiver to the next delivery and settle locally.
- receiver.advance();
- delivery.settle();
+ try {
+ if (delivery.isAborted()) {
+ // Aborting implicitly remotely settles, so advance
+ // receiver to the next delivery and settle locally.
+ receiver.advance();
+ delivery.settle();
+
+ // Replenish the credit if not doing a drain
+ if (!receiver.getDrain()) {
+ receiver.flow(1);
+ }
+
+ return;
+ } else if (delivery.isPartial()) {
Review comment:
I think you need to add some more metrics here on what constitutes a "large" message as there isn't anything that requires a client to strictly adhere to max frame size limits other that it shouldn't go over, it can certainly send under that amount. A client could split a 4k message into 4 1k chunks etc so assuming that a message is large just based on partial will lead to likely considering messages that don't meet the normal Artemis definition of a large message as being large. Also even if not partial you ought to be checking if what you have in a complete message that meets your definition of a large message as the AMQP max frame size is configurable on the protocol adapter so you might need a second metric there to say it's not partial but it exceeds what we consider "not large" and would prefer to store it in the large message way.
----------------------------------------------------------------
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 #2986: ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#discussion_r382127088
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java
##########
@@ -278,39 +283,66 @@ public void onMessage(Delivery delivery) throws ActiveMQAMQPException {
return;
}
- if (delivery.isAborted()) {
- // Aborting implicitly remotely settles, so advance
- // receiver to the next delivery and settle locally.
- receiver.advance();
- delivery.settle();
+ try {
+ if (delivery.isAborted()) {
+ // Aborting implicitly remotely settles, so advance
+ // receiver to the next delivery and settle locally.
+ receiver.advance();
+ delivery.settle();
+
+ // Replenish the credit if not doing a drain
+ if (!receiver.getDrain()) {
+ receiver.flow(1);
+ }
+
+ return;
+ } else if (delivery.isPartial()) {
Review comment:
@tabish121 makes sense.. thanks a lot.. will add another commit to the REVIEW
----------------------------------------------------------------
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 #2986: ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#discussion_r382197254
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java
##########
@@ -278,39 +283,66 @@ public void onMessage(Delivery delivery) throws ActiveMQAMQPException {
return;
}
- if (delivery.isAborted()) {
- // Aborting implicitly remotely settles, so advance
- // receiver to the next delivery and settle locally.
- receiver.advance();
- delivery.settle();
+ try {
+ if (delivery.isAborted()) {
+ // Aborting implicitly remotely settles, so advance
+ // receiver to the next delivery and settle locally.
+ receiver.advance();
+ delivery.settle();
+
+ // Replenish the credit if not doing a drain
+ if (!receiver.getDrain()) {
+ receiver.flow(1);
+ }
+
+ return;
+ } else if (delivery.isPartial()) {
Review comment:
@tabish121 commit added
These commits will be squahsed after we are done with the review.
----------------------------------------------------------------
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 #2986:
ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#issuecomment-589468544
I'm marking do not merge yet.. but please keep reviewing.
----------------------------------------------------------------
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 issue #2986: ARTEMIS-1975
Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#issuecomment-590891990
Going to merge this: well done bud!
----------------------------------------------------------------
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 #2986:
ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#issuecomment-588538063
I have done my own review and I realized I missed some flow control logic. this last commit is adding 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] clebertsuconic commented on a change in pull
request #2986: ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#discussion_r381055760
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/RefCountMessage.java
##########
@@ -19,66 +19,155 @@
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
-public abstract class RefCountMessage implements Message {
+// import org.apache.activemq.artemis.utils.collections.ConcurrentHashSet; -- #ifdef DEBUG
+
+public class RefCountMessage {
private static final AtomicIntegerFieldUpdater<RefCountMessage> DURABLE_REF_COUNT_UPDATER = AtomicIntegerFieldUpdater.newUpdater(RefCountMessage.class, "durableRefCount");
private static final AtomicIntegerFieldUpdater<RefCountMessage> REF_COUNT_UPDATER = AtomicIntegerFieldUpdater.newUpdater(RefCountMessage.class, "refCount");
+ private static final AtomicIntegerFieldUpdater<RefCountMessage> REF_USAGE_UPDATER = AtomicIntegerFieldUpdater.newUpdater(RefCountMessage.class, "usageCount");
private volatile int durableRefCount = 0;
private volatile int refCount = 0;
- private RefCountMessageListener context;
+ private volatile int usageCount = 0;
+
+ private volatile boolean fired = false;
- @Override
- public Message setContext(RefCountMessageListener context) {
- this.context = context;
- return this;
+ public int getRefCount() {
+ return REF_COUNT_UPDATER.get(this);
}
- @Override
- public RefCountMessageListener getContext() {
- return context;
+ public int getUsage() {
+ return REF_USAGE_UPDATER.get(this);
}
- @Override
- public int getRefCount() {
- return refCount;
+ public int getDurableCount() {
+ return DURABLE_REF_COUNT_UPDATER.get(this);
}
- @Override
- public int incrementRefCount() throws Exception {
- int count = REF_COUNT_UPDATER.incrementAndGet(this);
- if (context != null) {
- context.nonDurableUp(this, count);
+ /**
+ * in certain cases the large message is copied from another LargeServerMessage
+ * and the ref count needs to be on that place.
+ */
+ private volatile RefCountMessage parentRef;
+
+ public RefCountMessage getParentRef() {
+ return parentRef;
+ }
+ // I am usually against keeping commented out code
+ // However this is very useful for me to debug referencing counting.
+ // Uncomment out anything between #ifdef DEBUG and #endif
+
+ // #ifdef DEBUG -- comment out anything before endif if you want to debug REFERENCE COUNTS
+ //final ConcurrentHashSet<Exception> upSet = new ConcurrentHashSet<>();
+ // #endif
+
+ private void onUp() {
+ // #ifdef DEBUG -- comment out anything before endif if you want to debug REFERENCE COUNTS
+ // upSet.add(new Exception("upEvent(" + debugString() + ")"));
+ // #endif
+ }
+
+ private void onDown() {
+ // #ifdef DEBUG -- comment out anything before endif if you want to debug REFERENCE COUNTS
+ // upSet.add(new Exception("upEvent(" + debugString() + ")"));
+ // #endif
+ if (getRefCount() <= 0 && getUsage() <= 0 && getDurableCount() <= 0 && !fired) {
+
+ debugRefs();
+ fired = true;
+ releaseComplete();
+ }
+ }
+ /**
+ *
+ * This method will be useful if you remove commented out code around #ifdef AND #endif COMMENTS
+ * */
+ public final void debugRefs() {
Review comment:
I'm keeping these if I ever need to debug these again.
Too bad there's no IFDEF in 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.
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 #2986:
ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#issuecomment-589270846
Doing my own review here: since I now added some configuration option for minlargeMessageSize, I will need to touch some documentation around Large Messages.
I'm adding some paragraphs to the large message size chapter. I will add a new commit shortly.
----------------------------------------------------------------
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 #2986: ARTEMIS-1975
Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986
----------------------------------------------------------------
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 #2986:
ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#issuecomment-589389651
@tabish121
@franz1981
@jbertram
I'm going to squash this tomorrow. I had added separate commits this time instead of squashing to make the review eaiser.
Please let me know what you think... I think this is ready to be merged to I will squash tomorrow.
----------------------------------------------------------------
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 #2986: ARTEMIS-1975 Real Large Message support into AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2986: ARTEMIS-1975 Real Large Message support into AMQP
URL: https://github.com/apache/activemq-artemis/pull/2986#discussion_r382127241
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java
##########
@@ -278,39 +283,66 @@ public void onMessage(Delivery delivery) throws ActiveMQAMQPException {
return;
}
- if (delivery.isAborted()) {
- // Aborting implicitly remotely settles, so advance
- // receiver to the next delivery and settle locally.
- receiver.advance();
- delivery.settle();
+ try {
+ if (delivery.isAborted()) {
+ // Aborting implicitly remotely settles, so advance
+ // receiver to the next delivery and settle locally.
+ receiver.advance();
+ delivery.settle();
+
+ // Replenish the credit if not doing a drain
+ if (!receiver.getDrain()) {
+ receiver.flow(1);
+ }
+
+ return;
+ } else if (delivery.isPartial()) {
Review comment:
I was actually looking for that information.. if it made sense to use frame size or not.
----------------------------------------------------------------
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