[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2799: ARTEMIS-2453 Fixing deadLock between destroyQueue and removeAddressInfo
michaelandrepearce commented on a change in pull request #2799: ARTEMIS-2453
Fixing deadLock between destroyQueue and removeAddressInfo
URL: https://github.com/apache/activemq-artemis/pull/2799#discussion_r314583108
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##
@@ -484,7 +482,7 @@ public QueueBinding updateQueue(SimpleString name,
Long delayBeforeDispatch,
SimpleString user,
Boolean configurationManaged) throws
Exception {
- synchronized (addressLock) {
+ synchronized (this) {
Review comment:
Why not make synchronized method for these methods if the whole block is
syncd?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on issue #2750: ARTEMIS-2399 Improve performance when there are a lot of subscribers
wy96f commented on issue #2750: ARTEMIS-2399 Improve performance when there are a lot of subscribers URL: https://github.com/apache/activemq-artemis/pull/2750#issuecomment-521883667 @michaelandrepearce Can you wait a moment? When the browse cursor iterators(like the one created by QueueControl::browse, browse only consumer, etc) are closed, the page opened by them is not closed. I would push another commit to fix this. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2750: ARTEMIS-2399 Improve performance when there are a lot of subscribers
michaelandrepearce commented on issue #2750: ARTEMIS-2399 Improve performance when there are a lot of subscribers URL: https://github.com/apache/activemq-artemis/pull/2750#issuecomment-521882489 @wy96f can you squash commits so we can merge? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2785: ARTEMIS-2442 throw ActiveMQResourceLimitException if session/queue limit is reached
michaelandrepearce commented on a change in pull request #2785: ARTEMIS-2442
throw ActiveMQResourceLimitException if session/queue limit is reached
URL: https://github.com/apache/activemq-artemis/pull/2785#discussion_r314581735
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/ActiveMQExceptionType.java
##
@@ -261,6 +261,12 @@ public ActiveMQException createException(String msg) {
public ActiveMQException createException(String msg) {
return new ActiveMQReplicationTimeooutException(msg);
}
+ },
+ RESOURCE_LIMIT_REACHED(221) {
Review comment:
People can use native core client. Not everyone uses jms.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on a change in pull request #2785: ARTEMIS-2442 throw ActiveMQResourceLimitException if session/queue limit is reached
wy96f commented on a change in pull request #2785: ARTEMIS-2442 throw
ActiveMQResourceLimitException if session/queue limit is reached
URL: https://github.com/apache/activemq-artemis/pull/2785#discussion_r314566368
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/ActiveMQResourceLimitException.java
##
@@ -0,0 +1,30 @@
+/*
+ * 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.api.core;
+
+public final class ActiveMQResourceLimitException extends ActiveMQException {
+
Review comment:
Do you mean ActiveMQResourceLimitException extends
ActiveMQSessionCreationException, and with RESOURCE_LIMIT_REACHED
ActiveMQExceptionType? If so, older client still can't understand new exception
by new ActiveMQExceptionType code when the code is passed back to client.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on a change in pull request #2785: ARTEMIS-2442 throw ActiveMQResourceLimitException if session/queue limit is reached
wy96f commented on a change in pull request #2785: ARTEMIS-2442 throw
ActiveMQResourceLimitException if session/queue limit is reached
URL: https://github.com/apache/activemq-artemis/pull/2785#discussion_r314566358
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/ActiveMQExceptionType.java
##
@@ -261,6 +261,12 @@ public ActiveMQException createException(String msg) {
public ActiveMQException createException(String msg) {
return new ActiveMQReplicationTimeooutException(msg);
}
+ },
+ RESOURCE_LIMIT_REACHED(221) {
Review comment:
For older client that don't understand, "ActiveMQException"(type code is
GENERIC_EXCEPTION) would be decoded and thrown. There are two places where
ActiveMQExceptionType.SESSION_CREATION_REJECTED is judged, one is
ActiveMQSessionContext::recreateSession(), the other is BridgeImpl::connect().
For both of them, the logic is same in the case of GENERIC_EXCEPTION(old
client) and RESOURCE_LIMIT_EXCEEDED(new client).
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on a change in pull request #2791: ARTEMIS-2450 page-size-bytes should not be greater than Integer.MAX_VALUE
wy96f commented on a change in pull request #2791: ARTEMIS-2450 page-size-bytes
should not be greater than Integer.MAX_VALUE
URL: https://github.com/apache/activemq-artemis/pull/2791#discussion_r314557230
##
File path:
artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/FileConfigurationParserTest.java
##
@@ -257,6 +257,26 @@ public void
testParsingDefaultServerConfigWithENCMaskedPwd() throws Exception {
assertEquals("helloworld", bconfig.getPassword());
}
+ @Test
+ public void testParsingOverflowPageSize() throws Exception {
+ FileConfigurationParser parser = new FileConfigurationParser();
Review comment:
> E.g. other sizes are quite happily long currently also. Why is this special
I see in Page::write()/Page::read() all operations are limited to int range,
such as PagingStoreImpl::currentPageSize, Page::size, and local variables like
fileSize, processedBytes in Page::readFromSequentialFile(), etc.
> This test, test the new change, but doesnt test for what we are protecting
from. E.g whats the underlying issue being fixed
I'm not sure what you mean. We're protecting page-size-bytes from being
greater than Integer.MAX_VALUE(2147483647). So if we set page-size-bytes to
2147483648, the broker is expected to fail fast to throw exception , correct?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2798: NO-JIRA fix failing Travis build
clebertsuconic commented on issue #2798: NO-JIRA fix failing Travis build URL: https://github.com/apache/activemq-artemis/pull/2798#issuecomment-521799929 For that reason I only use the scripts. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2798: NO-JIRA fix failing Travis build
clebertsuconic commented on issue #2798: NO-JIRA fix failing Travis build URL: https://github.com/apache/activemq-artemis/pull/2798#issuecomment-521799868 @michaelandrepearce did you use the merge button on github? Be careful with merge commits, they are the worst. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] asfgit merged pull request #2799: ARTEMIS-2453 Fixing deadLock between destroyQueue and removeAddressInfo
asfgit merged pull request #2799: ARTEMIS-2453 Fixing deadLock between destroyQueue and removeAddressInfo URL: https://github.com/apache/activemq-artemis/pull/2799 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] jbertram commented on issue #2792: NO-JIRA: Javadoc for ActiveMQServerMessagePlugin interface
jbertram commented on issue #2792: NO-JIRA: Javadoc for ActiveMQServerMessagePlugin interface URL: https://github.com/apache/activemq-artemis/pull/2792#issuecomment-521689923 I think what you wrote looks good. I wouldn't go as far as saying that using a `ThreadLocal` is a *good* idea, but it should work nonetheless. :) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #2799: ARTEMIS-2453 Fixing deadLock between destroyQueue and removeAddressInfo
clebertsuconic commented on a change in pull request #2799: ARTEMIS-2453 Fixing
deadLock between destroyQueue and removeAddressInfo
URL: https://github.com/apache/activemq-artemis/pull/2799#discussion_r314370749
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##
@@ -649,9 +647,7 @@ public AddressInfo removeAddressInfo(SimpleString address,
boolean force) throws
@Override
public AddressInfo getAddressInfo(SimpleString addressName) {
- synchronized (addressLock) {
- return addressManager.getAddressInfo(addressName);
- }
+ return addressManager.getAddressInfo(addressName);
Review comment:
@jbertram thanks.. that confirms what I thought.
I'm running the whole testsuite to confirm it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #2799: ARTEMIS-2453 Fixing deadLock between destroyQueue and removeAddressInfo
jbertram commented on a change in pull request #2799: ARTEMIS-2453 Fixing
deadLock between destroyQueue and removeAddressInfo
URL: https://github.com/apache/activemq-artemis/pull/2799#discussion_r314367295
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##
@@ -649,9 +647,7 @@ public AddressInfo removeAddressInfo(SimpleString address,
boolean force) throws
@Override
public AddressInfo getAddressInfo(SimpleString addressName) {
- synchronized (addressLock) {
- return addressManager.getAddressInfo(addressName);
- }
+ return addressManager.getAddressInfo(addressName);
Review comment:
I don't see a need for the `synchronized (addressLock)` given that the
underlying data structure in the address manager is a `ConcurrentHashMap` and
no other operations are being done (unlike in the other places which use
`addressLock`). I think this fix looks good.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] jbertram commented on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache
jbertram commented on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache URL: https://github.com/apache/activemq-artemis/pull/2796#issuecomment-521683921 It's *possible* that `tempQueues` could grow like `knownDestinations`. However, entries are only added to `tempQueues` for temporary destinations that the client actually creates and entries from `tempQueues` get removed when the corresponding temporary destination is deleted from the broker. An entry is added to `knownDestinations` for every unique destination to which a client sends a message and those entries *never* get removed unless they are temporary destinations which the client itself deletes. In my mind these facts make the pathological growth much less likely. Also, in JMS terms the lifetime of a temporary destination is actually the lifetime of the *connection* that created it, not the session. The temporary destinations need to be tracked somehow so that the client implementation can delete them implicitly when the connection is closed in the case that the application doesn't delete them explicitly. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #2799: ARTEMIS-2453 Fixing deadLock between destroyQueue and removeAddressInfo
clebertsuconic commented on a change in pull request #2799: ARTEMIS-2453 Fixing
deadLock between destroyQueue and removeAddressInfo
URL: https://github.com/apache/activemq-artemis/pull/2799#discussion_r314363282
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
##
@@ -649,9 +647,7 @@ public AddressInfo removeAddressInfo(SimpleString address,
boolean force) throws
@Override
public AddressInfo getAddressInfo(SimpleString addressName) {
- synchronized (addressLock) {
- return addressManager.getAddressInfo(addressName);
- }
+ return addressManager.getAddressInfo(addressName);
Review comment:
@jbertram What do you think about this?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #2799: ARTEMIS-2453 Fixing deadLock between destroyQueue and removeAddressInfo
clebertsuconic opened a new pull request #2799: ARTEMIS-2453 Fixing deadLock between destroyQueue and removeAddressInfo URL: https://github.com/apache/activemq-artemis/pull/2799 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] jdanekrh commented on issue #2792: NO-JIRA: Javadoc for ActiveMQServerMessagePlugin interface
jdanekrh commented on issue #2792: NO-JIRA: Javadoc for ActiveMQServerMessagePlugin interface URL: https://github.com/apache/activemq-artemis/pull/2792#issuecomment-521680441 (Does it look reasonable so far, what I wrote? Especially that using ThreadLocal is a good idea?) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq] coheigea opened a new pull request #389: AMQ-7275 - Update Commons BeanUtils
coheigea opened a new pull request #389: AMQ-7275 - Update Commons BeanUtils URL: https://github.com/apache/activemq/pull/389 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] cjwmorgan-sol commented on issue #9: [PoC] Testing toolkit
cjwmorgan-sol commented on issue #9: [PoC] Testing toolkit URL: https://github.com/apache/activemq-nms-amqp/pull/9#issuecomment-521632602 Hey I found that the provider sends transfer frames on a sender link as settled even when the link sender settled mode is Unsettled. This is against amqp spec. So far I've been able to find at least one broker that detachs the sender link when a non-persistent(settled) message is sent. Sender links that support fire and forget settled message shoul dhave a send settle mode of Mixed not Unsettled. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce edited a comment on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache
michaelandrepearce edited a comment on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache URL: https://github.com/apache/activemq-artemis/pull/2796#issuecomment-521563091 @jbertram re: > BTW, the issue I saw wasn't a leak, per se. It was simply unwanted accumulation based on the way it was designed. Won't we get similar issue on the tempoary queue set thats used to track temp queues? I guess that could probably move into the session object, so theyre cleaned up as a session is closed, which is anyhow the lifetime of a temporary queue anyhow. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce edited a comment on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache
michaelandrepearce edited a comment on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache URL: https://github.com/apache/activemq-artemis/pull/2796#issuecomment-521563091 @jbertram re: > BTW, the issue I saw wasn't a leak, per se. It was simply unwanted accumulation based on the way it was designed. Won't we get similar issue on the tempoary queue set thats used to track temp queues in connection? I guess that could probably move into the session object, so theyre cleaned up as a session is closed, which is anyhow the lifetime of a temporary queue anyhow. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce edited a comment on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache
michaelandrepearce edited a comment on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache URL: https://github.com/apache/activemq-artemis/pull/2796#issuecomment-521563091 @jbertram re: > BTW, the issue I saw wasn't a leak, per se. It was simply unwanted accumulation based on the way it was designed. Won't we get similar issue on the tempoary queue set thats used to track temp queues? I guess that could probably move into the session object, so theyre cleaned up as a session is closed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
franz1981 edited a comment on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521563027 Yes, sure and will provide proper test coverage too as it deserves :) Thanks for the suggestions Michael!! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache
michaelandrepearce commented on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache URL: https://github.com/apache/activemq-artemis/pull/2796#issuecomment-521563091 @jbertram re: > BTW, the issue I saw wasn't a leak, per se. It was simply unwanted accumulation based on the way it was designed. Won't we get similar issue on the tempoary queue set thats used to track temp queues? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
franz1981 commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521563027 Yes, sure and will provide proper yes and coverage too as it deserves :) Thanks for the suggestions Michael!! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache
michaelandrepearce commented on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache URL: https://github.com/apache/activemq-artemis/pull/2796#issuecomment-521562542 @jbertram this was a quick scrap togeather of what i was trying to describe in review comment on your pr. Im happy to complete it fully for you, but it wont be till next week. Im more than happy if you wish to just take it and complete it (e.g. dont worry about keeping my authorship) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce edited a comment on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache
michaelandrepearce edited a comment on issue #2796: ARTEMIS-2451 remove need for knownDestination Cache URL: https://github.com/apache/activemq-artemis/pull/2796#issuecomment-521562542 @jbertram this was a quick scrap togeather of what i was trying to describe in review comment on your pr. Im happy to complete it fully for you, but it wont be till next week. Im more than happy if you need this done faster and wish to just take it and complete it (e.g. dont worry about keeping my authorship) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2798: NO-JIRA fix failing Travis build
michaelandrepearce commented on issue #2798: NO-JIRA fix failing Travis build URL: https://github.com/apache/activemq-artemis/pull/2798#issuecomment-521561991 Nice +1 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce merged pull request #2798: NO-JIRA fix failing Travis build
michaelandrepearce merged pull request #2798: NO-JIRA fix failing Travis build URL: https://github.com/apache/activemq-artemis/pull/2798 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521560596 @franz1981 i assume you're going to have a think on this and re-work bits? e.g. no further comments needed for now right? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
franz1981 commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521559226 I understand your comment now, my comment was related to the code itself,not the feature as a whole: I see that there exists cases where it can work.. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
franz1981 commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521557724 The feature of shared store colocated is not totally broken, but the shared store colocation using group names, yes. But is normal, if the were not using at all group names in their logic I believe that's ok.. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
franz1981 commented on a change in pull request #2793: ARTEMIS-2452 group-name
ignored in shared store colocated setup
URL: https://github.com/apache/activemq-artemis/pull/2793#discussion_r314213953
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/ha/ColocatedHAManager.java
##
@@ -163,6 +174,23 @@ private synchronized boolean
activateSharedStoreBackup(String journalDirectory,
return true;
}
+ private TopologyMember validateBackupGroupName(SimpleString nodeID) {
+ // Older versions of artemis don't send the nodeID in
BackupRequestMessage:
+ // in this case we cannot trust the request, making the requesting server
Review comment:
It was working fine because you haven't had any other member of topology
with a group name and been lucky, I believe :P
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce edited a comment on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce edited a comment on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521556891 Well it can't be totally broken, theres users out there using it (mostly because they are forming single groups or ensuring clusters are rign fenced, therefor group name isnt really mandatory there, yes theres a bug if you need multiple, and thats not ideal, but its not totally broken as you say, This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521556891 Well it can't be totally broken, theres users out there using it (mostly because they are forming single groups or ensuring clusters are rign fenced), yes theres a bug thats not ideal. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
franz1981 commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521556443 The version proposal is probably the best solution indeed: the reason why the old logic breaks the new one is that the old one just ignored the group names and let any requesting server that arrive first to be able to form a pair on any group name regardless both the requesting server group name and the target server. It was just totally broken... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
franz1981 edited a comment on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521556443 The version proposal is probably the best solution indeed: the reason why the old logic breaks the new one is that the old one just ignores the group names and let any requesting server that arrive first to be able to form a pair on any group name regardless both the requesting server group name and the target server. It was just totally broken... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce commented on a change in pull request #2793: ARTEMIS-2452
group-name ignored in shared store colocated setup
URL: https://github.com/apache/activemq-artemis/pull/2793#discussion_r314212795
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/ha/ColocatedHAManager.java
##
@@ -163,6 +174,23 @@ private synchronized boolean
activateSharedStoreBackup(String journalDirectory,
return true;
}
+ private TopologyMember validateBackupGroupName(SimpleString nodeID) {
+ // Older versions of artemis don't send the nodeID in
BackupRequestMessage:
+ // in this case we cannot trust the request, making the requesting server
Review comment:
Why can't we, we know we had old servers and setups working fine, as such
why be so strict, simply fail back (or even tter have a v1 and v2 version of
the message, that way if all v2 cluster you can be fully strict, else in a
v1/v2 mix you can be less strict
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce commented on a change in pull request #2793: ARTEMIS-2452
group-name ignored in shared store colocated setup
URL: https://github.com/apache/activemq-artemis/pull/2793#discussion_r314211592
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/BackupRequestMessage.java
##
@@ -34,12 +34,14 @@ public BackupRequestMessage() {
}
public BackupRequestMessage(int backupSize,
+ SimpleString nodeID,
String journalDirectory,
String bindingsDirectory,
String largeMessagesDirectory,
String pagingDirectory) {
super(BACKUP_REQUEST);
this.backupSize = backupSize;
+ this.nodeID = nodeID;
Review comment:
Please make a v2 version, this would be a breaking change, for anyone with
existing clusters
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce commented on a change in pull request #2793: ARTEMIS-2452
group-name ignored in shared store colocated setup
URL: https://github.com/apache/activemq-artemis/pull/2793#discussion_r314211592
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/BackupRequestMessage.java
##
@@ -34,12 +34,14 @@ public BackupRequestMessage() {
}
public BackupRequestMessage(int backupSize,
+ SimpleString nodeID,
String journalDirectory,
String bindingsDirectory,
String largeMessagesDirectory,
String pagingDirectory) {
super(BACKUP_REQUEST);
this.backupSize = backupSize;
+ this.nodeID = nodeID;
Review comment:
Please make a v2 version, this would be a breaking change, for anyone with
existing clusters
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521554151 Also why would having old logic break new? Another option is to version the message, and if you get v1 do old logic, if you get new v2 message do new. Then no need even for a flag. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce edited a comment on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce edited a comment on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521552397 Why not have a setting that makes it behave as old version, then once all servers uograded the setting can change to strict..making the flag dynamic would also mean the last change can be done without need for further bounces This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521552397 Why not have a setting that makes it behave as old version, then once all servers uograded the setting can change to strict and then simply another round or rolling bounces. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] jdanekrh commented on issue #2792: NO-JIRA: Javadoc for ActiveMQServerMessagePlugin interface
jdanekrh commented on issue #2792: NO-JIRA: Javadoc for ActiveMQServerMessagePlugin interface URL: https://github.com/apache/activemq-artemis/pull/2792#issuecomment-521549275 Ok, I can move it there. I saw some java classes with quite extensive Javadoc, and I felt that explaining how user provided methods are called belongs in a Javadoc. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup
michaelandrepearce commented on issue #2793: ARTEMIS-2452 group-name ignored in shared store colocated setup URL: https://github.com/apache/activemq-artemis/pull/2793#issuecomment-521533815 Its important broker can be rolling upgraded, e.g. old broker instance can talk with new without issue so zero downtime upgrades can occur. I think the issue you called out must be addressed before this can merge, if it breaks that. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2785: ARTEMIS-2442 throw ActiveMQResourceLimitException if session/queue limit is reached
michaelandrepearce commented on a change in pull request #2785: ARTEMIS-2442
throw ActiveMQResourceLimitException if session/queue limit is reached
URL: https://github.com/apache/activemq-artemis/pull/2785#discussion_r314189923
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/ActiveMQResourceLimitException.java
##
@@ -0,0 +1,30 @@
+/*
+ * 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.api.core;
+
+public final class ActiveMQResourceLimitException extends ActiveMQException {
+
Review comment:
Probably should be an extension of the existing
ActiveMQSessionCreationException so any old code with an explicit catch clause
still gets caught
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2785: ARTEMIS-2442 throw ActiveMQResourceLimitException if session/queue limit is reached
michaelandrepearce commented on a change in pull request #2785: ARTEMIS-2442
throw ActiveMQResourceLimitException if session/queue limit is reached
URL: https://github.com/apache/activemq-artemis/pull/2785#discussion_r314189516
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/ActiveMQExceptionType.java
##
@@ -261,6 +261,12 @@ public ActiveMQException createException(String msg) {
public ActiveMQException createException(String msg) {
return new ActiveMQReplicationTimeooutException(msg);
}
+ },
+ RESOURCE_LIMIT_REACHED(221) {
Review comment:
What occurs with older clients that dont understand this new exception if
this new exception is thrown and passed back to them
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2791: ARTEMIS-2450 page-size-bytes should not be greater than Integer.MAX_VALUE
michaelandrepearce commented on a change in pull request #2791: ARTEMIS-2450
page-size-bytes should not be greater than Integer.MAX_VALUE
URL: https://github.com/apache/activemq-artemis/pull/2791#discussion_r314189108
##
File path:
artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/FileConfigurationParserTest.java
##
@@ -257,6 +257,26 @@ public void
testParsingDefaultServerConfigWithENCMaskedPwd() throws Exception {
assertEquals("helloworld", bconfig.getPassword());
}
+ @Test
+ public void testParsingOverflowPageSize() throws Exception {
+ FileConfigurationParser parser = new FileConfigurationParser();
Review comment:
E.g. other sizes are quite happily long currently also. Why is this special
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2791: ARTEMIS-2450 page-size-bytes should not be greater than Integer.MAX_VALUE
michaelandrepearce commented on a change in pull request #2791: ARTEMIS-2450
page-size-bytes should not be greater than Integer.MAX_VALUE
URL: https://github.com/apache/activemq-artemis/pull/2791#discussion_r314188891
##
File path:
artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/FileConfigurationParserTest.java
##
@@ -257,6 +257,26 @@ public void
testParsingDefaultServerConfigWithENCMaskedPwd() throws Exception {
assertEquals("helloworld", bconfig.getPassword());
}
+ @Test
+ public void testParsingOverflowPageSize() throws Exception {
+ FileConfigurationParser parser = new FileConfigurationParser();
Review comment:
This test, test the new change, but doesnt test for what we are protecting
from. E.g whats the underlying issue being fixed
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2791: ARTEMIS-2450 page-size-bytes should not be greater than Integer.MAX_VALUE
michaelandrepearce commented on a change in pull request #2791: ARTEMIS-2450
page-size-bytes should not be greater than Integer.MAX_VALUE
URL: https://github.com/apache/activemq-artemis/pull/2791#discussion_r314188595
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/settings/impl/AddressSettings.java
##
@@ -505,6 +505,9 @@ public long getPageSizeBytes() {
}
public AddressSettings setPageSizeBytes(final long pageSize) {
+ if (pageSize > Integer.MAX_VALUE) {
+ throw new IllegalArgumentException("pageSize must be < " +
Integer.MAX_VALUE);
Review comment:
Why not change long to int, this would then be cleaner from anyone trying to
use the method
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2750: ARTEMIS-2399 Improve performance when there are a lot of subscribers
michaelandrepearce commented on issue #2750: ARTEMIS-2399 Improve performance when there are a lot of subscribers URL: https://github.com/apache/activemq-artemis/pull/2750#issuecomment-521531183 @franz1981 @wy96f @clebertsuconic as no further comments/feedback will merge this tomorrow. Last call :) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
