[GitHub] activemq-artemis pull request #:
Github user michaelandrepearce commented on the pull request: https://github.com/apache/activemq-artemis/commit/a2da41ee2e347bcf8fe721bded89e4a78ca14cfb#commitcomment-29775703 In artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java: In artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java on line 948: This should only be blocking if sendblocking. Else its not honouring the send type ---
[GitHub] activemq-artemis pull request #:
Github user michaelandrepearce commented on the pull request: https://github.com/apache/activemq-artemis/commit/a2da41ee2e347bcf8fe721bded89e4a78ca14cfb#commitcomment-29775732 In artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java: In artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java on line 574: Could this be a volatile with an atomic field updater ---
[GitHub] activemq-artemis pull request #:
Github user michaelandrepearce commented on the pull request: https://github.com/apache/activemq-artemis/commit/a2da41ee2e347bcf8fe721bded89e4a78ca14cfb#commitcomment-29775919 In artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java: In artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java on line 574: On that note this shouldnt have been needed as when the responses comeback to the correlation cache its meant to remove thus avoid situation of double confirm. If you saw double confirm then the code there should be addressed. ---
[GitHub] activemq-artemis issue #2187: ARTEMIS-1545 Support JMS 2.0 Completion Listen...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2187 @jbertram left a few comments i am unfortuantly on leave abroad so only a brief view via github diff via phone ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 See inline comments ---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2191#discussion_r204785897
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
---
@@ -822,6 +833,20 @@ public void resetMetadata(HashMap
metaDataToSend) {
}
}
+ @Override
+ public int getDefaultConsumerWindowSize(SimpleString address) throws
ActiveMQException {
+ if (defaultConsumerWindowSize != null) {
+ return defaultConsumerWindowSize;
+ } else if
(sessionChannel.supports(PacketImpl.SESS_CONS_WINDOW_SIZE_RESP,
getServerVersion())) {
+ Packet packet = sessionChannel.sendBlocking(new
ConsumerWindowSizeQueryMessage(address), PacketImpl.SESS_CONS_WINDOW_SIZE_RESP);
+ ConsumerWindowSizeQueryResponseMessage response =
(ConsumerWindowSizeQueryResponseMessage) packet;
--- End diff --
Could this not be returned in the create consumer response or address
settings lookup, to avoid extra calls. Imagine further defaults etc if
everything was an individual request it would bloat fast.
---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user michaelandrepearce commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2191#discussion_r204784325 --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java --- @@ -347,7 +347,7 @@ private ServerLocatorImpl(final Topology topology, minLargeMessageSize = ActiveMQClient.DEFAULT_MIN_LARGE_MESSAGE_SIZE; - consumerWindowSize = ActiveMQClient.DEFAULT_CONSUMER_WINDOW_SIZE; + consumerWindowSize = -1; --- End diff -- Should still be defaulted to a constant ---
[GitHub] activemq-artemis issue #2175: ARTEMIS-856 - Support consumersBeforeDispatch ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2175 @franz1981 @clebertsuconic can this be merged if no further comments. ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 Is it worth a few other client settings to also be able to be defaulted centrally in broker? ---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2191#discussion_r204908501
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionQueueQueryResponseMessage_V4.java
---
@@ -0,0 +1,151 @@
+/*
+ * 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.activemq.artemis.core.protocol.core.impl.wireformat;
+
+import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
+import org.apache.activemq.artemis.api.core.RoutingType;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.api.core.client.ActiveMQClient;
+import org.apache.activemq.artemis.api.core.client.ClientSession;
+import org.apache.activemq.artemis.core.client.impl.QueueQueryImpl;
+import org.apache.activemq.artemis.core.server.QueueQueryResult;
+
+public class SessionQueueQueryResponseMessage_V4 extends
SessionQueueQueryResponseMessage_V3 {
+
+ protected int defaultConsumerWindowSize;
+
+ public SessionQueueQueryResponseMessage_V4(final QueueQueryResult
result) {
+ this(result.getName(), result.getAddress(), result.isDurable(),
result.isTemporary(), result.getFilterString(), result.getConsumerCount(),
result.getMessageCount(), result.isExists(), result.isAutoCreateQueues(),
result.isAutoCreated(), result.isPurgeOnNoConsumers(), result.getRoutingType(),
result.getMaxConsumers(), result.isExclusive(), result.isLastValue(),
result.getDefaultConsumerWindowSize());
--- End diff --
I would keep it as is. I think a dicussion is needes as currently having
the version and typing means there is a strong contract, moving to a map
removes the strong versioned contract.
---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2191#discussion_r204915141
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionQueueQueryResponseMessage_V4.java
---
@@ -0,0 +1,151 @@
+/*
+ * 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.activemq.artemis.core.protocol.core.impl.wireformat;
+
+import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
+import org.apache.activemq.artemis.api.core.RoutingType;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.api.core.client.ActiveMQClient;
+import org.apache.activemq.artemis.api.core.client.ClientSession;
+import org.apache.activemq.artemis.core.client.impl.QueueQueryImpl;
+import org.apache.activemq.artemis.core.server.QueueQueryResult;
+
+public class SessionQueueQueryResponseMessage_V4 extends
SessionQueueQueryResponseMessage_V3 {
+
+ protected int defaultConsumerWindowSize;
+
+ public SessionQueueQueryResponseMessage_V4(final QueueQueryResult
result) {
+ this(result.getName(), result.getAddress(), result.isDurable(),
result.isTemporary(), result.getFilterString(), result.getConsumerCount(),
result.getMessageCount(), result.isExists(), result.isAutoCreateQueues(),
result.isAutoCreated(), result.isPurgeOnNoConsumers(), result.getRoutingType(),
result.getMaxConsumers(), result.isExclusive(), result.isLastValue(),
result.getDefaultConsumerWindowSize());
--- End diff --
By the way there is no need for a new version with this change if the new
param is added at the end and is nullable (aka Integer) old clients simply wont
read it and new ones will, for new clients they just need to check if still
readable bytes to keep client compatible to older broker. See last change when
we added exclusive how it was done.
I would request this change before merge btw.
---
[GitHub] activemq-artemis pull request #2175: ARTEMIS-856 - Support consumersBeforeDi...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2175#discussion_r205174465
--- Diff:
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/AtomicBooleanFieldUpdater.java
---
@@ -0,0 +1,154 @@
+/*
+ * 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.activemq.artemis.utils;
+
+import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
+import sun.reflect.CallerSensitive;
+
+public class AtomicBooleanFieldUpdater {
+
+ /**
+* Creates and returns an updater for objects with the given field.
+* The Class argument is needed to check that reflective types and
+* generic types match.
+*
+* @param tclass the class of the objects holding the field
+* @param fieldName the name of the field to be updated
+* @param the type of instances of tclass
+* @return the updater
+* @throws IllegalArgumentException if the field is not a
+* volatile long type
+* @throws RuntimeException with a nested reflection-based
+* exception if the class does not hold field or is the wrong type,
+* or the field is inaccessible to the caller according to Java language
+* access control
+*/
+ @CallerSensitive
+ public static AtomicBooleanFieldUpdater newUpdater(Class
tclass, String fieldName) {
--- End diff --
@franz1981 using Boolean you might as well then just use atomic boolean,
which you commented eadlier on and rightly so to use atomic updater. As the
point is to save having object reference overheads.
The point of the class is just to make it reusable if else where you want
else you end up with lots of dupe code just for all the int to bool logic.
---
[GitHub] activemq-artemis issue #2175: ARTEMIS-856 - Support consumersBeforeDispatch ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2175 @clebertsuconic if could merge as is, ill get docs done in another pr but before release if ok ---
[GitHub] activemq-artemis issue #2175: ARTEMIS-856 - Support consumersBeforeDispatch ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2175 Ill rebase tomorrow ---
[GitHub] activemq-artemis pull request #2175: ARTEMIS-856 - Support consumersBeforeDi...
Github user michaelandrepearce closed the pull request at: https://github.com/apache/activemq-artemis/pull/2175 ---
[GitHub] activemq-artemis issue #2175: ARTEMIS-856 - Support consumersBeforeDispatch ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2175 @clebertsuconic ill close this as i dont have time today or monday to dedicate further on this, as coming off holiday have a bit of a full plate in work, and ill work offline to see whats going and re-open once i figured it out. btw trying to ping you on IRC ;) ---
[GitHub] activemq-artemis pull request #2198: ARTEMIS-856 - Support consumersBeforeDi...
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2198 ARTEMIS-856 - Support consumersBeforeDispatch and delayBeforeDispatch https://issues.apache.org/jira/browse/ARTEMIS-856 This is equivalent to consumersBeforeDispatchStarts and timeBeforeDispatchStarts in ActiveMQ 5.x http://activemq.apache.org/message-groups.html This is addressing one of the items on the artemis roadmap: http://activemq.apache.org/activemq-artemis-roadmap.html You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-856 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2198.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2198 commit d01e2d2539ed67ce4f617363ac536b4d0a6d02da Author: Michael André Pearce Date: 2018-07-06T06:44:15Z ARTEMIS-856 - Support consumersBeforeDispatch and delayBeforeDispatch https://issues.apache.org/jira/browse/ARTEMIS-856 This is equivalent to consumersBeforeDispatchStarts and timeBeforeDispatchStarts in ActiveMQ 5.x http://activemq.apache.org/message-groups.html This is addressing one of the items on the artemis roadmap: http://activemq.apache.org/activemq-artemis-roadmap.html ---
[GitHub] activemq-artemis issue #2198: ARTEMIS-856 - Support consumersBeforeDispatch ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2198 @clebertsuconic would let me re-open so heres new PR of the same branch. Ive deduced the failure in test case in is that it seems when the journal is reloaded the queues arent there, thus it doesnt reload messages, on debugging it seems to write to the journal as far as i can tell, but it seems something go wrong in that area. ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 Whats the failure ---
[GitHub] activemq-artemis issue #2198: ARTEMIS-856 - Support consumersBeforeDispatch ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2198 i think its the same issue @cshannon is getting on his PR also. https://github.com/apache/activemq-artemis/pull/2191 ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 @cshannon agreed, seems im hitting the same issue on a similar pr. ---
[GitHub] activemq-artemis pull request #2200: ARTEMIS-1997 - un-needed SimpleString c...
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2200 ARTEMIS-1997 - un-needed SimpleString creation on hotpath with Filters Create the SimpleString on construction of PropertyExpression so it can be re-used instead of creating it every time its filtered in the FilterImpl You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1997-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2200.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2200 commit 4fce78f068cc5474e43704e1fa8da2fb435b170d Author: Michael André Pearce Date: 2018-07-22T03:17:50Z ARTEMIS-1997 - un-needed SimpleString creation on hotpath with Filters Create the SimpleString on construction of PropertyExpression so it can be re-used instead of creating it every time its filtered in the FilterImpl ---
[GitHub] activemq-artemis pull request #:
Github user michaelandrepearce commented on the pull request: https://github.com/apache/activemq-artemis/commit/a2da41ee2e347bcf8fe721bded89e4a78ca14cfb#commitcomment-29887691 In artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java: In artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java on line 574: https://github.com/apache/activemq-artemis/pull/2187/commits/74f6f1887e9c3234812d2b0997c97d30164b985e#diff-27ebccaa2ddf3732ed88750c09f43635 so here it should not happen as handling the packet the correlation id is removed from the cache so simply wouldn't be found if a second confirmation packet comes. ---
[GitHub] activemq-artemis pull request #:
Github user michaelandrepearce commented on the pull request: https://github.com/apache/activemq-artemis/commit/a2da41ee2e347bcf8fe721bded89e4a78ca14cfb#commitcomment-29899636 In artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java: In artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java on line 948: ah ok, sorry missed you moved that logic upwards. ---
[GitHub] activemq-artemis pull request #:
Github user michaelandrepearce commented on the pull request: https://github.com/apache/activemq-artemis/commit/a2da41ee2e347bcf8fe721bded89e4a78ca14cfb#commitcomment-29899679 In artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java: In artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java on line 574: The new response handler only gets invoked for correlating ids, Also the dedupe is the reason for the cache, e.g. it should only via the cache which "should" take care of this, by the nature that the id is removed from cache, so next will have cache miss and do nothing. Was there a case it was being invoked twice? If so that should be fixed as would be a bug in the cache code , or somewhere its being called not via the cache ---
[GitHub] activemq-artemis pull request #2187: ARTEMIS-1545 Support JMS 2.0 Completion...
Github user michaelandrepearce commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2187#discussion_r206422634 --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/PacketImpl.java --- @@ -31,7 +31,8 @@ // 2.0.0 public static final int ADDRESSING_CHANGE_VERSION = 129; - public static final int SHARED_QUEUE_SECURITY_FIX_CHANGE_VERSION = 130; --- End diff -- This was used in version 2.6.1 shouldn't be removed, need to bump the async response change version. (as was made prior to the security fix version) ---
[GitHub] activemq-artemis pull request #2187: ARTEMIS-1545 Support JMS 2.0 Completion...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2187#discussion_r206424803
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
---
@@ -189,16 +185,24 @@ public void kill() {
this.killed = true;
}
+ private void setHandlers() {
+
sessionChannel.setCommandConfirmationHandler(commandConfirmationHandler);
--- End diff --
This was meant to be either newer response handler or the older command
confirm handler. The intent was both shouldn't be set. This may be the reason
of your dupes issue, what was this needed still even with the async response
change?
---
[GitHub] activemq-artemis pull request #2204: ARTEMIS-2001 - JMSXGroupID and JMSXUser...
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2204 ARTEMIS-2001 - JMSXGroupID and JMSXUserID in getPropertyNames Ensure JMSXGroupID and JMSXUserID is correctly returned by JMS getPropertyNames when set. You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-2001 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2204.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2204 commit 2147f8204572c167459d29f12e333726cf38b87b Author: Michael André Pearce Date: 2018-07-31T13:49:39Z ARTEMIS-2001 - JMSXGroupID and JMSXUserID in getPropertyNames Ensure JMSXGroupID and JMSXUserID is correctly returned by JMS getPropertyNames when set. ---
[GitHub] activemq-artemis issue #2203: ARTEMIS-1999 Broker uses 100% core's CPU time ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2203 @franz1981 i exclusive yes i think would have similar issue, after all it followed the same logic of message groups in part. ---
[GitHub] activemq-artemis pull request #2203: ARTEMIS-1999 Broker uses 100% core's CP...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2203#discussion_r206558769
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
---
@@ -2370,10 +2370,10 @@ private void deliver() {
}
}
-if (pos == endPos) {
- // Round robin'd all
+if (pos == endPos || groupConsumer != null) {
--- End diff --
groupConsumer is only set if a msg group is already assigned, or has
succesfully handled. i would change this to check if groupID is not null.
---
[GitHub] activemq-artemis issue #2203: ARTEMIS-1999 Broker uses 100% core's CPU time ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2203 @franz1981 once you do fix for this with test and this is merged, ill fix exclusive quickly, as then i can just rip your hard work :P :P :P ---
[GitHub] activemq-artemis issue #2204: ARTEMIS-2001 - JMSXGroupID and JMSXUserID in g...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2204 @clebertsuconic @franz1981 test failure is unrelated thats failing for many PR's MultiThreadAsynchronousFileTest. Could you look at this and merged, this caused an issue with spring integration, as it relies on getPropertyNames to get all the properties but we found when using msg groups it not being set and found this is why. ---
[GitHub] activemq-artemis issue #2200: ARTEMIS-1997 - un-needed SimpleString creation...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2200 @franz1981 test failure is un-related, its the same one plaguing other PR's currently. If you're happy if you could merge? ---
[GitHub] activemq-artemis issue #2198: ARTEMIS-856 - Support consumersBeforeDispatch ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2198 @clebertsuconic re-pushed to kick off build again and all green. You ok to merge? ---
[GitHub] activemq-artemis issue #2198: ARTEMIS-856 - Support consumersBeforeDispatch ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2198 @clebertsuconic i removed the custom AtomicBooleanFieldUpdater as @franz1981 wanted and just used AtomicIntegerFieldUpdater direct. Seems i fix the issue and make @franz1981 happy. ---
[GitHub] activemq-artemis issue #2203: ARTEMIS-1999 Broker uses 100% core's CPU time ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2203 @clebertsuconic before i went on holiday it didnt seem to error as much as it is now, i wonder if some recent merge has destabilized the build? I would worry about ignoring it, as its a concurrency test so it maybe actually highlighting an issue thats been introduced by some recent change / merge. Has there been any changes around the journals? ---
[GitHub] activemq-artemis issue #1950: ARTEMIS-1732 AMQP anonymous producer not block...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/1950 @clebertsuconic Whats occuring on this one? We safe to merge this to master now? ---
[GitHub] activemq-artemis issue #2155: ARTEMIS-1949 fix IllegalMonitorStateException ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2155 @wy96f can you look at the failed build? Ideally need a succesful pr build to merge ---
[GitHub] activemq-artemis pull request #2187: ARTEMIS-1545 Support JMS 2.0 Completion...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2187#discussion_r206769179
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
---
@@ -189,16 +185,24 @@ public void kill() {
this.killed = true;
}
+ private void setHandlers() {
+
sessionChannel.setCommandConfirmationHandler(commandConfirmationHandler);
--- End diff --
I have concern why the handler is getting invoked twice. It shouldnt be
possible if its always invoked via the response cache.
Interestingly if i take just the changes justin made in the large message
method, then the original failing tests pass. So the other changes seem to be
not needed but introduce either issue causing the dupe issue that justin
encounters and needs the atomic. I think it needs more discussion and
development still. The answer is probably somewhere inbetween
---
[GitHub] activemq-artemis pull request #2211: Redistfix
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2211 Redistfix @clebetsuconic Here's a slight alternative option to your pr (https://github.com/apache/activemq-artemis/pull/2209) i took the fixes for the server and control, but in the queueimpl I wanted to keep away from having the redistributor in the list (reason being later where want to try make the dispatch policies more pluggable it makes it very hard) Also interestingly applying the same change that Franz has done for CPU here: https://github.com/apache/activemq-artemis/pull/2203 But for the redistributor fixes many of the redistributor issues. One case in the messageredistributiontest though still fails (testRedistributionWithMessageGroups), im looking into why it might, but if you can spot please do tell. If you need master fixed asap, maybe merge your PR, and i can pr this over the top once i figure it out. You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis REDISTFIX Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2211.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2211 commit 59520b9018d4a44fffa0bf05c49970d50ed69ab5 Author: Clebert Suconic Date: 2018-08-02T02:04:31Z ARTEMIS-856 Fixing QueueCommandTest commit f0aca5c953cb3712e668bdfa72dd3fceb7c164e1 Author: Michael André Pearce Date: 2018-08-02T09:51:27Z ARTEMIS-856 Fixing MessageRedistributionTest ---
[GitHub] activemq-artemis issue #2209: ARTEMIS-856 Test fixes
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2209 @clebertsuconic, looks good, there we some bits with consumer lists with the redistributor i heavily want to avoid, im working an alternative option, but if to fix master happy to merge this, and i can supply my alternative later (have raised a pr so you can see, but still not complete) ---
[GitHub] activemq-artemis pull request #2209: ARTEMIS-856 Test fixes
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2209#discussion_r207214263
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
---
@@ -544,6 +560,7 @@ public boolean isExclusive() {
@Override
public synchronized void setExclusive(boolean exclusive) {
+ new Exception("exclusive set at " + exclusive).printStackTrace();
--- End diff --
Think this needs to be removed.
---
[GitHub] activemq-artemis issue #2211: ARTEMIS-856 Test fixes - Alternative
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2211 @clebertsuconic i figured out the last case and fixed it, as normal a one liner. ---
[GitHub] activemq-artemis pull request #2187: ARTEMIS-1545 Support JMS 2.0 Completion...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2187#discussion_r207235658
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
---
@@ -189,16 +185,24 @@ public void kill() {
this.killed = true;
}
+ private void setHandlers() {
+
sessionChannel.setCommandConfirmationHandler(commandConfirmationHandler);
--- End diff --
So issue then will be if you dont handle via the cache, the response
handlers will keep reference to it, and would slowley leak memory
---
[GitHub] activemq-artemis issue #2211: ARTEMIS-856 Test fixes - Alternative
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2211 @franz1981 i dont think it made it before the merge. Could you pr it by itself and ill merge ---
[GitHub] activemq-artemis issue #2211: ARTEMIS-856 Test fixes - Alternative
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2211 @clebertsuconic have the day off dude, i will look into tomorrow during the day, im sure i can sort, if any others just list. ---
[GitHub] activemq-artemis pull request #2212: ARTEMIS-856 Fixing ScaleDownTest
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2212 ARTEMIS-856 Fixing ScaleDownTest Don't increment the pos if redistributor. causes pos to be > size thus index out of bounds when getting the consumer on next loop. You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis fix_scaledown Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2212.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2212 commit 66ff00ad6db53e43c5b23983c3cc0885262b5119 Author: Michael André Pearce Date: 2018-08-03T07:52:47Z ARTEMIS-856 Fixing ScaleDownTest Don't increment the pos if redistributor. causes pos to be > size thus index out of bounds when getting the consumer on next loop. ---
[GitHub] activemq-artemis issue #2211: ARTEMIS-856 Test fixes - Alternative
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2211 @clebertsuconic see https://github.com/apache/activemq-artemis/pull/2212. Was a simple one. ---
[GitHub] activemq-artemis issue #2212: ARTEMIS-856 Fixing ScaleDownTest
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2212 @clebertsuconic for the CI failures you raised. ---
[GitHub] activemq-artemis issue #2212: ARTEMIS-856 Fixing ScaleDownTest
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2212 @clebertsuconic test failure is the intermittent one we spoke on already: MultiThreadAsynchronousFileTest Ill try kick off build again just to get a green pr. but if not this is a known flaky test on Travis. ---
[GitHub] activemq-artemis issue #2212: ARTEMIS-856 Fixing ScaleDownTest
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2212 @clebertsuconic and a rebuild gives us green :) ---
[GitHub] activemq-artemis pull request #:
Github user michaelandrepearce commented on the pull request: https://github.com/apache/activemq-artemis/commit/825081cfc095d52541ebc5631f031059618d871d#commitcomment-29950253 Yup! ---
[GitHub] activemq-artemis pull request #2217: FOR jbertram - ARTEMIS-1545 Memory leak
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2217 FOR jbertram - ARTEMIS-1545 Memory leak @jbertram here is what i was trying to explain as a bit of code about the leaking objects in the response cache when relying on the confirmation handler logic, for the missing responses. If we need to do this, then we must handle the response via the response cache. You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis Memory Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2217.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2217 commit 74f6f1887e9c3234812d2b0997c97d30164b985e Author: Michael André Pearce Date: 2017-12-14T07:47:30Z ARTEMIS-1545 Support JMS 2.0 Completion Listener for Exceptions commit a2da41ee2e347bcf8fe721bded89e4a78ca14cfb Author: Justin Bertram Date: 2018-07-17T15:53:21Z ARTEMIS-1545 refactor & rework a few incompatible pieces Existing commit for ARTEMIS-1545 broke bridges and large messages. This commit fixes those, and refactors the solution a bit to be more clear. commit ffed219e6cd60e9c13f462750c037554c495024f Author: Michael André Pearce Date: 2018-08-06T16:38:05Z Test case to demo, memory/object leak Demonstrate the response cache is not removed when relying on confirm handler, thus holding onto response handlers leaking memory/objects over time. ---
[GitHub] activemq-artemis pull request #2187: ARTEMIS-1545 Support JMS 2.0 Completion...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2187#discussion_r207957133
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
---
@@ -189,16 +185,24 @@ public void kill() {
this.killed = true;
}
+ private void setHandlers() {
+
sessionChannel.setCommandConfirmationHandler(commandConfirmationHandler);
--- End diff --
see https://github.com/apache/activemq-artemis/pull/2217
hopefully this quick test knocked up explains or demo's the issue.
---
[GitHub] activemq-artemis issue #2227: ARTEMIS-1482 Add back check for SimpleString
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2227 The test that was added in the original PR testOutOfBoundsThrownOnMalformedString seems to still pass on master, as such should this test be updated? ---
[GitHub] activemq-artemis pull request #2228: ARTEMIS-2017 SelectorParser cache not t...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2228#discussion_r208655107
--- Diff:
artemis-selector/src/main/java/org/apache/activemq/artemis/selector/impl/SelectorParser.java
---
@@ -80,11 +78,15 @@ public static BooleanExpression parse(String sql)
throws FilterException {
StrictParser parser = new StrictParser(new
StringReader(actual));
e = parser.JmsSelector();
}
-cache.put(sql, e);
+synchronized (cache) {
--- End diff --
Must be a better way than sync blocks.
@franz1981 ideas?
---
[GitHub] activemq-artemis issue #2227: ARTEMIS-1482 Add back check for SimpleString
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2227 My point was more ensuring in future its not regressed again. ---
[GitHub] activemq-artemis pull request #2228: ARTEMIS-2017 SelectorParser cache not t...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2228#discussion_r208765467
--- Diff:
artemis-selector/src/main/java/org/apache/activemq/artemis/selector/impl/SelectorParser.java
---
@@ -80,11 +78,15 @@ public static BooleanExpression parse(String sql)
throws FilterException {
StrictParser parser = new StrictParser(new
StringReader(actual));
e = parser.JmsSelector();
}
-cache.put(sql, e);
+synchronized (cache) {
--- End diff --
I would have two points here then:
If perf here isn't critical, why even bother then having an LRUCache?
Removing it would remove any threading issues right.
If it is critical:
Then a more elegant option than sync blocks would look to update LRUCache
to extend ConcurrentLinkedHashMap
(https://github.com/ben-manes/concurrentlinkedhashmap) or even better full hog
replace it entirely and migrate to using something like Caffeine
(https://github.com/ben-manes/caffeine)
---
[GitHub] activemq-artemis pull request #2228: ARTEMIS-2017 SelectorParser cache not t...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2228#discussion_r208822183
--- Diff:
artemis-selector/src/main/java/org/apache/activemq/artemis/selector/impl/SelectorParser.java
---
@@ -80,11 +78,15 @@ public static BooleanExpression parse(String sql)
throws FilterException {
StrictParser parser = new StrictParser(new
StringReader(actual));
e = parser.JmsSelector();
}
-cache.put(sql, e);
+synchronized (cache) {
--- End diff --
On that then, i would simply remove the use in the SelectorParser for this
particular case.
Re the actual class, if its used in OpenWireProtocolManager then it would
suggest the LRUCache is being used beyond its original scope and should move
out of its current location into common util, if it is to be re-used in other
area's and then if it is to slowly be used in other areas such us the protocol
manager, it may warrant some future efforts to harden it.
---
[GitHub] activemq-artemis issue #2229: ARTEMIS-2018 - Add bridge events to plugin API
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2229 Looks good to me. Not to affect this PR, but a general comment that we should start thinking about, there now is a case that by having one broker plugin that it will be invoked left right and centre on every intercept, e.g. say i want to have broker plugin to capture something not on the hotpath, the plugin will still be called on the hotpath, as the checks done in the if statement are simply hasBrokerPlugins, i wonder if we could make it some time to have the "hasBrokerPlugins" to have specific types, where when the plugin is added it registers the types its applicable for, and thus later we could have "hasBrokerPluginsBridgeType" and "hasBrokerPluginsBindingType". ---
[GitHub] activemq-artemis pull request #2187: ARTEMIS-1545 Support JMS 2.0 Completion...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2187#discussion_r208837845
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
---
@@ -189,16 +185,24 @@ public void kill() {
this.killed = true;
}
+ private void setHandlers() {
+
sessionChannel.setCommandConfirmationHandler(commandConfirmationHandler);
--- End diff --
@jbertram
So i think to solve the issue of the responseCache still having reference
and the memory leak, we can just add in the code that acks the commands upto
the latest, we can just also call the cache, it will mean a double invocation
but as you noted for the interim with your flag it actually wont have effect.
Thought? Ill send a pr in a bit to your branch
---
[GitHub] activemq-artemis issue #2229: ARTEMIS-2018 - Add bridge events to plugin API
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2229 Cool ill see if i can knock something up. Ill merge this a bit later today if no other commenta ---
[GitHub] activemq-artemis pull request #2231: ARTEMIS-2019 - Seperate ServerPlugin In...
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2231 ARTEMIS-2019 - Seperate ServerPlugin Interfaces Seperate plugin interface by area, all extending a base interface. Update code to check and call only plugins implementing specific interfaces. Existing interface extends all the new interfaces for back compatibility or those who want simplicity and don't care about perf. You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis SeperatePlugins Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2231.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2231 commit 09980c2d5cbce5eae5c06ef963e03d68a674bdd3 Author: Michael André Pearce Date: 2018-08-09T12:43:45Z ARTEMIS-2019 - Seperate ServerPlugin Interfaces Seperate plugin interface by area, all extending a base interface. Update code to check and call only plugins implementing specific interfaces. Existing interface extends all the new interfaces for back compatibility or those who want simplicity and don't care about perf. ---
[GitHub] activemq-artemis issue #2231: ARTEMIS-2019 - Seperate ServerPlugin Interface...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2231 @cshannon this is what i was thinking, in regards to our discussion on https://github.com/apache/activemq-artemis/pull/2229 ---
[GitHub] activemq-artemis pull request #2217: FOR jbertram - ARTEMIS-1545 Memory leak
Github user michaelandrepearce closed the pull request at: https://github.com/apache/activemq-artemis/pull/2217 ---
[GitHub] activemq-artemis pull request #2187: ARTEMIS-1545 Support JMS 2.0 Completion...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2187#discussion_r208923062
--- Diff:
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
---
@@ -189,16 +185,24 @@ public void kill() {
this.killed = true;
}
+ private void setHandlers() {
+
sessionChannel.setCommandConfirmationHandler(commandConfirmationHandler);
--- End diff --
https://github.com/jbertram/activemq-artemis/pull/6
---
[GitHub] activemq-artemis pull request #2232: ARTEMIS-1482 Enhance test to ensure len...
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2232 ARTEMIS-1482 Enhance test to ensure len check is done before byte[] init Set the int to Integer.MAX_VALUE thus if the len check is not done before byte[] initialization the test would blow with an OOM. You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1482 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2232.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2232 commit 5d8079845ee3fe1092b84533ae63438d79ad353c Author: Michael André Pearce Date: 2018-08-09T14:54:32Z ARTEMIS-1482 Enhance test to ensure len check is done before byte[] init Set the int to Integer.MAX_VALUE thus if the len check is not done before byte[] initialization the test would blow with an OOM. ---
[GitHub] activemq-artemis issue #2232: ARTEMIS-1482 Enhance test to ensure len check ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2232 @mtaylor here is updated test that will catch (by the test going OOM) if the byte[] is initialized, and the validity check isnt done first. I checked and unit test fails without your change, but passes with it. ---
[GitHub] activemq-artemis issue #2227: ARTEMIS-1482 Add back check for SimpleString
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2227 @mtaylor see https://github.com/apache/activemq-artemis/pull/2232 please. ---
[GitHub] activemq-artemis issue #2232: ARTEMIS-1482 Enhance test to ensure len check ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2232 @mtaylor yes it will crash, it OOM's, but then at least you know the build isnt good, and would be picked up and avoid it being regressed, which is the point of test suite. ---
[GitHub] activemq-artemis issue #2232: ARTEMIS-1482 Enhance test to ensure len check ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2232 @mtaylor i was editing my comment sorry with the output when your fix would be regressed: The other tests will run when run via maven if thats what you're worried about? Here is example output of the test suite run, when i comment out your fix (so it OOM's) Results : Tests in error: SimpleStringTest.testOutOfBoundsThrownOnMalformedString:36 » OutOfMemory Reque... Tests run: 112, Failures: 0, Errors: 1, Skipped: 0 I think its quite clear and maven nicely outputs ---
[GitHub] activemq-artemis issue #2228: ARTEMIS-2017 SelectorParser cache not thread-s...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2228 LGTM ---
[GitHub] activemq-artemis issue #2199: ARTEMIS-1996 MappedSequentialFileFactory may c...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2199 @franz1981 , @morefuntang provided a testcase to reproduce the issue, i note on your branch you havent copied the test, to prove your alternative change also would fix the issue. Could i suggest you add the same test case and raise a PR ---
[GitHub] activemq-artemis issue #2232: ARTEMIS-1482 Enhance test to ensure len check ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2232 @mtaylor i giggle, its like there are forces out there that just know, literally came into work today, and one of our app teams just experienced this issue this morning... least i know whats the issue :) ---
[GitHub] activemq-artemis issue #2232: ARTEMIS-1482 Enhance test to ensure len check ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2232 @clebertsuconic can this be ported to the 2.6.x branch? ---
[GitHub] activemq-artemis issue #2232: ARTEMIS-1482 Enhance test to ensure len check ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2232 So we have no idea of the cause, it is a consumer so we think its a producer where producee maybe on older version. It happend 4 times since monday our app team said ---
[GitHub] activemq-artemis issue #2237: [ARTEMIS-2022] Create count messages 'group by...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2237 This should probably filter using the selectors logic, rather than some seperate logic, so the fiktering expectations are the same. ---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209312103
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
--- End diff --
Sould parse this to a filter and then later do message match filter check.
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209312787
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
--- End diff --
Look at FilterImpl.createFilter to create the filter.
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209315364
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
--- End diff --
Yes but using a filter to do this allows you to do that and more powerful,
and means people can use the same filtering logic theyre used to and is well
tested
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209317084
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
+ checkStarted();
+ clearIO();
+ try {
+ try (LinkedListIterator iterator =
queue.browserIterator()) {
+Map result = new HashMap<>();
+String propertySearch = filter == null ? UNDEFINED : filter;
+try {
+ while (iterator.hasNext()) {
+ MessageReference ref = iterator.next();
+ String messageProperty =
ref.getMessage().getStringProperty(propertySearch);
--- End diff --
Why only string property
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209369043
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
--- End diff --
Thr point of filter isnt to be the filter currently on thr queue but to
query / filter for this count.
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209369424
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
+ checkStarted();
+ clearIO();
+ try {
+ try (LinkedListIterator iterator =
queue.browserIterator()) {
+Map result = new HashMap<>();
+String propertySearch = filter == null ? UNDEFINED : filter;
+try {
+ while (iterator.hasNext()) {
+ MessageReference ref = iterator.next();
+ String messageProperty =
ref.getMessage().getStringProperty(propertySearch);
--- End diff --
But it isnt being used as such new objects will be created for no reason.
All you want to do is check if the property exists
As i said doing this with a filter to run over to get the counts will be
more powerful and clea er
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209373519
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -63,6 +63,7 @@
public class QueueControlImpl extends AbstractControl implements
QueueControl {
public static final int FLUSH_LIMIT = 500;
+ public static final String UNDEFINED = "*_UNDEFINED_*";
--- End diff --
This isn't needed, if you getobjectproperty and do Object.toString on it,
the java standard representation "null" will return.
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209375108
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
+ checkStarted();
+ clearIO();
+ try {
+ try (LinkedListIterator iterator =
queue.browserIterator()) {
+Map result = new HashMap<>();
+String propertySearch = filter == null ? UNDEFINED : filter;
+try {
+ while (iterator.hasNext()) {
+ MessageReference ref = iterator.next();
+ String messageProperty =
ref.getMessage().getStringProperty(propertySearch);
--- End diff --
You will better calling to get object as then where there maybe properties
that dont actually get stored in the property map but are actually a top level
field it would still return for all. And then you can simply call
Objects.toString on the returned object, this would also then remove the need
for undefined and would return the java normal string representation for null
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209381476
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
+ checkStarted();
+ clearIO();
+ try {
+ try (LinkedListIterator iterator =
queue.browserIterator()) {
+Map result = new HashMap<>();
--- End diff --
Counter should be long, not integer
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209381661
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java
---
@@ -711,6 +712,32 @@ public long countMessages(final String filterStr)
throws Exception {
}
}
+ @Override
+ public String countMessagesProperty(final String filter) throws
Exception {
+ checkStarted();
+ clearIO();
+ try {
+ try (LinkedListIterator iterator =
queue.browserIterator()) {
+Map result = new HashMap<>();
+String propertySearch = filter == null ? UNDEFINED : filter;
+try {
+ while (iterator.hasNext()) {
+ MessageReference ref = iterator.next();
+ String messageProperty =
ref.getMessage().getStringProperty(propertySearch);
+ messageProperty = messageProperty == null ? UNDEFINED :
messageProperty;
+ Integer value = result.getOrDefault(messageProperty, 0);
--- End diff --
Should be SimpleString, thats initialized once.
---
[GitHub] activemq-artemis pull request #2237: [ARTEMIS-2022] Create count messages 'g...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2237#discussion_r209386511
--- Diff:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/QueueControlTest.java
---
@@ -1781,6 +1781,66 @@ public void testCountMessagesWithInvalidFilter()
throws Exception {
session.deleteQueue(queue);
}
+ @Test
+ public void testCountMessagesPropertyExist() throws Exception {
+ String key = new String("key_group");
+ String valueGroup1 = "group_1";
+ String valueGroup2 = "group_2";
+
+ SimpleString address = RandomUtil.randomSimpleString();
+ SimpleString queue = RandomUtil.randomSimpleString();
+
+ session.createQueue(address, queue, null, false);
+ ClientProducer producer = session.createProducer(address);
+
+ for (int i = 0; i < 100; i++) {
+ ClientMessage msg = session.createMessage(false);
+ if(i % 3 == 0){
--- End diff --
checkstyle , need to space if and curly
---
[GitHub] activemq-artemis issue #2237: [ARTEMIS-2022] Create count messages 'group by...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2237 @ArthurFritz i sent your branch a PR with a number of improvements on this, see here: https://github.com/ArthurFritz/activemq-artemis/pull/1 ---
[GitHub] activemq-artemis pull request #2239: ARTEMIS-2022 Create count messages 'gro...
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2239 ARTEMIS-2022 Create count messages 'group by' this property filter Checking build You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-2022 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2239.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2239 commit 259f3f8be9014b79503e1ed0ead8c9598f340ff6 Author: Arthur Fritz Santiago Date: 2018-08-10T14:49:16Z [ARTEMIS-2022] Create count messages 'group by' this property filter commit d7d7f7499cf7ca675f901090e3199ad71bc0f922 Author: Arthur Fritz Santiago Date: 2018-08-10T15:09:32Z [ARTEMIS-2022] Ajust checkstyle commit a01822be0bef5fd09d196a717036819257e280f6 Author: Michael André Pearce Date: 2018-08-10T21:37:26Z ARTEMIS-2022 - Enhancements Fix checkstyle Avoid duplicated logic Ability to filter and group Instantiate SimpleString property key once Get property value via getObjectProprty to ensure all special mapped properties such as in AMQPMessage would return Avoid a custom string to represent null, instead rely on Java's representation "null" by using Objects.toString to get the string value of the property value used to group by. ---
[GitHub] activemq-artemis pull request #2239: ARTEMIS-2022 Create count messages 'gro...
Github user michaelandrepearce closed the pull request at: https://github.com/apache/activemq-artemis/pull/2239 ---
[GitHub] activemq-artemis pull request #2238: ARTEMIS-2023 Support 1x prefixes for JM...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2238#discussion_r209438598
--- Diff:
artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/config/impl/ConnectionFactoryConfigurationImpl.java
---
@@ -623,6 +636,8 @@ public void decode(final ActiveMQBuffer buffer) {
deserializationBlackList =
BufferHelper.readNullableSimpleStringAsString(buffer);
deserializationWhiteList =
BufferHelper.readNullableSimpleStringAsString(buffer);
+
+ enable1xPrefixes = buffer.readBoolean();
--- End diff --
should check buffer has readable bytes before reading, for compatibility if
the buffer is written by a previous version
---
[GitHub] activemq-artemis pull request #2238: ARTEMIS-2023 Support 1x prefixes for JM...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2238#discussion_r209438668
--- Diff:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQSession.java
---
@@ -885,7 +891,7 @@ public TemporaryQueue createTemporaryQueue() throws
JMSException {
}
try {
- ActiveMQTemporaryQueue queue =
ActiveMQDestination.createTemporaryQueue(this);
+ ActiveMQTemporaryQueue queue =
ActiveMQDestination.createTemporaryQueue(this, (enable1xPrefixes ?
PacketImpl.OLD_TEMP_QUEUE_PREFIX.toString() : ""));
--- End diff --
nit: but can avoid a string concatenation that occurs in the new
createTemporaryQueue method for users not enabling, if the if was moved
around, and the existing method called for that.
final ActiveMQTemporaryQueue queue;
if (enable1xPrefixes) {
queue = ActiveMQDestination.createTemporaryQueue(this,
PacketImpl.OLD_TEMP_QUEUE_PREFIX.toString());
} else {
queue = ActiveMQDestination.createTemporaryQueue(this);
}
---
[GitHub] activemq-artemis issue #2240: ARTEMIS-2025 Ensure correct calculation of mes...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2240 @clebertsuconic @mtaylor seems also MessageBodyTest.testBytesMessage is broker due to this change, and any PR build is failing now. ---
[GitHub] activemq-artemis pull request #2242: 2022
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2242 2022 You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis 2022 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2242.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2242 commit 259f3f8be9014b79503e1ed0ead8c9598f340ff6 Author: Arthur Fritz Santiago Date: 2018-08-10T14:49:16Z [ARTEMIS-2022] Create count messages 'group by' this property filter commit d7d7f7499cf7ca675f901090e3199ad71bc0f922 Author: Arthur Fritz Santiago Date: 2018-08-10T15:09:32Z [ARTEMIS-2022] Ajust checkstyle commit fdf3fadac52b94f060e8baff3e475c9f806b4baa Author: Michael André Pearce Date: 2018-08-10T21:37:26Z ARTEMIS-2022 - Enhancements Fix checkstyle Avoid duplicated logic Ability to filter and group Instantiate SimpleString property key once Get property value via getObjectProprty to ensure all special mapped properties such as in AMQPMessage would return Avoid a custom string to represent null, instead rely on Java's representation "null" by using Objects.toString to get the string value of the property value used to group by. commit 803e7021e987601ab62b9f5b09359adec76391a8 Author: Arthur Fritz Santiago Date: 2018-08-11T18:39:42Z Merge pull request #1 from michaelandrepearce/ARTEMIS-2022 ARTEMIS-2022 - Enhancements ---
[GitHub] activemq-artemis pull request #2242: 2022
Github user michaelandrepearce closed the pull request at: https://github.com/apache/activemq-artemis/pull/2242 ---
[GitHub] activemq-artemis pull request #2243: Artemis-2022 create count messages with...
GitHub user michaelandrepearce opened a pull request: https://github.com/apache/activemq-artemis/pull/2243 Artemis-2022 create count messages with group by filter You can merge this pull request into a Git repository by running: $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-2022 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2243.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2243 commit 259f3f8be9014b79503e1ed0ead8c9598f340ff6 Author: Arthur Fritz Santiago Date: 2018-08-10T14:49:16Z [ARTEMIS-2022] Create count messages 'group by' this property filter commit d7d7f7499cf7ca675f901090e3199ad71bc0f922 Author: Arthur Fritz Santiago Date: 2018-08-10T15:09:32Z [ARTEMIS-2022] Ajust checkstyle commit fdf3fadac52b94f060e8baff3e475c9f806b4baa Author: Michael André Pearce Date: 2018-08-10T21:37:26Z ARTEMIS-2022 - Enhancements Fix checkstyle Avoid duplicated logic Ability to filter and group Instantiate SimpleString property key once Get property value via getObjectProprty to ensure all special mapped properties such as in AMQPMessage would return Avoid a custom string to represent null, instead rely on Java's representation "null" by using Objects.toString to get the string value of the property value used to group by. ---
[GitHub] activemq-artemis pull request #2241: Added a Spring Boot Sample integration
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2241#discussion_r209509591
--- Diff: examples/features/standard/spring-boot-integration/pom.xml ---
@@ -0,0 +1,94 @@
+http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ 4.0.0
+
+ org.apache.activemq.examples.broker
+ jms-examples
+ 2.7.0-SNAPSHOT
+
+ spring-boot-integration
+ ActiveMQ Artemis JMS Spring Boot Integration Example
+
+
+
${project.basedir}/../../../..
+
+
+ 2.0.1
+
+ 1.0.2
+
+
+
+
+ org.apache.activemq
+ artemis-amqp-protocol
+ ${project.version}
+
+
+ org.amqphub.spring
+ amqp-10-jms-spring-boot-starter
+ ${amqp-10-jms-spring.version}
+
+
+ org.messaginghub
--- End diff --
This does seem like If this is a spring boot example, and example using
Spring JMS CachingConnection Factory would be more suitable. As spring users
use this.
---
[GitHub] activemq-artemis issue #2237: [ARTEMIS-2022] Create count messages 'group by...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2237 @ArthurFritz as your branch just had the changes i suggested, but also an additonal but un-needed extra merge commit, i merged from the branch i had of yours, https://github.com/apache/activemq-artemis/pull/2243. Could you close this? ---
[GitHub] activemq-artemis issue #2237: [ARTEMIS-2022] Create count messages 'group by...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2237 @ArthurFritz thanks for the contribution :) ---
