[GitHub] activemq-artemis pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user asfgit closed the pull request at: https://github.com/apache/activemq-artemis/pull/2191 ---
[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 #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 clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2191#discussion_r204881984
--- 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 existing stuff the way it is... and use the map for new values.
Anyway.. that's a suggestion... if you don't have time to do it now I will
merge it as is... let me know?
---
[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 pull request #2191: ARTEMIS-1987 - Add consumer window size...
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2191#discussion_r204860714
--- 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 --
@cshannon just wondering.. wouldn't be worth to invest here to return a
hashMap instead?
We keep creating V4, V5... if we start using a Map, we wouldn't need to
change the wire for this ever again.
---
[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 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 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 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 ---
