[GitHub] activemq-artemis issue #2402: NO-JIRA: Clarify journal-buffer-timeout docume...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2402 I know, either way, this is just to update the documentation to say that 0 will disable the timed buffer ---
[GitHub] activemq-artemis pull request #2402: NO-JIRA: Clarify journal-buffer-timeout...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2402 NO-JIRA: Clarify journal-buffer-timeout documentation Clarify how to disable the buffer timeout. On one of my test brokers I disabled journal-sync-transactional and journal-sync-non-transactional as data loss is not a big deal. In this scenario I just want to write directly to the file and not use a timed buffer and let the OS handle data syncing, however it was not clear on how to accomplish this. Looking at the code I discovered setting the timeout to 0 will disable the buffer so I'm adding a comment in case others want to do the same. You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis persistenceDocUpdate Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2402.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 #2402 ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 Yes you need the latest client as well. This feature works by retrieving the value from the server as part of a query on connect. The old client doesn't know to read the default configured value. ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 This is only available for version 2.7.0 which has not been released yet. There is documentation available for it as well that will be released with it ---
[GitHub] activemq-artemis issue #2278: ARTEMIS-2052 Fixing initial credit negotiation
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2278 @clebertsuconic - besides the test fix it looks good to me, thanks for fixing it ---
[GitHub] activemq-artemis issue #2278: ARTEMIS-2052 Fixing initial credit negotiation
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2278 The issue is here: https://github.com/apache/activemq-artemis/blob/106e71bdf4f1cf7ddf0dfc37ce6bab76d5c00dad/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/client/ConsumerWindowSizeTest.java#L1457 That line should now change to: assertTrue(Wait.waitFor(() -> cons.getAvailableCredits().get() == consumer.getClientWindowSize() * 2, 5000, 500)); And that will fix it ---
[GitHub] activemq-artemis issue #2278: ARTEMIS-2052 Fixing initial credit negotiation
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2278 It looks like one of tests I added in ConsumerWindowSizeTest brokertestConsumerWindowSizeAddressSettingsDifferentAddressAndQueueName ---
[GitHub] activemq-artemis pull request #2267: ARTEMIS-2052 - QueueQuery should use ad...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2267 ARTEMIS-2052 - QueueQuery should use address name for address settings The name used for looking up address settings for a queue now uses the address name if there is a local queue binding You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-2052 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2267.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 #2267 commit f3b0e3ae9e17f877794ec3d2202e0c55589c8112 Author: Christopher L. Shannon (cshannon) Date: 2018-08-24T15:02:22Z ARTEMIS-2052 - QueueQuery should use address name for address settings The name used for looking up address settings for a queue now uses the address name if there is a local queue binding ---
[GitHub] activemq-artemis issue #2262: ARTEMIS-1663 fix disparity on messagecount
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2262 @michaelandrepearce - Do you have an actual use case or example where it's not reliable? The new metric gets used for size stuff as well so I don't think it should be changed for consistency and should be fixed if there is an issue However I also see you closed this out so is there not actually an issue here? ---
[GitHub] activemq-artemis issue #2258: ARTEMIS-2044 Add onSendError, onMessageRouteEr...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2258 @calohmn - ah ok, yeah that makes sense I didn't look closely enough so if it's spaces then all should be good ---
[GitHub] activemq-artemis issue #2258: ARTEMIS-2044 Add onSendError, onMessageRouteEr...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2258 Adding exception callbacks seems good to me, one thing I noticed is a lot of white space changes. My guess is your IDE is set to use tabs instead of spaces so I would fix that before merging ---
[GitHub] activemq-artemis issue #2231: ARTEMIS-2019 - Seperate ServerPlugin Interface...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2231 Looks good to me ---
[GitHub] activemq-artemis issue #2229: ARTEMIS-2018 - Add bridge events to plugin API
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2229 I like the idea of using separate interfaces and having ActiveMQServerPlugin extend them. This way if someone doesn't care they can make it easy and just extend ActiveMQServerPlugin (as well as keep backwards compatibility) or the user can choose to pick specific interfaces for performance. ---
[GitHub] activemq-artemis pull request #2229: ARTEMIS-2018 - Add bridge events to plu...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2229 ARTEMIS-2018 - Add bridge events to plugin API Add callbacks to handle bridge events including beforeDeliverBridge, afterDeliverBridge and afterAcknowledgeBridge You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-2018 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2229.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 #2229 commit 95ff39f6bfae9c51e6c3c1d1ac2782f60f5d2daa Author: Christopher L. Shannon (cshannon) Date: 2018-08-08T18:10:22Z ARTEMIS-2018 - Add bridge events to plugin API Add callbacks to handle bridge events including beforeDeliverBridge, afterDeliverBridge and afterAcknowledgeBridge ---
[GitHub] activemq-artemis issue #2206: ARTEMIS-2003 - Add bridge metrics
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2206 Done, test is running now ---
[GitHub] activemq-artemis issue #2206: ARTEMIS-2003 - Add bridge metrics
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2206 @jbertram - patch updated...not sure why CI keeps failing but the tests pass on my machine ---
[GitHub] activemq-artemis issue #2206: ARTEMIS-2003 - Add bridge metrics
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2206 @jbertram - renaming the metrics is no problem, I had just used what 5.x had called them but it's fine to rename them for consistency with other existing metrics I added the map primarily because when getting the aggregate metrics for a cluster the metrics are computed by iterating over all the bridges. This was a way to only have to iterate 1 time and not for every call. However, I like your idea of just having both the map and individual calls exposed so I will do that. ---
[GitHub] activemq-artemis issue #2206: ARTEMIS-2003 - Add bridge metrics
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2206 @franz1981 - makes sense with the API hints if that change is done in other cases to make it known to developers the semantics...in this case i think we are good as no change should be made ---
[GitHub] activemq-artemis issue #2206: ARTEMIS-2003 - Add bridge metrics
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2206 I don't think that change can be made. The metrics for the dequeue case on the send acknowledge in this case can be updated in another runnable (new thread) Also, honestly I don't know that it's a great idea to start making this change everywhere if it only works for a single thread because in the future if someone refactors something and it no longer is a single thread for the writer then the metrics will be broken. ---
[GitHub] activemq-artemis pull request #2206: ARTEMIS-2003 - Add bridge metrics
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2206 ARTEMIS-2003 - Add bridge metrics This commit adds support for tracking metrics for bridges for both normal bridges and bridges that are part of a cluster. The two statistics added in this commit are enqueue count and dequeue count but more can be added later. You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-2003 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2206.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 #2206 commit 098f92d5b16527ebdb817b5210b034adf2f7e3e7 Author: Christopher L. Shannon (cshannon) Date: 2018-07-31T17:01:32Z ARTEMIS-2003 - Add bridge metrics This commit adds support for tracking metrics for bridges for both normal bridges and bridges that are part of a cluster. The two statistics added in this commit are enqueue count and dequeue count but more can be added later. ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 @clebertsuconic and @michaelandrepearce - i rebased against master and it looks like the tests all pass so this should be good to merge ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 I don't know what the latest test failure is but the tests pass on my machine. The travis CI build seems to be pretty inconsistent with random failures. ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 I had to add a setting to one of the tests so hopefully that fixes it, will find out in a few minutes ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 @clebertsuconic - Is this ok to merge? ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 Ok latest changes have been pushed which includes removing the new version and using a nullable integer instead ---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2191#discussion_r204867142
--- 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 --
So for V4 just use a hash map for all values? And then have all the getters
reference the map? That would be fine with me especially because
@michaelandrepearce mentioned maybe adding other settings as well...while we
could add some settings now there will always be more changes
I can make the change tomorrow morning
---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2191#discussion_r204867293
--- 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 --
Or actually do you just want to keep existing stuff the way it is and only
use the Map for new values starting with defaultConsumerWindowSize?
---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 @clebertsuconic and @michaelandrepearce - PR has been updated ---
[GitHub] activemq-artemis issue #2191: ARTEMIS-1987 - Add consumer window size to Add...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2191 @clebertsuconic - My use case is for the "service provider" case where the administrators want to protect a shared resource broker that services many different customers and fine tune settings. In this case there is often many different clients competing for resources and an administrator may want to tune the broker to lower the window size for slow or bad clients, etc. This doesn't prevent the user from overriding the setting on their client if they really want to change the window size of course. While it's probably not the most common use case it's the scenario I deal with the most and it is very useful coming from a 5.x broker where prefetch can be configured per policy. ---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2191#discussion_r204819939
--- 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 --
@michaelandrepearce - yes I could probably just extend the
SessionQueueQueryResponseMessage and create a
SessionQueueQueryResponseMessage_V4 and add that info to it...I will go ahead
and do that and rework this
---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user cshannon commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2191#discussion_r204819904 --- 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 -- I can change it back but I was using -1 to figure out whether or not the value was set. The issue is that if someone manually wants to set the client to the same value as that constant then it will always be overriden by the default from the server. The idea is if the client sets the value that should be used insteadNot sure how to get around that without adding another flag to indicate the user overrode the value (or set it to null by default) ---
[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2191 ARTEMIS-1987 - Add consumer window size to AddressSettings Support configuring a default consumer window size via AddressSettings which will allow sensible defaults to be used by address type You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1987 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2191.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 #2191 commit 878b53e455e402ea0de0d64dcd53550f16273664 Author: Christopher L. Shannon (cshannon) Date: 2018-04-09T12:20:57Z ARTEMIS-1987 - Add consumer window size to AddressSettings Support configuring a default consumer window size via AddressSettings which will allow sensible defaults to be used by address type ---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2116 The build looks good now ---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2116 I don't think it's related as the test it failed on doesn't have a plugin registered. I tried to see if I could trigger it to re-run the build in travis ci but i didn't see a way to do that. ---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2116 @clebertsuconic - is this ok to merge? ---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2116 I removed the re-throw and the expect for the JMSException so it will only pass on InvalidClientIDException as that is the only type that should be thrown ---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2116#discussion_r191788973
--- Diff:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/SessionMetadataAddExceptionTest.java
---
@@ -75,6 +88,24 @@ public void testInvalidClientIdSetFactory() throws
Exception {
cf.createConnection();
}
+ @Test(timeout = 5000, expected = JMSException.class)
+ public void testDuplicateClientIdSet() throws Exception {
+ ActiveMQConnectionFactory activeMQConnectionFactory =
(ActiveMQConnectionFactory) cf;
+ Connection con = cf.createConnection();
+ Connection con2 = cf.createConnection();
+ try {
+ con.setClientID("valid");
+ con2.setClientID("valid");
+ fail("Should have failed for duplicate clientId");
+ } catch (Exception e) {
--- End diff --
I also added another test to verify that ActiveMQException bubbles up and
is converted to a JMSException by the client
---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2116#discussion_r191787441
--- Diff:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/SessionMetadataAddExceptionTest.java
---
@@ -75,6 +88,24 @@ public void testInvalidClientIdSetFactory() throws
Exception {
cf.createConnection();
}
+ @Test(timeout = 5000, expected = JMSException.class)
+ public void testDuplicateClientIdSet() throws Exception {
+ ActiveMQConnectionFactory activeMQConnectionFactory =
(ActiveMQConnectionFactory) cf;
+ Connection con = cf.createConnection();
+ Connection con2 = cf.createConnection();
+ try {
+ con.setClientID("valid");
+ con2.setClientID("valid");
+ fail("Should have failed for duplicate clientId");
+ } catch (Exception e) {
--- End diff --
I switched it to catch the InvalidClientIdException that gets thrown
---
[GitHub] activemq-artemis issue #2116: ARTEMIS-1895 - Add duplicate metadata failure ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2116 @michaelandrepearce - good point, i fixed it to bubble up the ActiveMQException and so it is handled properly by the client ---
[GitHub] activemq-artemis pull request #2116: ARTEMIS-1895 - Add duplicate metadata f...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2116 ARTEMIS-1895 - Add duplicate metadata failure callback to ActiveMQServerPlugin Add a callback on duplicate metadata which will allow extra functionality to be added. You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1895 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2116.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 #2116 commit afba4982b661759b3e7f1c6a2e4095fbc4c5a1dd Author: Christopher L. Shannon (cshannon) Date: 2018-05-30T12:50:05Z ARTEMIS-1895 - Add duplicate metadata failure callback to ActiveMQServerPlugin Add a callback on duplicate metadata which will allow extra functionality to be added. ---
[GitHub] activemq-artemis issue #2109: ARTEMIS-1888 - Add forceSSLParameters flag to ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2109 It's fine to wait for next week ---
[GitHub] activemq-artemis issue #2109: ARTEMIS-1888 - Add forceSSLParameters flag to ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2109 I updated the message...when are you going to cut 2.6.1? If it's going to be within a couple of days then ok otherwise we should probably just cherry-pick because it will hold up people form pushing their commits. ---
[GitHub] activemq-artemis pull request #2109: ARTEMIS-1888 - Fix SSL order of precede...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2109 ARTEMIS-1888 - Fix SSL order of precedence in core client Local connection SSL settings should be preferred over javax.ssl settings You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1888 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2109.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 #2109 commit 6aca83dfe64c661ecb29d07e0676a7698f450293 Author: Christopher L. Shannon (cshannon) Date: 2018-05-24T16:04:56Z ARTEMIS-1888 - Fix SSL order of precedence in core client Local connection SSL settings should be preferred over javax.ssl settings ---
[GitHub] activemq-artemis issue #2044: ARTEMIS-1829 Remove deprecated plugin's messag...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2044 Ah yep you are right so there is no change needed. ---
[GitHub] activemq-artemis issue #2044: ARTEMIS-1829 Remove deprecated plugin's messag...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2044 @franz1981 - You should probably also update the tests as well. The MethodCalledVerifier class implements ActiveMQServerPlugin in the integration tests project and should probably have its messageExpired method signature updated as well although it should still work as is with your change. ---
[GitHub] activemq-artemis issue #2044: ARTEMIS-1829 Remove deprecated plugin's messag...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2044 Oops, yeah this is the correct fix. The new default method would call off to the old one as it's default if nothing overrides it to stay backwards compatible. And then with Artemis 3.0.0 we can remove all the deprecated default methods altogether. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Pass ServerConsumer to messageE...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @franz1981 and @michaelandrepearce - thanks for taking a look ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Pass ServerConsumer to messageE...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @franz1981 and @michaelandrepearce - I updated the patch to pass a reference to the ServerConsumer (when it applies) for both the expired and acked callbacks and dropped the extra reference to the serverId in the MessageReference classlet me know what you think ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 I understand wanting to keep memory low but there's a point where you take things too far. At some point usability and correctness is more important. Having just a consumerId is just not correct, end of story. It is not a unique value and will be duplicated. The client library is responsible for creating that consumerId per session so there's no way to go back and make the consumerId unique (can't change old client libraries). I wish there were, that would solve this problem. I do not agree that refactoring is the best approach. At the end of the day if we can't add a reference to a String I think we are taking trying to limit memory usage too far. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @michaelandrepearce - because 1) there can be more than just that use case for having sessionId part of the reference in the future and 2) the acknowledge code is not part of the consumer and is handled later by the QueueImpl...see the acknowledge method inside QueueImpl..it just gets the reference and does the acking and doesn't know the consumer by that point because the ServerConsumer calls ack on the ref but doesn't pass itself to it...there would have to be a good amount of refactoring to change this along with changes to public interfaces ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 For example, it's a common use case to fire notifications or logging when a message is acknowledged. I have a requirement to do this for my organization and I need to quickly track the specific consumer that acked the message from inside the plugin. Having the sessionId as part of the reference is the only way to do this. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @franz1981 - So what size would you recommend? Seems like maybe 8 is too much to add to the estimation based on the output. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 Furthermore there is more to a broker than just processing. Monitoring and metrics are very important to business flows and trying to save a few bytes of memory is not worth it if you sacrifice basic things such as being able to do proper logging and troubleshooting. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 It's not attached to the message. The reference has the information for which consumer is attached to it. The consumer id is not unique so you need to have the session Id as well otherwise there is no way to find out which consumer the reference belongs to so this has to be in memory. I really don't think it's going to be a big deal to add a reference id. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @franz1981 - If you don't mind double checking with the tool again I would appreciate it since you have already validated the memory before but if you don't have time I can try and figure it out ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 I updated the PR and added 8 to the estimated size in MessageRefereceImpl. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @franz1981 - Did you mean probably "would" be enough? The string is a UUID (which is normally 16 bytes I think) but it is owned by the ServerSessionImpl so it would just be a reference so however much memory that takes up (maybe 8 bytes to be safe?) ---
[GitHub] activemq-artemis pull request #2012: ARTEMIS-1803 - Add sessionId to Message...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2012 ARTEMIS-1803 - Add sessionId to MessageReference Track the sessionId along with the consumerId in a MessageReference when appropriate in order to figure out which consumer the reference belongs to You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1803 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2012.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 #2012 commit 262cac9f0b8b581b56bda6219bd43b7922edf6c9 Author: Christopher L. Shannon (cshannon) Date: 2018-04-12T11:05:16Z ARTEMIS-1803 - Add sessionId to MessageReference Track the sessionId along with the consumerId in a MessageReference when appropriate in order to figure out which consumer the reference belongs to ---
[GitHub] activemq-artemis pull request #2009: ARTEMIS-1800 - Fix metrics decrement on...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2009#discussion_r180774964
--- Diff:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/QueueControlTest.java
---
@@ -2510,6 +2510,39 @@ public void testSendMessage() throws Exception {
Assert.assertEquals(new String(body), "theBody");
}
+ @Test
+ public void testGetScheduledCountOnRemove() throws Exception {
+ long delay = Integer.MAX_VALUE;
+ SimpleString address = RandomUtil.randomSimpleString();
+ SimpleString queue = RandomUtil.randomSimpleString();
+
+ session.createQueue(address, RoutingType.MULTICAST, queue, null,
durable);
+
+ QueueControl queueControl = createManagementControl(address, queue);
+ Assert.assertEquals(0, queueControl.getScheduledCount());
+
+ Field queueMemorySizeField =
QueueImpl.class.getDeclaredField("queueMemorySize");
+ queueMemorySizeField.setAccessible(true);
+ final LocalQueueBinding binding = (LocalQueueBinding)
server.getPostOffice().getBinding(queue);
+ Queue q = binding.getQueue();
+ AtomicInteger queueMemorySize1 = (AtomicInteger)
queueMemorySizeField.get(q);
+ assertTrue(queueMemorySize1.get() == 0);
+
+ ClientProducer producer = session.createProducer(address);
+ ClientMessage message = session.createMessage(durable);
+ message.putLongProperty(Message.HDR_SCHEDULED_DELIVERY_TIME,
System.currentTimeMillis() + delay);
+ producer.send(message);
+
+ queueControl.removeAllMessages();
+
+ Assert.assertEquals(0, queueControl.getMessageCount());
--- End diff --
I think the removeAllMessages() call is synchronous but if it's an issue
it's easy enough to switch out the assertion in the test
---
[GitHub] activemq-artemis pull request #2009: ARTEMIS-1800 - Fix metrics decrement on...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2009 ARTEMIS-1800 - Fix metrics decrement on scheduled message cancel The queue metrics were being decremented improperly because on iteration over the cancelled scheduled messages because the flag for fromMessageReferences was not set to false. Setting the flag to false skips over the metrics update which is what we want as the scheduled messages were never added to the message references in the first place so the metrics don't need updating You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1800 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2009.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 #2009 commit 70f0908b4ee64e68059a21009db9dd3236c26732 Author: Christopher L. Shannon (cshannon) Date: 2018-04-10T19:54:29Z ARTEMIS-1800 - Fix metrics decrement on scheduled message cancel The queue metrics were being decremented improperly because on iteration over the cancelled scheduled messages because the flag for fromMessageReferences was not set to false. Setting the flag to false skips over the metrics update which is what we want as the scheduled messages were never added to the message references in the first place so the metrics don't need updating ---
[GitHub] activemq-artemis pull request #2008: ARTEMIS-1800 - fix duplicate metrics up...
Github user cshannon closed the pull request at: https://github.com/apache/activemq-artemis/pull/2008 ---
[GitHub] activemq-artemis pull request #2008: ARTEMIS-1800 - fix duplicate metrics up...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2008 ARTEMIS-1800 - fix duplicate metrics update on scheduled message cancel When removing scheduled messages from a queue the scheduled message metrics were being decremented twice You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1800 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2008.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 #2008 commit 122782ae460a789c883beb21bfccb80089d9bb59 Author: Christopher L. Shannon (cshannon) Date: 2018-04-10T19:06:28Z ARTEMIS-1800 - fix duplicate metrics update on scheduled message cancel When removing scheduled messages from a queue the scheduled message metrics were being decremented twice ---
[GitHub] activemq-artemis issue #2007: ARTEMIS-1799 - Add a NotificationActiveMQServe...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2007 @clebertsuconic - thanks for merging ---
[GitHub] activemq-artemis issue #2007: ARTEMIS-1799 - Add a NotificationActiveMQServe...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2007 I don't understand the confusion. That call just calls off to any registered plugins if they exist. This commit is a brand new plugin that implements the afterDeliver call and fires off a notification. But it only fires off that notification if the flag in the plugin is set to true. ---
[GitHub] activemq-artemis pull request #2007: ARTEMIS-1799 - Add a NotificationActive...
Github user cshannon commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2007#discussion_r180449915 --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/CoreNotificationType.java --- @@ -39,7 +39,15 @@ PROPOSAL(18), PROPOSAL_RESPONSE(19), UNPROPOSAL(20), - CONSUMER_SLOW(21); + CONSUMER_SLOW(21), + ADDRESS_ADDED(22), + ADDRESS_REMOVED(23), + CONNECTION_CREATED(24), + CONNECTION_DESTROYED(25), + SESSION_CREATED(26), + SESSION_CLOSED(27), + MESSAGE_DELIVERED(28), --- End diff -- No because every notification is off by default. The user can configure what they want. By default if you configure the plugin it doesn't do anything unless the user sets a type to true to send. So if someone doesn't care about message delivered notifications then just leave that flag as false. ---
[GitHub] activemq-artemis pull request #2007: ARTEMIS-1799 - Add a NotificationActive...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/2007 ARTEMIS-1799 - Add a NotificationActiveMQServerPlugin Adds a new plugin that will support sending new types of notifications for broker events which will allow enhanced broker monitoring You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1799 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2007.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 #2007 commit 4795f7c6d0e9242c2fc1f364b1cc025a1ce037b6 Author: Christopher L. Shannon (cshannon) Date: 2018-04-10T13:19:11Z ARTEMIS-1799 - Add a NotificationActiveMQServerPlugin Adds a new plugin that will support sending new types of notifications for broker events which will allow enhanced broker monitoring ---
[GitHub] activemq-artemis issue #1959: ARTEMIS-1754 LargeServerMessageImpl.toString()...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1959 Shouldn't we just have the getPersistentSize() method open/close the encoder? The idea is that this method might be called by anyone as it is public so this is a bigger issue than just in the toString call ---
[GitHub] activemq-artemis issue #1925: ARTEMIS-1727 - Make sure transport is stopped ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1925 @tabish121 - PR has been updated to use the broker's scheduled pool ---
[GitHub] activemq-artemis pull request #1925: ARTEMIS-1727 - Make sure transport is s...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1925#discussion_r171924792
--- Diff:
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java
---
@@ -641,13 +647,37 @@ public void fail(ActiveMQException me, String
message) {
}
}
try {
- protocolManager.removeConnection(this.getConnectionInfo(), me);
+ if (this.getConnectionInfo() != null) {
+protocolManager.removeConnection(this.getConnectionInfo(), me);
+ }
} catch (InvalidClientIDException e) {
ActiveMQServerLogger.LOGGER.warn("Couldn't close connection
because invalid clientID", e);
}
shutdown(true);
}
+ private void delayedStop(final int waitTime, final String reason,
Throwable cause) {
+ if (waitTime > 0) {
+ try {
+server.getExecutorFactory().getExecutor().execute(new
Runnable() {
+ @Override
+ public void run() {
+ try {
+ Thread.sleep(waitTime);
--- End diff --
I won't have time today but I will fix my patch and push it up first thing
monday morning.
---
[GitHub] activemq-artemis pull request #1925: ARTEMIS-1727 - Make sure transport is s...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1925#discussion_r171921837
--- Diff:
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java
---
@@ -641,13 +647,37 @@ public void fail(ActiveMQException me, String
message) {
}
}
try {
- protocolManager.removeConnection(this.getConnectionInfo(), me);
+ if (this.getConnectionInfo() != null) {
+protocolManager.removeConnection(this.getConnectionInfo(), me);
+ }
} catch (InvalidClientIDException e) {
ActiveMQServerLogger.LOGGER.warn("Couldn't close connection
because invalid clientID", e);
}
shutdown(true);
}
+ private void delayedStop(final int waitTime, final String reason,
Throwable cause) {
+ if (waitTime > 0) {
+ try {
+server.getExecutorFactory().getExecutor().execute(new
Runnable() {
+ @Override
+ public void run() {
+ try {
+ Thread.sleep(waitTime);
--- End diff --
Agreed, I will make that change and use a scheduled executor instead.
---
[GitHub] activemq-artemis pull request #1925: ARTEMIS-1727 - Make sure transport is s...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/1925 ARTEMIS-1727 - Make sure transport is stopped on failed OpenWire connection To prevent a socket from hanging open by a bad client the broker should make sure to stop the transport if a connection attempt fails by an OpenWire client You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1727 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/1925.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 #1925 commit 8de2fccc883aa3a532e540e5e700e2c668e4e289 Author: Christopher L. Shannon (cshannon) Date: 2018-03-02T16:38:07Z ARTEMIS-1727 - Make sure transport is stopped on failed OpenWire connection To prevent a socket from hanging open by a bad client the broker should make sure to stop the transport if a connection attempt fails by an OpenWire client ---
[GitHub] activemq-artemis pull request #1923: ARTEMIS-1726 - check proper permissions...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/1923 ARTEMIS-1726 - check proper permissions when using OpenWire Ensure that on queue creation and deletion that the proper permissions are checked You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1726 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/1923.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 #1923 commit 17b0e6547ab848fd0f61b109abddd8c2e8c506e0 Author: Christopher L. Shannon (cshannon) Date: 2018-02-28T21:28:55Z ARTEMIS-1726 - check proper permissions when using OpenWire Ensure that on queue creation and deletion that the proper permissions are checked ---
[GitHub] activemq-artemis pull request #1910: ARTEMIS-1713 - Fix NPE inside OpenWireC...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/1910 ARTEMIS-1713 - Fix NPE inside OpenWireConnection fix NPE inside getClientId() in OpenWireConnection You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1713 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/1910.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 #1910 commit 8bd0d8aad684872273e0d3e34a280a5ebe0e531e Author: Christopher L. Shannon (cshannon) Date: 2018-02-28T19:01:51Z ARTEMIS-1713 - Fix NPE inside OpenWireConnection fix NPE inside getClientId() in OpenWireConnection ---
[GitHub] activemq-artemis pull request #1908: ARTEMIS-1711 - Fix openwire exlusive di...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/1908 ARTEMIS-1711 - Fix openwire exlusive divert Fixing the failure on send from an OpenWire producer when an exclusive divert exists You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1711 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/1908.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 #1908 commit 3237ef9f0de9653ed70e8bba8ae09bcdbebaa011 Author: Christopher L. Shannon (cshannon) Date: 2018-02-28T14:30:36Z ARTEMIS-1711 - Fix openwire exlusive divert Fixing the failure on send from an OpenWire producer when an exclusive divert exists ---
[GitHub] activemq-artemis issue #1903: ARTEMIS-1706 - Add support for wantClientAuth
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1903 @clebertsuconic and @tabish121 - PR has been updated and rebased. ---
[GitHub] activemq-artemis issue #1903: ARTEMIS-1706 - Add support for wantClientAuth
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1903 I am going to update my PR with Timâs suggestion to have it match 5.x and rebase in the morning ---
[GitHub] activemq-artemis pull request #1903: ARTEMIS-1706 - Add support for wantClie...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/1903 ARTEMIS-1706 - Add support for wantClientAuth Support setting wantClientAuth on a netty acceptor You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1706 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/1903.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 #1903 commit d52397a8db814dc7d470c2d4bf049c680713d969 Author: Christopher L. Shannon (cshannon) Date: 2018-02-27T14:47:36Z ARTEMIS-1706 - Add support for wantClientAuth Support setting wantClientAuth on a netty acceptor ---
[GitHub] activemq-artemis pull request #1886: ARTEMIS-1695 - Improve STOMP compatibli...
GitHub user cshannon opened a pull request: https://github.com/apache/activemq-artemis/pull/1886 ARTEMIS-1695 - Improve STOMP compatiblity with 5.x clients Also make sure on authentication error in version 1.0 a client will disconnect You can merge this pull request into a Git repository by running: $ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1695 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/1886.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 #1886 commit e93fbf25fcf12319aa7ad07c49bbb1d7798bc681 Author: Christopher L. Shannon (cshannon) Date: 2018-02-22T15:28:57Z ARTEMIS-1695 - Improve STOMP compatiblity with 5.x clients Also make sure on authentication error in version 1.0 a client will disconnect ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 Had to update the PR again as something got messed up when rebasing ---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1853#discussion_r166700570
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java
---
@@ -439,7 +453,7 @@ public void confirmPosition(final Transaction tx, final
PagePosition position) t
public void ackTx(final Transaction tx, final PagedReference reference)
throws Exception {
confirmPosition(tx, reference.getPosition());
- counter.increment(tx, -1);
+ counter.increment(tx, -1, -getPersistentSize(reference));
--- End diff --
@michaelandrepearce - So because of this line here we can't really default
the persistentSize to -1, it needs to be 0. This call here will store a new
PageCountRecordInc() record to the journal with a negative count and negative
size on message ack to decrement the counters. Because of this a negative can
actually be a valid value and really 0 should mean not set (we should never
have a message size of 0)
---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 @clebertsuconic - I did some cleanup work and I added a couple of basic tests to the compatibility test. It doesn't do much but show that the current snapshot can load a 2.4 journal successfully (and it can load the paging store as well). We can expand the tests with more if you want. Also, I removed the persistentSize from the PageCountPendingImpl class as the size (and the count apparently) aren't actually used and not needed. On recovery we can recover the size from the paged reference. Do you think this is ready to merge? ---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1853#discussion_r166632761
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/codec/PageCountPendingImpl.java
---
@@ -62,21 +67,41 @@ public long getPageID() {
return pageID;
}
+ /**
+* @return the persistentSize
+*/
+ @Override
+ public long getPersistentSize() {
+ return persistentSize;
+ }
+
+ /**
+* @param persistentSize the persistentSize to set
+*/
+ public void setPersistentSize(long persistentSize) {
+ this.persistentSize = persistentSize;
+ }
+
@Override
public int getEncodeSize() {
- return DataConstants.SIZE_LONG * 2;
+ return DataConstants.SIZE_LONG * 3;
}
@Override
public void encode(ActiveMQBuffer buffer) {
buffer.writeLong(queueID);
buffer.writeLong(pageID);
+ buffer.writeLong(persistentSize);
--- End diff --
I can add a compatibility test. The value will just be a default
(@michaelandrepearce wants me to use -1 instead of 0 and i guess we just would
ignore it if not set for the count).
Where's the journal test located in the test suite?
---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1853#discussion_r166630379
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/codec/PageCountPendingImpl.java
---
@@ -62,21 +67,41 @@ public long getPageID() {
return pageID;
}
+ /**
+* @return the persistentSize
+*/
+ @Override
+ public long getPersistentSize() {
+ return persistentSize;
+ }
+
+ /**
+* @param persistentSize the persistentSize to set
+*/
+ public void setPersistentSize(long persistentSize) {
+ this.persistentSize = persistentSize;
+ }
+
@Override
public int getEncodeSize() {
- return DataConstants.SIZE_LONG * 2;
+ return DataConstants.SIZE_LONG * 3;
}
@Override
public void encode(ActiveMQBuffer buffer) {
buffer.writeLong(queueID);
buffer.writeLong(pageID);
+ buffer.writeLong(persistentSize);
--- End diff --
@clebertsuconic - here is an example of where the size is now persisted
---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 @clebertsuconic - The metric in the page counters is persisted along with the page counters now. When the page counters are written to the journal that metric is encoded to disk and reloaded on restart. Older versions of the journal are compatible, they will just not have that size value loaded as it won't have been saved previously. ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 Oh ok I get it now. Yeah you are right that the PagingStore interface has a method to get the number of pages and the page size which can be used to get an idea of consumption. This value is exposed through AddressControl so users can see it. ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 @clebertsuconic - I don't think we need to re-calculate anything if the values are missing for paging. The point of this metric isn't to give the user information on storage consumption as that data already exists. They can find out how many page files are being used and disk usage already, etc. The point of these metrics is to know exactly how much data on a queue is left to consume. So there might be a 5 megabyte page file but only 2 messages left to consume and the rest of the file is just waiting to be GC'd. This metric only includes the 2 messages to be consumed and not the rest of the file. ---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/1853#discussion_r166601621 --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/codec/PageCountRecordInc.java --- @@ -26,17 +26,20 @@ private int value; + private long persistentSize; --- End diff -- @michaelandrepearce - That is fine I will make those values -1, but I don't think we would try and re-calculate a correct value. When loading the journal you'd have to re-load the messages to calculate from the paging store which we don't want to do. I think the proper behavior is to just ignore it and not use the value in the calculations. Then over time as new records come in the values would be filled in and correct. If I change the value to -1 on the record, are you thinking we should just have the getter do the negative check and return 0 if it's -1 or were you thinking to just return -1 from the getter and then check for negative everywhere else the value is used? ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 I updated the PR with getPersistentSize() everywhere along with comments to try and make it more clear what the value represented. I can tweak it some more if people still think it's not clear. ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 Ok I can just use getPersistentSize() then for everything, I will update the PR shortly. ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 @clebertsuconic - hmm so I'm starting to like you original idea of just using getEncodeSize() more as the the message might be non-durable and in that case the encode size is still used but it isn't on disk so that would be a little misleading. However there is still the issue of what to do aboutt large messages? Maybe we need a name like fullEncodeSize() or something so that it is implies it is the full message including the body? ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 getPersistentSize() is fine with me as it specifies that the size represents the entire persisted size of the message ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 @clebertsuconic - Fair enough, but I think I should call it getMessageSize() instead (i already did this in a couple places) The reason why I would use that name is because the value is slightly different than getEncodeSize(). For the normal message case, getMessageSize() and geEncodeSize() are equal. But for a large message getMessageSize() will be getEncodeSize() + the body size. ---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1853#discussion_r166401530
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/codec/PageCountPendingImpl.java
---
@@ -43,6 +44,8 @@ public PageCountPendingImpl(long queueID, long pageID,
int inc) {
long pageID;
+ long size;
--- End diff --
@michaelandrepearce - did you have in mind having the getter in the class
handle the -1 (ie have the getSize() method check for -1 and return 0 if
negative) or having the getSize() method return the -1 as is and then have the
code using the value explicitly do the check for negative?
---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1853#discussion_r166388367
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/codec/PageCountPendingImpl.java
---
@@ -43,6 +44,8 @@ public PageCountPendingImpl(long queueID, long pageID,
int inc) {
long pageID;
+ long size;
--- End diff --
The issue is backwards compatibility. These values are now being stored to
the journal and old records won't have a size. So by defaulting to a 0 value
for when an old record is it makes everything simple. Yes I could make it -1
but then I have to add checks for -1 versus just using 0 as a default and then
the logic is clean as we don't need to handle negative edge cases.
---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1853#discussion_r166386744
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/codec/PageCountPendingImpl.java
---
@@ -43,6 +44,8 @@ public PageCountPendingImpl(long queueID, long pageID,
int inc) {
long pageID;
+ long size;
--- End diff --
Why would we use -1 and not 0? These values default to 0 because we don't
want the counter to change if the size is not set. If we default to -1 then
the counters will go negative where as if they are 0 when not set then nothing
changes with the counters which is what we want.
---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 @clebertsuconic - No, I'm not trying to measure the size on the JVM. I want to measure the size of the messages in general (including how much space is on disk) as a metric to know how much data is left to consume on the Queue. The most accurate way is to use the encode size as this is the amount of space taken up on disk. For large messages this also includes the body size. ---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 @michaelandrepearce - ok made those 2 changes so it is ready for another look ---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/1853#discussion_r166346352 --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/PagedReferenceImpl.java --- @@ -53,6 +53,8 @@ private Object protocolData; + private Long messageSize; --- End diff -- Yep, I'll set this to -1 to mean not set. ---
[GitHub] activemq-artemis pull request #1853: ARTEMIS-1663 - Add new message count an...
Github user cshannon commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1853#discussion_r166346269
--- Diff:
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java
---
@@ -1309,4 +1336,68 @@ public void remove() {
public void close() {
}
}
+
+
+ private static class PagePositionSize {
--- End diff --
I will fix this and extend PagePosition as if the position is incremented
that is fine as the size will just go back to 0 unless set.
---
[GitHub] activemq-artemis issue #1853: ARTEMIS-1663 - Add new message count and size ...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/1853 @michaelandrepearce - I updated my PR, take a look and let me know what you think. ---
