[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
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 <[hidden email]>
Date:   2017-12-05T16:01:20Z

    ARTEMIS-1537 broker was less strict while reloading configuration

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
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")


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1688: ARTEMIS-1537 broker was less strict while relo...

pgfox
In reply to this post by pgfox
Github user stanlyDoge commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1688
 
    Thanks for your comment @jdanekrh


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
In reply to this post by pgfox
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
In reply to this post by pgfox
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));


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
In reply to this post by pgfox
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).


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
In reply to this post by pgfox
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'.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
In reply to this post by pgfox
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
In reply to this post by pgfox
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1688: ARTEMIS-1537 broker was less strict while relo...

pgfox
In reply to this post by pgfox
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

pgfox
In reply to this post by pgfox
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/1688


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1688: ARTEMIS-1537 broker was less strict while relo...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1688
 
    @stanlyDoge all merged, thanks for this.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1688: ARTEMIS-1537 broker was less strict while relo...

pgfox
In reply to this post by pgfox
Github user jdanekrh commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1688
 
    wow!
    such fix!
    thanks much!


---