[GitHub] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250776531
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
e.g. no point calling, clientSession.createAddress(address,
RoutingType.ANYCAST, true);
if its not allowed
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250776567
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
I agree with you. although It wasn't part of my original change.. I was just
fixing a bug
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250776970
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
let me rewrite this
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] ehsavoie commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
ehsavoie commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250783079
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
We run WildFly test suite with a security manager on (which is how this leak
was found) BUT I don't have the runs where this happened so I don't know the
symptoms :( for a test ensure that this is resolved.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250812517
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/QueueAutoCreationTest.java
##
@@ -114,7 +114,7 @@ public void testHugeString() throws Exception {
}
- @Test(timeout = 3)
+ @Test //(timeout = 3)
Review comment:
? is this now broken
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250812517
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/QueueAutoCreationTest.java
##
@@ -114,7 +114,7 @@ public void testHugeString() throws Exception {
}
- @Test(timeout = 3)
+ @Test //(timeout = 3)
Review comment:
? is this now broker?
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin
michaelandrepearce commented on a change in pull request #2516: ARTEMIS-2192
fix listener for LegacyLDAPSecuritySettingPlugin
URL: https://github.com/apache/activemq-artemis/pull/2516#discussion_r250764887
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/LegacyLDAPSecuritySettingPlugin.java
##
@@ -333,14 +333,14 @@ private void processSearchResult(Map>
securityRoles,
String destination = null;
String destinationType = "unknown";
List rdns = searchResultLdapName.getRdns();
- if (rdns.size() != 3) {
+ if (rdns.size() < 3) {
if (logger.isDebugEnabled()) {
logger.debug("\tSkipping unexpected search result with " +
rdns.size() + " RDNs.");
}
return;
}
// we can count on the RNDs being in order from right to left
- Rdn rdn = rdns.get(0);
+ Rdn rdn = rdns.get(rdns.size() - 3);
Review comment:
fair enough
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250779232
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
Review comment:
this is missing adding the destination to known destinations, (another bug
introduced by the original change)
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250779232
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
Review comment:
this is missing added the destination (another bug introduced by the
original change)
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250825292
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I mean we need a test in ActiveMQ Artemis.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250809081
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
Review comment:
why not simply throw exception here, then on debugging from logs with stack
trace it would make it easier. maybe having a cleaner exception message for
users, e.g.
"Queue " + address + " does not exist, auto create addresses is enabled, but
auto create queues is not."
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250809792
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
Review comment:
add else clause here with throws:
then on debugging from logs with stack trace it would make it easier, as
would get line where it errored out so could figure out. Also could then allow
a better description so end users understand better, e.g.
depending on destination.isQueue()
"Queue " + address + " does not exist, auto create addresses is disabled."
"Topic " + address + " does not exist, auto create addresses is disabled."
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250810104
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
+
+
+// Second we create the queue, but we only do it if the address
was created
+if (destination.isQueue() && addressExists) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ if (addressQuery.isAutoCreateQueues()) {
+ try {
+if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, addressQuery);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, addressQuery);
+}
+ } catch (ActiveMQQueueExistsException thatsOK) {
+// nothing to be done
+ }
+ } else {
+ throw new InvalidDestinationException("Queue " + address
+ " does not exist");
Review comment:
make this more descriptive: "Queue " + address + " does not exist, address
exists, but auto create queues is disabled."
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250814957
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
+
+
+// Second we create the queue, but we only do it if the address
was created
+if (destination.isQueue() && addressExists) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ if (addressQuery.isAutoCreateQueues()) {
+ try {
+if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, addressQuery);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, addressQuery);
+}
+ } catch (ActiveMQQueueExistsException thatsOK) {
+// nothing to be done
+ }
+ } else {
+ throw new InvalidDestinationException("Queue " + address
+ " does not exist");
+ }
+ }
+}
+
+if (!addressExists) {
Review comment:
-1 lets keep that exceptions are thrown from explicit points where checks
are done and fail, so can be more explicit and also we can debug where the
check fail occured by stack trace lines.
also general comment as this is JMS interface, we should use words like
Queue, Topic or Destination
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250814957
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
+
+
+// Second we create the queue, but we only do it if the address
was created
+if (destination.isQueue() && addressExists) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ if (addressQuery.isAutoCreateQueues()) {
+ try {
+if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, addressQuery);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, addressQuery);
+}
+ } catch (ActiveMQQueueExistsException thatsOK) {
+// nothing to be done
+ }
+ } else {
+ throw new InvalidDestinationException("Queue " + address
+ " does not exist");
+ }
+ }
+}
+
+if (!addressExists) {
Review comment:
lets keep that exceptions are thrown from explicit points where checks are
done and fail, so can be more explicit and also we can debug where the check
fail occured by stack trace lines.
also as this is JMS interface, we should use words like Queue, Topic or
Destination in exception messages
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250814957
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
+
+
+// Second we create the queue, but we only do it if the address
was created
+if (destination.isQueue() && addressExists) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ if (addressQuery.isAutoCreateQueues()) {
+ try {
+if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, addressQuery);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, addressQuery);
+}
+ } catch (ActiveMQQueueExistsException thatsOK) {
+// nothing to be done
+ }
+ } else {
+ throw new InvalidDestinationException("Queue " + address
+ " does not exist");
+ }
+ }
+}
+
+if (!addressExists) {
Review comment:
lets keep that exceptions are thrown from explicit points where checks are
done and fail, so can be more explicit and also we can debug where the check
fail occured by stack trace lines.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250814957
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
+
+
+// Second we create the queue, but we only do it if the address
was created
+if (destination.isQueue() && addressExists) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ if (addressQuery.isAutoCreateQueues()) {
+ try {
+if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, addressQuery);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, addressQuery);
+}
+ } catch (ActiveMQQueueExistsException thatsOK) {
+// nothing to be done
+ }
+ } else {
+ throw new InvalidDestinationException("Queue " + address
+ " does not exist");
+ }
+ }
+}
+
+if (!addressExists) {
Review comment:
-1 lets keep that exceptions are thrown from explicit points where checks
are done and fail, so can be more explicit and also we can debug where the
check fail occured by stack trace lines.
also general comment as this is JMS interface, we should use words like
Queue, Topic or Destination in exception messages
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] asfgit closed pull request #2519: ARTEMIS-2238 Fixing QueueQuery on every single send on topics
asfgit closed pull request #2519: ARTEMIS-2238 Fixing QueueQuery on every single send on topics URL: https://github.com/apache/activemq-artemis/pull/2519 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250777278
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
@michaelandrepearce I just wnated to show you an idea... but this is wrong
now
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250808877
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/QueueAutoCreationTest.java
##
@@ -133,6 +133,36 @@ public void testAutoCreateOnTopic() throws Exception {
Assert.assertTrue(((ActiveMQConnection)connection).containsKnownDestination(addressName));
}
+ @Test //(timeout = 3)
+ // QueueAutoCreationTest was created to validate auto-creation of queues
+ // and this test was added to validate a regression:
https://issues.apache.org/jira/browse/ARTEMIS-2238
+ public void testAutoCreateOnAddressOnly() throws Exception {
Review comment:
this is an incomplete test.. I will finish this before merging
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250808742
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,58 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
Review comment:
I don't think it would be an issue.. but I have ammended it...
I will create a test..
let me run tests on it.. please don't merge this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] asfgit merged pull request #17: AMQCPP-643: Add an option to time out connection attempts when blocked in ensureConnectionInfoSent
asfgit merged pull request #17: AMQCPP-643: Add an option to time out connection attempts when blocked in ensureConnectionInfoSent URL: https://github.com/apache/activemq-cpp/pull/17 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250761629
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I'd like to see a test case on this. I think the issue might be we have some
ThreadFactories which are not using the ActiveMQThreadFactory which should imo
ensure the privileged access if its used in replacement for those, and actually
we just need to fix those up. A test though is the best way for sure, so what
ever fix, we know its resolved.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on issue #2519: ARTEMIS-2238 Fixing QueueQuery on every single send on topics
michaelandrepearce commented on issue #2519: ARTEMIS-2238 Fixing QueueQuery on every single send on topics URL: https://github.com/apache/activemq-artemis/pull/2519#issuecomment-457346877 Have left some comments This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2519: ARTEMIS-2238 Fixing QueueQuery on every single send on topics
michaelandrepearce commented on a change in pull request #2519: ARTEMIS-2238
Fixing QueueQuery on every single send on topics
URL: https://github.com/apache/activemq-artemis/pull/2519#discussion_r250767135
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -423,16 +423,18 @@ private void doSendx(ActiveMQDestination destination,
throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
- ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
- if (queueQuery.isExists()) {
- connection.addKnownDestination(address);
- } else if (destination.isQueue() &&
query.isAutoCreateQueues()) {
- if (destination.isTemporary()) {
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
- } else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+ if (destination.isQueue()) {
Review comment:
should also be:
query.isAutoCreateQueues()
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250776531
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
e.g. no point calling, clientSession.createAddress(address,
RoutingType.ANYCAST, true);
if its now allowed
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2518: NO-JIRA fix race condition in QueueQueryTest
clebertsuconic commented on a change in pull request #2518: NO-JIRA fix race
condition in QueueQueryTest
URL: https://github.com/apache/activemq-artemis/pull/2518#discussion_r250747119
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/QueueQueryTest.java
##
@@ -155,19 +156,20 @@ public void testQueueQueryOnAutoCreatedQueueWithFQQN()
throws Exception {
SimpleString addressName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString queueName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString fqqn = addressName.concat("::").concat(queueName);
- JMSContext c = new ActiveMQConnectionFactory("vm://0").createContext();
- c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
- QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- queueQueryResult = server.queueQuery(queueName);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
- assertEquals(2, server.queueQuery(fqqn).getMessageCount());
- assertEquals(2, server.queueQuery(queueName).getMessageCount());
+ try (JMSContext c = new
ActiveMQConnectionFactory("vm://0").createContext()) {
+ c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
+ QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ Wait.assertEquals(1, () -> server.queueQuery(fqqn).getMessageCount());
+ queueQueryResult = server.queueQuery(queueName);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
+ Wait.assertEquals(2, () -> server.queueQuery(fqqn).getMessageCount());
Review comment:
Couldn't it be:
Wait.assertEquals(2, server.queueQuery(fqqn)::getMessageCount);
or you actually needed to perform a query every iteration on the loop?
no biggie.. I'm going to merge it regardless... just telling you so you know.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2518: NO-JIRA fix race condition in QueueQueryTest
clebertsuconic commented on a change in pull request #2518: NO-JIRA fix race
condition in QueueQueryTest
URL: https://github.com/apache/activemq-artemis/pull/2518#discussion_r250747842
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/QueueQueryTest.java
##
@@ -155,19 +156,20 @@ public void testQueueQueryOnAutoCreatedQueueWithFQQN()
throws Exception {
SimpleString addressName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString queueName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString fqqn = addressName.concat("::").concat(queueName);
- JMSContext c = new ActiveMQConnectionFactory("vm://0").createContext();
- c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
- QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- queueQueryResult = server.queueQuery(queueName);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
- assertEquals(2, server.queueQuery(fqqn).getMessageCount());
- assertEquals(2, server.queueQuery(queueName).getMessageCount());
+ try (JMSContext c = new
ActiveMQConnectionFactory("vm://0").createContext()) {
+ c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
+ QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ Wait.assertEquals(1, () -> server.queueQuery(fqqn).getMessageCount());
+ queueQueryResult = server.queueQuery(queueName);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
+ Wait.assertEquals(2, () -> server.queueQuery(fqqn).getMessageCount());
Review comment:
merged.. however I ammended this change
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic opened a new pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic opened a new pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer URL: https://github.com/apache/activemq-artemis/pull/2520 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250814254
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
Review comment:
or just simple use generic Destination as it was before even.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250814254
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
Review comment:
or just simple use generic Destination as all exceptions were before.
"Destination " + address + " does not exist, auto create addresses is
disabled."
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce opened a new pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce opened a new pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer URL: https://github.com/apache/activemq-artemis/pull/2521 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asfgit closed pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin
asfgit closed pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin URL: https://github.com/apache/activemq-artemis/pull/2516 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] ehsavoie commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
ehsavoie commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250782433
##
File path:
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/tools/DBOption.java
##
@@ -223,13 +225,17 @@ protected void initializeJournal(Configuration
configuration) throws Exception {
this.config = configuration;
executor = Executors.newFixedThreadPool(5,
ActiveMQThreadFactory.defaultThreadFactory());
executorFactory = new OrderedExecutorFactory(executor);
-
- scheduledExecutorService = new
ScheduledThreadPoolExecutor(configuration.getScheduledThreadPoolMaxSize(), new
ThreadFactory() {
+ scheduledExecutorService = new
ScheduledThreadPoolExecutor(configuration.getScheduledThreadPoolMaxSize(),
AccessController.doPrivileged(new PrivilegedAction() {
@Override
- public Thread newThread(Runnable r) {
-return new Thread(r);
+ public ThreadFactory run() {
Review comment:
Because i looked at all threadFactories that werre not protected, didn't
check the implementation. Sorry for this noise on ActiveMQThreadFactory
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250805377
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,58 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
Review comment:
We should only create an address for a JMSQueue if we can create both the
Address and Queue, else you could end up creating an address, but not the queue
(old code had correct logic here)
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250814254
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
Review comment:
or just simple use generic Destination to describe address as all exceptions
were before, with the extra detail.
"Destination " + address + " does not exist, auto create addresses is
disabled."
also would avoid confusion between JMSQueue vs "core" Queue.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250816406
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
Review comment:
maybe use generic Destination, instead of Queue, to avoid confusion between
JMSQueue and "core" Queue.
"Destination " + address + " does not exist, auto create addresses is
enabled, but auto create queues is not."
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250761629
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I'd like to see a test case on this. I think the issue might be we have some
ThreadFactories which are not using the ActiveMQThreadFactory which should imo
ensure the privileged access, and actually we just need to fix those up.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250761629
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I'd like to see a test case on this. I think the issue might be we have some
ThreadFactories which are not using the ActiveMQThreadFactory which should imo
ensure the privileged access if its used in replacement for those, and actually
we just need to fix those up.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250775737
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
should this not also, dare i say check: query.isAutoCreateAddresses()
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250775737
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+} else {
+ if (destination.isQueue()) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ checkQueue(destination, address, clientSession, query);
+ }
+ }
+
+ connection.addKnownDestination(address);
+}
+ } catch (ActiveMQQueueExistsException e) {
+// The queue was created by another client/admin between the query
check and send create queue packet
+ } catch (ActiveMQException e) {
+throw JMSExceptionHelper.convertFromActiveMQException(e);
+ }
+ }
+ }
+
+ private void checkQueue(ActiveMQDestination destination,
+SimpleString address,
+ClientSession clientSession,
+ClientSession.AddressQuery query) throws
ActiveMQException, InvalidDestinationException {
+ if (destination.isQueue() && query.isAutoCreateQueues()) {
Review comment:
should this not also, dare i say check as well: query.isAutoCreateAddresses()
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250780506
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
Review comment:
e.g. if we are succesfull, then the address becomes known right?
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on issue #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on issue #2521: ARTEMIS-2238 Enhancement to queueQuery on producer URL: https://github.com/apache/activemq-artemis/pull/2521#issuecomment-457402626 @clebertsuconic little less of a complete refactor, but addresses the issues. thought id offer it up as an alternative. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] jbertram commented on a change in pull request #2518: NO-JIRA fix race condition in QueueQueryTest
jbertram commented on a change in pull request #2518: NO-JIRA fix race
condition in QueueQueryTest
URL: https://github.com/apache/activemq-artemis/pull/2518#discussion_r250754348
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/QueueQueryTest.java
##
@@ -155,19 +156,20 @@ public void testQueueQueryOnAutoCreatedQueueWithFQQN()
throws Exception {
SimpleString addressName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString queueName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString fqqn = addressName.concat("::").concat(queueName);
- JMSContext c = new ActiveMQConnectionFactory("vm://0").createContext();
- c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
- QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- queueQueryResult = server.queueQuery(queueName);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
- assertEquals(2, server.queueQuery(fqqn).getMessageCount());
- assertEquals(2, server.queueQuery(queueName).getMessageCount());
+ try (JMSContext c = new
ActiveMQConnectionFactory("vm://0").createContext()) {
+ c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
+ QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ Wait.assertEquals(1, () -> server.queueQuery(fqqn).getMessageCount());
+ queueQueryResult = server.queueQuery(queueName);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
+ Wait.assertEquals(2, () -> server.queueQuery(fqqn).getMessageCount());
Review comment:
The test in the amended commit still fails after a handful of iterations.
The query needs to keep running so that it gets the new messageCount from the
server otherwise the messageCount never changes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250779232
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
Review comment:
this is missing adding the destination to known destinations (another bug
introduced by the original change)
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250800199
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +496,52 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery query =
clientSession.addressQuery(address);
+
+if (!query.isExists()) {
+ checkQueue(destination, address, clientSession, query);
Review comment:
it would always add the end, but I'm rewriting the logic now
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250809792
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
Review comment:
seperate the combined check to nested ifs, addressQuery.isExists() outer if,
addressQuery.isAutoCreateAddresses() inner if with inner if else clause here
with throws:
then on debugging from logs with stack trace it would make it easier, as
would get line where it errored out so could figure out. Also could then allow
a better description so end users understand better, e.g.
depending on destination.isQueue()
"Queue " + address + " does not exist, auto create addresses is disabled."
"Topic " + address + " does not exist, auto create addresses is disabled."
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250812700
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/QueueAutoCreationTest.java
##
@@ -133,6 +133,36 @@ public void testAutoCreateOnTopic() throws Exception {
Assert.assertTrue(((ActiveMQConnection)connection).containsKnownDestination(addressName));
}
+ @Test //(timeout = 3)
+ // QueueAutoCreationTest was created to validate auto-creation of queues
+ // and this test was added to validate a regression:
https://issues.apache.org/jira/browse/ARTEMIS-2238
+ public void testAutoCreateOnAddressOnly() throws Exception {
Review comment:
please address my other comments before merging
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250815971
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
+
+
+// Second we create the queue, but we only do it if the address
was created
+if (destination.isQueue() && addressExists) {
+ ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
+ if (!queueQuery.isExists()) {
+ if (addressQuery.isAutoCreateQueues()) {
+ try {
+if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, addressQuery);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, addressQuery);
+}
+ } catch (ActiveMQQueueExistsException thatsOK) {
+// nothing to be done
+ }
+ } else {
+ throw new InvalidDestinationException("Queue " + address
+ " does not exist");
Review comment:
maybe use generic Destination, instead of Queue, to avoid confusion between
JMSQueue and "core" Queue.
"Destination " + address + " does not exist, address exists, but auto create
queues is disabled."
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2520: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2520#discussion_r250814254
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -533,6 +499,67 @@ private void doSendx(ActiveMQDestination destination,
}
}
+ private void checkDestination(ActiveMQDestination destination,
+ SimpleString address,
+ ClientSession clientSession) throws
JMSException {
+
+ // TODO: What to do with FQQN
+ if (!connection.containsKnownDestination(address)) {
+ try {
+ClientSession.AddressQuery addressQuery =
clientSession.addressQuery(address);
+
+boolean addressExists = addressQuery.isExists();
+// first we check the address existence, and autoCreate it if
allowed in case it does not exists
+
+if (!addressExists && addressQuery.isAutoCreateAddresses()) {
+
+ if (destination.isQueue() &&
!addressQuery.isAutoCreateQueues()) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Address " + address + " was not created
because we would not have permission to create queue");
+ }
+ // if it can't create the internal queue on JMS Queues, why
bother creating the address, just mark it false now
+ addressExists = false;
+ } else {
+ RoutingType addressType = destination.isQueue() ?
RoutingType.ANYCAST : RoutingType.MULTICAST;
+ clientSession.createAddress(address, addressType, true);
+ addressExists = true;
+ }
+}
Review comment:
or just simple use generic Destination as all exceptions were before.
"Destination " + address + " does not exist, auto create addresses is
disabled."
also would avoid confusion between JMSQueue vs "core" Queue.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250829575
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
hmmm.. there's another check on createProducer.
It's not worth the effort.. I will just take your version.. and keep the
test I'm writing.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250828872
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
re defaultDestination, i dont beleive it has to, as when creating the
Producer in ActiveMQSession the queue / addresses are created then.
If anything if a defaultDestination exists, then we SHOULD not need go into
this code, as if one exists then any supplied destination must ==
defaultDestination anyhow. And this has to be created at the point of creation
of the producer, for the fact it has to fail to create a producer if it cannot
create/bind to the defaultDestination at that point.
If anything some rationalisation probably could occur, with some of the
logic being shared and pushed in ActiveMQSession
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact
[GitHub] michaelandrepearce closed pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce closed pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer URL: https://github.com/apache/activemq-artemis/pull/2521 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250828204
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
if we could sort out the exceptions in it, that would be great. its midnight
here, will pick up again tomorrow.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250825292
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I mean we need a test in ActiveMQ Artemis for this to merge.
We need to be able to build, refactor, and release with knowing we haven't
broken something upstream or dependent projects have on us. Like wise we need
to validate the fix.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250826843
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
I would prefer going with my PR. it's simpler after all if you look closely.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250825292
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I mean we need a test in ActiveMQ Artemis for this to merge.
We need to be able to build, refactor, and release with knowing we haven't
broken something our users and upstream/dependent projects have on us. Like
wise we need to validate the fix.
essentially i suggest a test is created with a security manager on, in the
activemq artemis project, with a similar leak detector to what youre doing in
your project "wildfly" so to create this testing scenario in artemis.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250825292
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I mean we need a test in ActiveMQ Artemis for this to merge.
We need to be able to build, refactor, and release with knowing we haven't
broken something our users and upstream/dependent projects have on us. Like
wise we need to validate the fix.
essentially i suggest a test is created with a security manager on, in
activemq artemis, which a similar leak detector to what youre doing in your
project "wildfly".
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250825292
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I mean we need a test in ActiveMQ Artemis, we need to be able to build,
refactor, and release with knowing we haven't broken something upstream or
dependent projects have on us. Like wise we need to validate the fix.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
clebertsuconic commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250827739
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
for example. another issue on the original code, it's never checking the
destination on a defaultDestination.
small issue, but my change also addressed that.
I don't know what you think, but the code I wrote it's easier to read.
Although I understand your concern as this is the JMS layer, and TCK is
applied here. But I think it's safe to make the change on this case. I would
run the full testsuite,
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250828872
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
re defaultDestination, i dont beleive it has to, as when creating the
Producer in ActiveMQSession the queue / addresses are created then.
If anything if a defaultDestination exists, then we SHOULD not need go into
this code, as if one exists then any supplied destination must ==
defaultDestination anyhow. And this has to be created at the point of creation
of the producer, for the fact it has to fail to create a producer if it cannot
create/bind to the defaultDestination at that point.
If anything some rationalisation probably (e.g. its all the same logic.
ironically and frustratingly with differences which really there shouldn't be)
could occur, with some of the logic being shared and pushed in ActiveMQSession
This is an automated message from the Apache Git Service.
To respond to the message, please log on
[GitHub] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250828872
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
re defaultDestination, i dont beleive it has to, as when creating the
Producer in ActiveMQSession the queue / addresses are created then.
If anything if a defaultDestination exists, then we SHOULD not need go into
this code, as if one exists then any supplied destination must ==
defaultDestination anyhow. And this has to be created at the point of creation
of the producer, for the fact it has to fail to create a producer if it cannot
create/bind to the defaultDestination at that point.
If anything some rationalisation probably (e.g. its all the same logic
ironically with some differences which really there shouldn't be, the rules
should be the same) could occur, with some of the logic being shared and pushed
in ActiveMQSession
This is an automated message from the Apache Git Service.
To respond to the message,
[GitHub] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250828872
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
re defaultDestination, i dont beleive it has to, as when creating the
Producer in ActiveMQSession the queue / addresses are created then.
If anything if a defaultDestination exists, then we SHOULD not need go into
this code, as if one exists then any supplied destination must ==
defaultDestination anyhow. And this has to be created at the point of creation
of the producer.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250828872
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
re defaultDestination, i dont beleive it has to, as when creating the
Producer in ActiveMQSession the queue / addresses are created then.
If anything if a defaultDestination exists, then we SHOULD not need go into
this code, as if one exists then any supplied destination must ==
defaultDestination anyhow. And this has to be created on creation of the
producer.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250825292
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
I mean we need a test in ActiveMQ Artemis for this to merge.
We need to be able to build, refactor, and release with knowing we haven't
broken something our users and upstream/dependent projects have on us. Like
wise we need to validate the fix.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250828872
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
re defaultDestination, i dont beleive it has to, as when creating the
Producer in ActiveMQSession the queue / addresses are created then.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250835700
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
im just about to send you pr
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250835700
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
im just about to send you pr to your branch
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on issue #2520: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on issue #2520: ARTEMIS-2238 Enhancement to queueQuery on producer URL: https://github.com/apache/activemq-artemis/pull/2520#issuecomment-457422187 sent a pr to your branch to address my comments on this, and also unify logic with ActiveMQSession This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238 Enhancement to queueQuery on producer
michaelandrepearce commented on a change in pull request #2521: ARTEMIS-2238
Enhancement to queueQuery on producer
URL: https://github.com/apache/activemq-artemis/pull/2521#discussion_r250828872
##
File path:
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##
@@ -409,33 +409,42 @@ private void doSendx(ActiveMQDestination destination,
ClientSession.AddressQuery query =
clientSession.addressQuery(address);
if (!query.isExists()) {
- if (destination.isQueue() && query.isAutoCreateQueues()) {
- clientSession.createAddress(address, RoutingType.ANYCAST,
true);
- if (destination.isTemporary()) {
-// TODO is it right to use the address for the queue
name here?
-session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ if (destination.isQueue()) {
+ if (query.isAutoCreateAddresses() &&
query.isAutoCreateQueues()) {
+clientSession.createAddress(address,
RoutingType.ANYCAST, true);
+if (destination.isTemporary()) {
+ // TODO is it right to use the address for the
queue name here?
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+} else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+}
} else {
-session.createQueue(destination, RoutingType.ANYCAST,
address, null, true, true, query);
+throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses() + " , isAutoCreateQueues=" +
query.isAutoCreateQueues());
+ }
+ } else {
+ if (query.isAutoCreateAddresses()) {
+clientSession.createAddress(address,
RoutingType.MULTICAST, true);
+ } else {
+throw new InvalidDestinationException("JMSTopic " +
address + " cannot be created, autoCreateAddresses is " +
query.isAutoCreateAddresses());
}
- } else if (!destination.isQueue() &&
query.isAutoCreateAddresses()) {
- clientSession.createAddress(address,
RoutingType.MULTICAST, true);
- } else if ((destination.isQueue() &&
!query.isAutoCreateQueues()) || (!destination.isQueue() &&
!query.isAutoCreateAddresses())) {
- throw new InvalidDestinationException("Destination " +
address + " does not exist");
}
} else {
if (destination.isQueue()) {
ClientSession.QueueQuery queueQuery =
clientSession.queueQuery(address);
if (!queueQuery.isExists()) {
-if (destination.isTemporary()) {
- session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+if (query.isAutoCreateQueues()) {
+ if (destination.isTemporary()) {
+ session.createTemporaryQueue(destination,
RoutingType.ANYCAST, address, null, query);
+ } else {
+ session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ }
} else {
- session.createQueue(destination,
RoutingType.ANYCAST, address, null, true, true, query);
+ throw new InvalidDestinationException("JMSQueue " +
address + " cannot be created, address exists but autoCreateQueues is " +
query.isAutoCreateQueues());
Review comment:
re defaultDestination, i dont beleive it has to, as when creating the
Producer in ActiveMQSession the queue / addresses are created then.
If anything if a defaultDestination exists, then we SHOULD not need go into
this code, as if one exists then any supplied destination must ==
defaultDestination anyhow.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] asfgit closed pull request #2427: ARTEMIS-2170 Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
asfgit closed pull request #2427: ARTEMIS-2170 Optimized CoreMessage's checkProperties and cleanupInternalProperties methods URL: https://github.com/apache/activemq-artemis/pull/2427 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on issue #2427: ARTEMIS-2170 Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
michaelandrepearce commented on issue #2427: ARTEMIS-2170 Optimized CoreMessage's checkProperties and cleanupInternalProperties methods URL: https://github.com/apache/activemq-artemis/pull/2427#issuecomment-457110595 @franz1981 merge, this is really great stuff. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce edited a comment on issue #2427: ARTEMIS-2170 Optimized CoreMessage's checkProperties and cleanupInternalProperties methods
michaelandrepearce edited a comment on issue #2427: ARTEMIS-2170 Optimized CoreMessage's checkProperties and cleanupInternalProperties methods URL: https://github.com/apache/activemq-artemis/pull/2427#issuecomment-457110595 @franz1981 merged, this is really great stuff. thanks! This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2287: ARTEMIS-2069 Backup doesn't activate after shared store is reconnected
michaelandrepearce commented on a change in pull request #2287: ARTEMIS-2069
Backup doesn't activate after shared store is reconnected
URL: https://github.com/apache/activemq-artemis/pull/2287#discussion_r250604071
##
File path:
tests/extra-tests/src/test/java/org/apache/activemq/artemis/tests/extras/byteman/FileLockNodeManagerTest.java
##
@@ -0,0 +1,75 @@
+package org.apache.activemq.artemis.tests.extras.byteman;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.activemq.artemis.core.server.impl.FileLockNodeManager;
+import org.jboss.byteman.contrib.bmunit.BMRule;
+import org.jboss.byteman.contrib.bmunit.BMRules;
+import org.jboss.byteman.contrib.bmunit.BMUnitRunner;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(BMUnitRunner.class)
+public class FileLockNodeManagerTest {
+
+private static final int TIMEOUT_TOLERANCE = 50;
+
+private File sharedDir;
+
+public FileLockNodeManagerTest() throws IOException {
+sharedDir = File.createTempFile("shared-dir", "");
+sharedDir.delete();
+Assert.assertTrue(sharedDir.mkdir());
+
+}
+
+@Test
+@BMRules(
+rules = {@BMRule(
+name = "throw IOException during activation",
+targetClass =
"org.apache.activemq.artemis.core.server.impl.FileLockNodeManager",
+targetMethod = "tryLock",
+targetLocation = "AT ENTRY",
+action = "THROW new IOException(\"IO Error\");")
+})
+public void test() throws Exception {
+measureLockAcquisisionTimeout(100); // warm-up
+
+assertMeasuredTimeoutFor(100);
+assertMeasuredTimeoutFor(120);
+assertMeasuredTimeoutFor(200);
+//assertMeasuredTimeoutFor(1000);
Review comment:
commented code, either un-comment or remove.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2287: ARTEMIS-2069 Backup doesn't activate after shared store is reconnected
michaelandrepearce commented on a change in pull request #2287: ARTEMIS-2069
Backup doesn't activate after shared store is reconnected
URL: https://github.com/apache/activemq-artemis/pull/2287#discussion_r250605719
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/FileLockNodeManager.java
##
@@ -299,44 +301,52 @@ protected FileLock tryLock(final long lockPos) throws
IOException {
protected FileLock lock(final long lockPosition) throws Exception {
long start = System.currentTimeMillis();
+ boolean isRecurringFailure = false;
while (!interrupted) {
- FileLock lock = tryLock(lockPosition);
-
- if (lock == null) {
-try {
- Thread.sleep(500);
-} catch (InterruptedException e) {
- return null;
+ try {
+FileLock lock = tryLock(lockPosition);
+isRecurringFailure = false;
+
+if (lock == null) {
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ return null;
+ }
+
+ if (lockAcquisitionTimeout != -1 && (System.currentTimeMillis()
- start) > lockAcquisitionTimeout) {
+ throw new Exception("timed out waiting for lock");
Review comment:
Little bit too generic, this exception
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2287: ARTEMIS-2069 Backup doesn't activate after shared store is reconnected
michaelandrepearce commented on a change in pull request #2287: ARTEMIS-2069
Backup doesn't activate after shared store is reconnected
URL: https://github.com/apache/activemq-artemis/pull/2287#discussion_r250605719
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/FileLockNodeManager.java
##
@@ -299,44 +301,52 @@ protected FileLock tryLock(final long lockPos) throws
IOException {
protected FileLock lock(final long lockPosition) throws Exception {
long start = System.currentTimeMillis();
+ boolean isRecurringFailure = false;
while (!interrupted) {
- FileLock lock = tryLock(lockPosition);
-
- if (lock == null) {
-try {
- Thread.sleep(500);
-} catch (InterruptedException e) {
- return null;
+ try {
+FileLock lock = tryLock(lockPosition);
+isRecurringFailure = false;
+
+if (lock == null) {
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ return null;
+ }
+
+ if (lockAcquisitionTimeout != -1 && (System.currentTimeMillis()
- start) > lockAcquisitionTimeout) {
+ throw new Exception("timed out waiting for lock");
Review comment:
Little bit too generic, this exception, should throw something more specific.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2287: ARTEMIS-2069 Backup doesn't activate after shared store is reconnected
michaelandrepearce commented on a change in pull request #2287: ARTEMIS-2069
Backup doesn't activate after shared store is reconnected
URL: https://github.com/apache/activemq-artemis/pull/2287#discussion_r250605929
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/FileLockNodeManager.java
##
@@ -299,44 +301,52 @@ protected FileLock tryLock(final long lockPos) throws
IOException {
protected FileLock lock(final long lockPosition) throws Exception {
long start = System.currentTimeMillis();
+ boolean isRecurringFailure = false;
while (!interrupted) {
- FileLock lock = tryLock(lockPosition);
-
- if (lock == null) {
-try {
- Thread.sleep(500);
-} catch (InterruptedException e) {
- return null;
+ try {
+FileLock lock = tryLock(lockPosition);
+isRecurringFailure = false;
+
+if (lock == null) {
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ return null;
+ }
+
+ if (lockAcquisitionTimeout != -1 && (System.currentTimeMillis()
- start) > lockAcquisitionTimeout) {
+ throw new Exception("timed out waiting for lock");
+ }
+} else {
+ return lock;
}
-
-if (lockAcquisitionTimeout != -1 && (System.currentTimeMillis() -
start) > lockAcquisitionTimeout) {
- throw new Exception("timed out waiting for lock");
+ } catch (IOException e) {
+// IOException during trylock() may be a temporary issue, e.g. NFS
volume not being accessible
+
+logger.log(isRecurringFailure ? Logger.Level.DEBUG :
Logger.Level.WARN,
+"Failure when accessing a lock file", e);
+isRecurringFailure = true;
+
+long waitTime = LOCK_ACCESS_FAILURE_WAIT_TIME;
+if (lockAcquisitionTimeout != -1) {
+ final long remainingTime = lockAcquisitionTimeout -
(System.currentTimeMillis() - start);
+ if (remainingTime <= 0) {
+ throw new Exception("timed out waiting for lock");
Review comment:
Little bit too generic, this exception, should throw something more specific.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250700525
##
File path:
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/tools/DBOption.java
##
@@ -223,13 +225,17 @@ protected void initializeJournal(Configuration
configuration) throws Exception {
this.config = configuration;
executor = Executors.newFixedThreadPool(5,
ActiveMQThreadFactory.defaultThreadFactory());
executorFactory = new OrderedExecutorFactory(executor);
-
- scheduledExecutorService = new
ScheduledThreadPoolExecutor(configuration.getScheduledThreadPoolMaxSize(), new
ThreadFactory() {
+ scheduledExecutorService = new
ScheduledThreadPoolExecutor(configuration.getScheduledThreadPoolMaxSize(),
AccessController.doPrivileged(new PrivilegedAction() {
@Override
- public Thread newThread(Runnable r) {
-return new Thread(r);
+ public ThreadFactory run() {
Review comment:
I know this inst your change, but it highlights a bigger question, as to why
is this not using ActiveMQThreadFactory?
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250702023
##
File path:
artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/bridge/impl/JMSBridgeImpl.java
##
@@ -1598,25 +1598,30 @@ private static void copyProperties(final Message msg)
throws JMSException {
* and 1 for the eventual failureHandler)
*/
private ExecutorService createExecutor() {
- ExecutorService service = Executors.newFixedThreadPool(3, new
ThreadFactory() {
-
- ThreadGroup group = new ThreadGroup("JMSBridgeImpl");
-
+ ExecutorService service = Executors.newFixedThreadPool(3,
AccessController.doPrivileged(new PrivilegedAction() {
@Override
- public Thread newThread(Runnable r) {
-final Thread thr = new Thread(group, r);
-if (moduleTccl != null) {
- AccessController.doPrivileged(new PrivilegedAction() {
- @Override
- public Object run() {
- thr.setContextClassLoader(moduleTccl);
- return null;
+ public ThreadFactory run() {
+return new ThreadFactory() {
Review comment:
I know this inst your change, but it highlights a bigger question, as to why
is this not using ActiveMQThreadFactory?
As this creates threads with the captured context.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250701848
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##
@@ -153,7 +153,12 @@ public synchronized void start() {
}
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory(this.getClass().getSimpleName() +
"-scheduled-threads", false, getThisClassLoader());
+ return AccessController.doPrivileged(new
PrivilegedAction() {
+ @Override
+ public ActiveMQThreadFactory run() {
+return new ActiveMQThreadFactory(this.getClass().getSimpleName() +
"-scheduled-threads", false, getThisClassLoader());
Review comment:
Threads created by ActiveMQThreadFactory should be already created by
doPrivileged. how are we sure its not just that the thread issues you are
seeing are maybe just where code isn't using ActiveMQThreadFactory
see
@Override
public Thread newThread(final Runnable command) {
// create a thread in a privileged block if running with Security
Manager
if (acc != null) {
return AccessController.doPrivileged(new
ThreadCreateAction(command), acc);
} else {
return createThread(command);
}
}
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250701848
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##
@@ -153,7 +153,12 @@ public synchronized void start() {
}
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory(this.getClass().getSimpleName() +
"-scheduled-threads", false, getThisClassLoader());
+ return AccessController.doPrivileged(new
PrivilegedAction() {
+ @Override
+ public ActiveMQThreadFactory run() {
+return new ActiveMQThreadFactory(this.getClass().getSimpleName() +
"-scheduled-threads", false, getThisClassLoader());
Review comment:
Threads created by ActiveMQThreadFactory should be already created by
doPrivileged. how are we sure its not just that the thread issues you are
seeing are maybe just where code isn't using ActiveMQThreadFactory
see
```
@Override
public Thread newThread(final Runnable command) {
// create a thread in a privileged block if running with Security
Manager
if (acc != null) {
return AccessController.doPrivileged(new
ThreadCreateAction(command), acc);
} else {
return createThread(command);
}
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin
michaelandrepearce commented on a change in pull request #2516: ARTEMIS-2192
fix listener for LegacyLDAPSecuritySettingPlugin
URL: https://github.com/apache/activemq-artemis/pull/2516#discussion_r250706777
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/LegacyLDAPSecuritySettingPlugin.java
##
@@ -333,14 +333,14 @@ private void processSearchResult(Map>
securityRoles,
String destination = null;
String destinationType = "unknown";
List rdns = searchResultLdapName.getRdns();
- if (rdns.size() != 3) {
+ if (rdns.size() < 3) {
if (logger.isDebugEnabled()) {
logger.debug("\tSkipping unexpected search result with " +
rdns.size() + " RDNs.");
}
return;
}
// we can count on the RNDs being in order from right to left
- Rdn rdn = rdns.get(0);
+ Rdn rdn = rdns.get(rdns.size() - 3);
Review comment:
Im not entirely sure what this code is doing and what expected results or
ldap setup must be followed, but i do know from LDAP its possible to get
duplicate rdns, so what occurs if rdns is > 3 and a dn is duplicated
e.g.
uid=john.doe,ou=People,dc=example,dc=com
dc is twice here, what do you need? also would mean you may miss the dn you
do want maybe the in this case, would be uid.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin
michaelandrepearce commented on a change in pull request #2516: ARTEMIS-2192
fix listener for LegacyLDAPSecuritySettingPlugin
URL: https://github.com/apache/activemq-artemis/pull/2516#discussion_r250707350
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/LegacyLDAPSecuritySettingPlugin.java
##
@@ -333,14 +333,14 @@ private void processSearchResult(Map>
securityRoles,
String destination = null;
String destinationType = "unknown";
List rdns = searchResultLdapName.getRdns();
- if (rdns.size() != 3) {
+ if (rdns.size() < 3) {
if (logger.isDebugEnabled()) {
logger.debug("\tSkipping unexpected search result with " +
rdns.size() + " RDNs.");
}
return;
}
// we can count on the RNDs being in order from right to left
- Rdn rdn = rdns.get(0);
+ Rdn rdn = rdns.get(rdns.size() - 3);
Review comment:
tbh the older, hard coded rdns made a bit more sense to me, at least i know
what rdn are being expected and looked for, and ordering (to handle duplicates
in response) wouldnt be an issue.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin
michaelandrepearce commented on a change in pull request #2516: ARTEMIS-2192
fix listener for LegacyLDAPSecuritySettingPlugin
URL: https://github.com/apache/activemq-artemis/pull/2516#discussion_r250706777
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/LegacyLDAPSecuritySettingPlugin.java
##
@@ -333,14 +333,14 @@ private void processSearchResult(Map>
securityRoles,
String destination = null;
String destinationType = "unknown";
List rdns = searchResultLdapName.getRdns();
- if (rdns.size() != 3) {
+ if (rdns.size() < 3) {
if (logger.isDebugEnabled()) {
logger.debug("\tSkipping unexpected search result with " +
rdns.size() + " RDNs.");
}
return;
}
// we can count on the RNDs being in order from right to left
- Rdn rdn = rdns.get(0);
+ Rdn rdn = rdns.get(rdns.size() - 3);
Review comment:
Hands up this isnt something im super hot on, and im not entirely sure what
this code is doing and what expected results or ldap setup must be followed
(so please choose to ignore this)
but i do know from LDAP its possible to get duplicate rdns, so what occurs
if rdns is > 3 and a dn is duplicated
e.g.
uid=john.doe,ou=People,dc=example,dc=com
dc is twice here, what do you need? also would mean you may miss the dn you
do want maybe the in this case, would be uid.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on issue #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on issue #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block. URL: https://github.com/apache/activemq-artemis/pull/2517#issuecomment-457289566 This **really** needs a test case, to a) validate the fix, and b) ensure no regression. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce removed a comment on issue #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce removed a comment on issue #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block. URL: https://github.com/apache/activemq-artemis/pull/2517#issuecomment-457289566 This **really** needs a test case, to a) validate the fix, and b) ensure no regression. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on issue #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on issue #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block. URL: https://github.com/apache/activemq-artemis/pull/2517#issuecomment-457290572 This really needs a test case A) validate the fix (and validate any alternative proposals) B) ensure no regression This is an automated message from the Apache Git Service. To respond to the message, please log on 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] jbertram commented on a change in pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin
jbertram commented on a change in pull request #2516: ARTEMIS-2192 fix listener
for LegacyLDAPSecuritySettingPlugin
URL: https://github.com/apache/activemq-artemis/pull/2516#discussion_r250717165
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/LegacyLDAPSecuritySettingPlugin.java
##
@@ -333,14 +333,14 @@ private void processSearchResult(Map>
securityRoles,
String destination = null;
String destinationType = "unknown";
List rdns = searchResultLdapName.getRdns();
- if (rdns.size() != 3) {
+ if (rdns.size() < 3) {
if (logger.isDebugEnabled()) {
logger.debug("\tSkipping unexpected search result with " +
rdns.size() + " RDNs.");
}
return;
}
// we can count on the RNDs being in order from right to left
- Rdn rdn = rdns.get(0);
+ Rdn rdn = rdns.get(rdns.size() - 3);
Review comment:
The problem with the hard-coded rdns is that the plugin won't work for
anybody who doesn't use the same values. I don't see duplicates as being a
problem with this code. It simply takes the first three rdns and interprets
them as permission-type, destination-name, & destination-type. It doesn't care
about the rdns past that.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] jbertram opened a new pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin
jbertram opened a new pull request #2516: ARTEMIS-2192 fix listener for LegacyLDAPSecuritySettingPlugin URL: https://github.com/apache/activemq-artemis/pull/2516 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] ehsavoie opened a new pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
ehsavoie opened a new pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block. URL: https://github.com/apache/activemq-artemis/pull/2517 * Ensuring that all threadPoolFactories are created under a privileged block so the threads created are under the same AccessControlContext. Jira: https://issues.apache.org/jira/browse/ARTEMIS-2171 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250700525
##
File path:
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/tools/DBOption.java
##
@@ -223,13 +225,17 @@ protected void initializeJournal(Configuration
configuration) throws Exception {
this.config = configuration;
executor = Executors.newFixedThreadPool(5,
ActiveMQThreadFactory.defaultThreadFactory());
executorFactory = new OrderedExecutorFactory(executor);
-
- scheduledExecutorService = new
ScheduledThreadPoolExecutor(configuration.getScheduledThreadPoolMaxSize(), new
ThreadFactory() {
+ scheduledExecutorService = new
ScheduledThreadPoolExecutor(configuration.getScheduledThreadPoolMaxSize(),
AccessController.doPrivileged(new PrivilegedAction() {
@Override
- public Thread newThread(Runnable r) {
-return new Thread(r);
+ public ThreadFactory run() {
Review comment:
I know this inst your change, but it highlights a bigger question, as to why
is this not using ActiveMQThreadFactory?
As this creates threads with the captured context.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
michaelandrepearce commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250701848
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##
@@ -153,7 +153,12 @@ public synchronized void start() {
}
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory(this.getClass().getSimpleName() +
"-scheduled-threads", false, getThisClassLoader());
+ return AccessController.doPrivileged(new
PrivilegedAction() {
+ @Override
+ public ActiveMQThreadFactory run() {
+return new ActiveMQThreadFactory(this.getClass().getSimpleName() +
"-scheduled-threads", false, getThisClassLoader());
Review comment:
Threads created by ActiveMQThreadFactory should be already created by
doPrivileged. how are we sure its not just that the thread issues you are
seeing are maybe just where code isn't using ActiveMQThreadFactory
see in ActiveMQThreadFactory
```
@Override
public Thread newThread(final Runnable command) {
// create a thread in a privileged block if running with Security
Manager
if (acc != null) {
return AccessController.doPrivileged(new
ThreadCreateAction(command), acc);
} else {
return createThread(command);
}
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] jbertram opened a new pull request #2518: NO-JIRA fix race condition in QueueQueryTest
jbertram opened a new pull request #2518: NO-JIRA fix race condition in QueueQueryTest URL: https://github.com/apache/activemq-artemis/pull/2518 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] clebertsuconic opened a new pull request #2519: ARTEMIS-2238 Fixing QueueQuery on every single send on topics
clebertsuconic opened a new pull request #2519: ARTEMIS-2238 Fixing QueueQuery on every single send on topics URL: https://github.com/apache/activemq-artemis/pull/2519 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2518: NO-JIRA fix race condition in QueueQueryTest
clebertsuconic commented on a change in pull request #2518: NO-JIRA fix race
condition in QueueQueryTest
URL: https://github.com/apache/activemq-artemis/pull/2518#discussion_r250731204
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/QueueQueryTest.java
##
@@ -155,19 +156,22 @@ public void testQueueQueryOnAutoCreatedQueueWithFQQN()
throws Exception {
SimpleString addressName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString queueName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString fqqn = addressName.concat("::").concat(queueName);
- JMSContext c = new ActiveMQConnectionFactory("vm://0").createContext();
- c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
- QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- queueQueryResult = server.queueQuery(queueName);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
- assertEquals(2, server.queueQuery(fqqn).getMessageCount());
- assertEquals(2, server.queueQuery(queueName).getMessageCount());
+ try (JMSContext c = new
ActiveMQConnectionFactory("vm://0").createContext()) {
+ c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
+ assertTrue(Wait.waitFor(() ->
server.locateQueue(queueName).getMessageCount() == 1, 2000, 100));
+ QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ queueQueryResult = server.queueQuery(queueName);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
+ assertTrue(Wait.waitFor(() ->
server.locateQueue(queueName).getMessageCount() == 2, 2000, 100));
Review comment:
you could have used: Wait.assertEquals(100,
server.locateQueue(queueName)::getMessageCount) ?
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2518: NO-JIRA fix race condition in QueueQueryTest
clebertsuconic commented on a change in pull request #2518: NO-JIRA fix race
condition in QueueQueryTest
URL: https://github.com/apache/activemq-artemis/pull/2518#discussion_r250731249
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/QueueQueryTest.java
##
@@ -155,19 +156,22 @@ public void testQueueQueryOnAutoCreatedQueueWithFQQN()
throws Exception {
SimpleString addressName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString queueName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString fqqn = addressName.concat("::").concat(queueName);
- JMSContext c = new ActiveMQConnectionFactory("vm://0").createContext();
- c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
- QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- queueQueryResult = server.queueQuery(queueName);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
- assertEquals(2, server.queueQuery(fqqn).getMessageCount());
- assertEquals(2, server.queueQuery(queueName).getMessageCount());
+ try (JMSContext c = new
ActiveMQConnectionFactory("vm://0").createContext()) {
+ c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
+ assertTrue(Wait.waitFor(() ->
server.locateQueue(queueName).getMessageCount() == 1, 2000, 100));
+ QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ queueQueryResult = server.queueQuery(queueName);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
+ assertTrue(Wait.waitFor(() ->
server.locateQueue(queueName).getMessageCount() == 2, 2000, 100));
+ assertEquals(2, server.queueQuery(fqqn).getMessageCount());
Review comment:
same here? and next?
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] clebertsuconic commented on a change in pull request #2517: [ARTEMIS-2171]: ThreadPoolExecutor leak under SM due to lack of privileged block.
clebertsuconic commented on a change in pull request #2517: [ARTEMIS-2171]:
ThreadPoolExecutor leak under SM due to lack of privileged block.
URL: https://github.com/apache/activemq-artemis/pull/2517#discussion_r250732153
##
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/NetworkHealthCheck.java
##
@@ -157,7 +157,12 @@ public NetworkHealthCheck parseURIList(String
addressList) {
@Override
protected ActiveMQThreadFactory getThreadFactory() {
- return new ActiveMQThreadFactory("NetworkChecker", "Network-Checker-",
false, getThisClassLoader());
Review comment:
This is just creating a ThreadFactory.. is it really needed?
I would expect a security check around new Threads, but against a new
ActiveMQThreadFactory?
are you sure about it? just checking it's not a typo?
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] jbertram commented on a change in pull request #2518: NO-JIRA fix race condition in QueueQueryTest
jbertram commented on a change in pull request #2518: NO-JIRA fix race
condition in QueueQueryTest
URL: https://github.com/apache/activemq-artemis/pull/2518#discussion_r250733623
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/QueueQueryTest.java
##
@@ -155,19 +156,22 @@ public void testQueueQueryOnAutoCreatedQueueWithFQQN()
throws Exception {
SimpleString addressName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString queueName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString fqqn = addressName.concat("::").concat(queueName);
- JMSContext c = new ActiveMQConnectionFactory("vm://0").createContext();
- c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
- QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- queueQueryResult = server.queueQuery(queueName);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
- assertEquals(2, server.queueQuery(fqqn).getMessageCount());
- assertEquals(2, server.queueQuery(queueName).getMessageCount());
+ try (JMSContext c = new
ActiveMQConnectionFactory("vm://0").createContext()) {
+ c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
+ assertTrue(Wait.waitFor(() ->
server.locateQueue(queueName).getMessageCount() == 1, 2000, 100));
+ QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ queueQueryResult = server.queueQuery(queueName);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
+ assertTrue(Wait.waitFor(() ->
server.locateQueue(queueName).getMessageCount() == 2, 2000, 100));
Review comment:
That's definitely more elegant. Will send an update in a bit.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] jbertram commented on a change in pull request #2518: NO-JIRA fix race condition in QueueQueryTest
jbertram commented on a change in pull request #2518: NO-JIRA fix race
condition in QueueQueryTest
URL: https://github.com/apache/activemq-artemis/pull/2518#discussion_r250736801
##
File path:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/QueueQueryTest.java
##
@@ -155,19 +156,22 @@ public void testQueueQueryOnAutoCreatedQueueWithFQQN()
throws Exception {
SimpleString addressName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString queueName =
SimpleString.toSimpleString(UUID.randomUUID().toString());
SimpleString fqqn = addressName.concat("::").concat(queueName);
- JMSContext c = new ActiveMQConnectionFactory("vm://0").createContext();
- c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
- QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- queueQueryResult = server.queueQuery(queueName);
- assertEquals(queueName, queueQueryResult.getName());
- assertEquals(addressName, queueQueryResult.getAddress());
- assertEquals(1, queueQueryResult.getMessageCount());
- c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
- assertEquals(2, server.queueQuery(fqqn).getMessageCount());
- assertEquals(2, server.queueQuery(queueName).getMessageCount());
+ try (JMSContext c = new
ActiveMQConnectionFactory("vm://0").createContext()) {
+ c.createProducer().send(c.createQueue(fqqn.toString()),
c.createMessage());
+ assertTrue(Wait.waitFor(() ->
server.locateQueue(queueName).getMessageCount() == 1, 2000, 100));
+ QueueQueryResult queueQueryResult = server.queueQuery(fqqn);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ queueQueryResult = server.queueQuery(queueName);
+ assertEquals(queueName, queueQueryResult.getName());
+ assertEquals(addressName, queueQueryResult.getAddress());
+ assertEquals(1, queueQueryResult.getMessageCount());
+ c.createProducer().send(c.createQueue(addressName.toString()),
c.createMessage());
+ assertTrue(Wait.waitFor(() ->
server.locateQueue(queueName).getMessageCount() == 2, 2000, 100));
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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
