You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by stanlyDoge <gi...@git.apache.org> on 2017/12/05 16:03:07 UTC
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
GitHub user stanlyDoge opened a pull request:
https://github.com/apache/activemq-artemis/pull/1688
ARTEMIS-1537 broker was less strict while reloading configuration
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/stanlyDoge/activemq-artemis-1 E689
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1688.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 #1688
----
commit 653b995e3dcda0b026dce4dcf4155d317950faf1
Author: Stanislav Knot <sk...@redhat.com>
Date: 2017-12-05T16:01:20Z
ARTEMIS-1537 broker was less strict while reloading configuration
----
---
[GitHub] activemq-artemis issue #1688: ARTEMIS-1537 broker was less strict while relo...
Posted by stanlyDoge <gi...@git.apache.org>.
Github user stanlyDoge commented on the issue:
https://github.com/apache/activemq-artemis/pull/1688
Thanks for your comment @jdanekrh
---
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
Posted by stanlyDoge <gi...@git.apache.org>.
Github user stanlyDoge commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1688#discussion_r156064778
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream input) throws Exception {
String xml = XMLUtil.readerToString(reader);
xml = XMLUtil.replaceSystemProps(xml);
Element e = XMLUtil.stringToElement(xml);
-
+ NodeList children = e.getElementsByTagName("core");
--- End diff --
Ah, didn't know about this, sorry. Should be ok now.
---
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
Posted by stanlyDoge <gi...@git.apache.org>.
Github user stanlyDoge commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1688#discussion_r155719121
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream input) throws Exception {
String xml = XMLUtil.readerToString(reader);
xml = XMLUtil.replaceSystemProps(xml);
Element e = XMLUtil.stringToElement(xml);
-
+ NodeList children = e.getElementsByTagName("core");
--- End diff --
Yes, that seems better than my solution. I wanted to use the same process while reloading conf. as parsing conf. when starting broker (https://github.com/apache/activemq-artemis/blob/master/artemis-server/src/main/java/org/apache/activemq/artemis/core/config/FileDeploymentManager.java#L85).
---
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
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/1688#discussion_r155674146
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream input) throws Exception {
String xml = XMLUtil.readerToString(reader);
xml = XMLUtil.replaceSystemProps(xml);
Element e = XMLUtil.stringToElement(xml);
-
+ NodeList children = e.getElementsByTagName("core");
--- End diff --
Why not just get the broker.xml fully validated by the xsd using javax.xml.validation.Validator instead of manually getting elements and recursing.
---
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
Posted by stanlyDoge <gi...@git.apache.org>.
Github user stanlyDoge commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1688#discussion_r156024601
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream input) throws Exception {
String xml = XMLUtil.readerToString(reader);
xml = XMLUtil.replaceSystemProps(xml);
Element e = XMLUtil.stringToElement(xml);
-
+ NodeList children = e.getElementsByTagName("core");
--- End diff --
Hmm, after another investigation I am not sure if this is possible with current XSD schema. It does not contain 'configuration' element, so it cannot be checked this way. There are two solutions - modify XSD schema to contain missing element or use my solution, where I am getting first child of 'configuration', which is 'core'.
---
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
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/1688#discussion_r155675210
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream input) throws Exception {
String xml = XMLUtil.readerToString(reader);
xml = XMLUtil.replaceSystemProps(xml);
Element e = XMLUtil.stringToElement(xml);
-
+ NodeList children = e.getElementsByTagName("core");
--- End diff --
e.g.
SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI);
Schema schema = schemaFactory.newSchema(new File("schema/artemis-configuration.xsd"));
Validator validator = schema.newValidator();
validator.validate(new DOMSource(e));
---
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1688#discussion_r155199713
--- Diff: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/config/impl/ConfigurationValidationTest.java ---
@@ -57,4 +57,23 @@ public void testFullConfiguration() throws Exception {
Assert.assertEquals(true, fc.isPersistDeliveryCountBeforeDelivery());
}
+
+ @Test
+ public void testChangeConfiguration() throws Exception {
+ FileConfiguration fc = new FileConfiguration();
+ FileDeploymentManager deploymentManager = new FileDeploymentManager("ConfigurationTest-full-config.xml");
+ deploymentManager.addDeployable(fc);
+ deploymentManager.readConfiguration();
+
+ boolean success = false; // test should fail because config contains wrong element
+
+ deploymentManager = new FileDeploymentManager("ConfigurationTest-full-config-wrong-address.xml");
+ deploymentManager.addDeployable(fc);
+ try {
+ deploymentManager.readConfiguration();
+ } catch (Exception e) {
+ success = true;
+ }
+ Assert.assertTrue(success);
--- End diff --
Tests like that seem to be usually written a bit differently, like this https://github.com/apache/activemq-artemis/blob/f698a7f8189af7b70160ba18596be371642776bb/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/jms2client/BodyTest.java#L66
It can be worth it to name the exception variable `ignored`, not `e`, because that is a hint to unused variable inspection in IntelliJ. https://www.reddit.com/r/ProgrammerHumor/comments/2so5tu/mildly_amusing_intellij_suggests_to_rename_a/ Not sure what it would suggest if you have multiple ignored exceptions in scope... It is less verbose than https://docs.oracle.com/javase/7/docs/api/java/lang/SuppressWarnings.html ("unused")
---
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1688
---
[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...
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/1688#discussion_r156030409
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream input) throws Exception {
String xml = XMLUtil.readerToString(reader);
xml = XMLUtil.replaceSystemProps(xml);
Element e = XMLUtil.stringToElement(xml);
-
+ NodeList children = e.getElementsByTagName("core");
--- End diff --
https://github.com/apache/activemq-artemis/blob/master/artemis-server/src/main/resources/schema/artemis-server.xsd it does.
---
[GitHub] activemq-artemis issue #1688: ARTEMIS-1537 broker was less strict while relo...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1688
@stanlyDoge good work :) +1, will look to merge later today if no one else does before me.
---
[GitHub] activemq-artemis issue #1688: ARTEMIS-1537 broker was less strict while relo...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1688
wow!
such fix!
thanks much!
---
[GitHub] activemq-artemis issue #1688: ARTEMIS-1537 broker was less strict while relo...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1688
@stanlyDoge all merged, thanks for this.
---