You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by rpelisse <gi...@git.apache.org> on 2018/09/13 15:10:28 UTC
[GitHub] activemq-artemis pull request #2312: JBEAP-14106 - Improve validation of MDB...
GitHub user rpelisse opened a pull request:
https://github.com/apache/activemq-artemis/pull/2312
JBEAP-14106 - Improve validation of MDB activation config properties values
Issue: https://issues.jboss.org/browse/JBEAP-14106
No upstream issue
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rpelisse/activemq-artemis JBEAP-14016_4_UPSTREAM
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2312.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2312
----
commit ee723bd0e20cea8174ba4c90891ca3161c041686
Author: Romain Pelisse <be...@...>
Date: 2018-09-13T15:02:58Z
JBEAP-14106 - Improve validation of MDB activation config properties values
----
---
[GitHub] activemq-artemis issue #2312: JBEAP-14106 - Improve validation of MDB activa...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
Of course! I just did not had an account on Apache. I guess I will just have to create one :)
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2312#discussion_r224148249
--- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivation.java ---
@@ -556,7 +556,7 @@ protected void setupDestination() throws Exception {
calculatedDestinationName = spec.getQueuePrefix() + calculatedDestinationName;
}
- logger.debug("Unable to retrieve " + destinationName + " from JNDI. Creating a new " + destinationType.getName() + " named " + calculatedDestinationName + " to be used by the MDB.");
+ logger.warn("Unable to retrieve " + destinationName + " from JNDI. Creating a new " + destinationType.getName() + " named " + calculatedDestinationName + " to be used by the MDB.");
--- End diff --
if this is no longer debug, it requires to user proper logger, with a defined logger code.
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2312#discussion_r224151789
--- Diff: artemis-ra/src/test/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationsSpecTest.java ---
@@ -0,0 +1,46 @@
+package org.apache.activemq.artemis.ra.inflow;
--- End diff --
RAT failure, requires license header
---
[GitHub] activemq-artemis issue #2312: JBEAP-14106 - Improve validation of MDB activa...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
INFO level message and above need to be "methodized." See `org.apache.activemq.artemis.ra.ActiveMQRALogger`.
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2312#discussion_r224460300
--- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationValidationUtils.java ---
@@ -0,0 +1,80 @@
+package org.apache.activemq.artemis.ra.inflow;
+
+import java.beans.IntrospectionException;
+import java.beans.PropertyDescriptor;
+import java.util.ArrayList;
+import java.util.List;
+
+import javax.jms.Queue;
+import javax.jms.Session;
+import javax.jms.Topic;
+import javax.resource.spi.InvalidPropertyException;
+
+import org.apache.activemq.artemis.ra.ActiveMQRALogger;
+
+/**
+ * A set of validation methods, isolated here to be easily testable.
+ *
+ * @author rpelisse
--- End diff --
Done.
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2312#discussion_r224148916
--- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationSpec.java ---
@@ -603,7 +596,11 @@ public void setMaxSession(final Integer value) {
logger.trace("setMaxSession(" + value + ")");
}
- maxSession = value;
+ if ( value < 1 ) {
+ logger.warn("Invalid number of session (negative):" + value + ", defaulting to 1.");
--- End diff --
this requires a declared logger in ActiveMQRALogger and logger code
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
> I merged this on master and 2.6.x. @rpelisse, can you please close this?
Gladly!
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
@jbertram thanks!
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2312#discussion_r224151461
--- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationValidationUtils.java ---
@@ -0,0 +1,80 @@
+package org.apache.activemq.artemis.ra.inflow;
--- End diff --
RAT failure, requires license headers
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
@rpelisse, to restart the Travis CI build you'll need to do something like `git commit --amend` and then `git push -f <prBranch>`.
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
I merged this on master and 2.6.x. @rpelisse, can you please close this?
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
@rpelisse theres a suite of tests i believe under (org.apache.activemq.artemis.tests.integration.ra ) package in the integration test module, this would probably where i would look to add such test.
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2312#discussion_r224151533
--- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationValidationUtils.java ---
@@ -0,0 +1,80 @@
+package org.apache.activemq.artemis.ra.inflow;
+
+import java.beans.IntrospectionException;
+import java.beans.PropertyDescriptor;
+import java.util.ArrayList;
+import java.util.List;
+
+import javax.jms.Queue;
+import javax.jms.Session;
+import javax.jms.Topic;
+import javax.resource.spi.InvalidPropertyException;
+
+import org.apache.activemq.artemis.ra.ActiveMQRALogger;
+
+/**
+ * A set of validation methods, isolated here to be easily testable.
+ *
+ * @author rpelisse
--- End diff --
remove please
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
@michaelandrepearce ok all fix now, let me know if you want me to change anything else!
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
@michaelandrepearce I've updated this branch with my Unit tests. I did a bit of refactor also, I hope it's fine with you and let me know if you want me to change something!
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
@jbertram logger methodize - let me know if you want me to change something there.
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
@michaelandrepearce there is no unit test in the artemis-ra, do you want me to add a first unit to validate this behavior? (I'm fine doing so, I just want to be sure it is what you meant).
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2312#discussion_r224459911
--- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationSpec.java ---
@@ -603,7 +596,11 @@ public void setMaxSession(final Integer value) {
logger.trace("setMaxSession(" + value + ")");
}
- maxSession = value;
+ if ( value < 1 ) {
+ logger.warn("Invalid number of session (negative):" + value + ", defaulting to 1.");
--- End diff --
Quite sorry about that. I forgot about those. I'll fix it up right away.
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
@michaelandrepearce maybe it's me, but it feels a bit like an overkill to add such a test into the integration test. I would rather move the validation logic into an Utils class (with static method) and add a simple JUnit test, with some mock object if needed. This way the validation logic can tested, enhance and modified completely out of the container. Would you mind if I did just that?
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2312
---
[GitHub] activemq-artemis pull request #2312: ARTEMIS-2085 - Improve validation of MD...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2312#discussion_r224460169
--- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationValidationUtils.java ---
@@ -0,0 +1,80 @@
+package org.apache.activemq.artemis.ra.inflow;
--- End diff --
Same, my bad. I knew I needed to double check the license headers and forgot to do it. I'll update the PR accordingly.
---
[GitHub] activemq-artemis issue #2312: JBEAP-14106 - Improve validation of MDB activa...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
Please can you create an activemq jira and update all references to that,not to a commercial vendors internal bugs system
---
[GitHub] activemq-artemis issue #2312: ARTEMIS-2085 - Improve validation of MDB activ...
Posted by rpelisse <gi...@git.apache.org>.
Github user rpelisse commented on the issue:
https://github.com/apache/activemq-artemis/pull/2312
retest this please
---