[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
GitHub user iweiss opened a pull request:

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

    [ARTEMIS-1819] Missing fields on listAllConsumersAsJSON, listConsumersAsJSON and listConnectionsAsJSON

    Issue: https://issues.apache.org/jira/browse/ARTEMIS-1819

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/iweiss/activemq-artemis ARTEMIS-1819

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2035.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 #2035
   
----
commit ae148fa72d5544ae236530ab03223d9c955abef2
Author: Unknown <ingo@...>
Date:   2018-04-20T07:44:07Z

    [ARTEMIS-1819] Missing fields on listAllConsumersAsJSON, listConsumersAsJSON and listConnectionsAsJSON
   
    Issue: https://issues.apache.org/jira/browse/ARTEMIS-1819

----


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
Github user iweiss commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    The failure doesn't look related to the patch


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    Couple of points & questions:
    * This PR is putting JMS-specific logic into the core broker where it doesn't belong.
    * Is the `clientID` being returned here supposed to be the JMS client ID? The client ID on the connection object being retrieved is not the JMS client ID. The JMS client ID is set on the core session's internal meta-data.
    * The JMS prefixes that are being used here to determine the  are no longer valid as they were removed in 2.0.
    * The test is only validating that the fields exist in the reply and not that they actually have accurate values.
    * Is the new `durable` flag meant to be some kind of indication about a JMS durable subscriber? The value for `durable` being returned is for the queue to which the consumer is attached which isn't directly related to JMS durable subscriptions.


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r183194317
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
    @@ -1830,7 +1830,18 @@ public String listConnectionsAsJSON() throws Exception {
              Set<RemotingConnection> connections = server.getRemotingService().getConnections();
     
              for (RemotingConnection connection : connections) {
    -            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("connectionID", connection.getID().toString()).add("clientAddress", connection.getRemoteAddress()).add("creationTime", connection.getCreationTime()).add("implementation", connection.getClass().getSimpleName()).add("sessionCount", server.getSessions(connection.getID().toString()).size());
    +            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("connectionID", connection.getID().toString())
    +               .add("clientAddress", connection.getRemoteAddress())
    +               .add("creationTime", connection.getCreationTime())
    +               .add("implementation", connection.getClass().getSimpleName())
    --- End diff --
   
    Please avoid reformatting like this, there's no code change here


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r183194324
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
    @@ -1849,7 +1860,9 @@ public String listSessionsAsJSON(final String connectionID) throws Exception {
           try {
              List<ServerSession> sessions = server.getSessions(connectionID);
              for (ServerSession sess : sessions) {
    -            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID", sess.getName()).add("creationTime", sess.getCreationTime()).add("consumerCount", sess.getServerConsumers().size());
    +            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID", sess.getName())
    +               .add("creationTime", sess.getCreationTime())
    --- End diff --
   
    Please avoid reformatting like this, there's no code change here


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r183194331
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
    @@ -1873,7 +1886,9 @@ public String listAllSessionsAsJSON() throws Exception {
           try {
              Set<ServerSession> sessions = server.getSessions();
              for (ServerSession sess : sessions) {
    -            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID", sess.getName()).add("creationTime", sess.getCreationTime()).add("consumerCount", sess.getServerConsumers().size());
    +            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID", sess.getName())
    +               .add("creationTime", sess.getCreationTime())
    --- End diff --
   
    Please avoid reformatting like this, there's no code change here


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r183194361
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
    @@ -1943,11 +1958,43 @@ public String listAllConsumersAsJSON() throws Exception {
        }
     
        private JsonObject toJSONObject(ServerConsumer consumer) throws Exception {
    -      JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("consumerID", consumer.getID()).add("connectionID", consumer.getConnectionID().toString()).add("sessionID", consumer.getSessionID()).add("queueName", consumer.getQueue().getName().toString()).add("browseOnly", consumer.isBrowseOnly()).add("creationTime", consumer.getCreationTime()).add("deliveringCount", consumer.getDeliveringMessages().size());
    +      JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("consumerID", consumer.getID())
    +         .add("connectionID", consumer.getConnectionID().toString())
    +         .add("sessionID", consumer.getSessionID())
    --- End diff --
   
    Please avoid reformatting like this, there's no code change here


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r183194622
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
    @@ -1943,11 +1958,43 @@ public String listAllConsumersAsJSON() throws Exception {
        }
     
        private JsonObject toJSONObject(ServerConsumer consumer) throws Exception {
    -      JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("consumerID", consumer.getID()).add("connectionID", consumer.getConnectionID().toString()).add("sessionID", consumer.getSessionID()).add("queueName", consumer.getQueue().getName().toString()).add("browseOnly", consumer.isBrowseOnly()).add("creationTime", consumer.getCreationTime()).add("deliveringCount", consumer.getDeliveringMessages().size());
    +      JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("consumerID", consumer.getID())
    +         .add("connectionID", consumer.getConnectionID().toString())
    +         .add("sessionID", consumer.getSessionID())
    +         .add("queueName", consumer.getQueue().getName().toString())
    +         .add("browseOnly", consumer.isBrowseOnly())
    +         .add("creationTime", consumer.getCreationTime())
    +         .add("deliveringCount", consumer.getDeliveringMessages().size());
           if (consumer.getFilter() != null) {
              obj.add("filter", consumer.getFilter().getFilterString().toString());
           }
     
    +      String coreAddress = consumer.getQueue().getAddress().toString();
    +
    +      String[] result = new String[2]; // destination name & type
    --- End diff --
   
    Please remove this is brittle code based on jms prefixes which is bad, broker uses core model. Saying that this is address and queue level information (e.g. Multicast vs Anycast), not consumer


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r183194685
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
    @@ -1943,11 +1958,43 @@ public String listAllConsumersAsJSON() throws Exception {
        }
     
        private JsonObject toJSONObject(ServerConsumer consumer) throws Exception {
    -      JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("consumerID", consumer.getID()).add("connectionID", consumer.getConnectionID().toString()).add("sessionID", consumer.getSessionID()).add("queueName", consumer.getQueue().getName().toString()).add("browseOnly", consumer.isBrowseOnly()).add("creationTime", consumer.getCreationTime()).add("deliveringCount", consumer.getDeliveringMessages().size());
    +      JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("consumerID", consumer.getID())
    +         .add("connectionID", consumer.getConnectionID().toString())
    +         .add("sessionID", consumer.getSessionID())
    +         .add("queueName", consumer.getQueue().getName().toString())
    +         .add("browseOnly", consumer.isBrowseOnly())
    +         .add("creationTime", consumer.getCreationTime())
    +         .add("deliveringCount", consumer.getDeliveringMessages().size());
           if (consumer.getFilter() != null) {
              obj.add("filter", consumer.getFilter().getFilterString().toString());
           }
     
    +      String coreAddress = consumer.getQueue().getAddress().toString();
    +
    +      String[] result = new String[2]; // destination name & type
    +      if (coreAddress.startsWith("jms.queue.")) {
    +         result[0] = coreAddress.substring("jms.queue.".length());
    +         result[1] = "queue";
    +      } else if (coreAddress.startsWith("jms.tempqueue.")) {
    +         result[0] = coreAddress.substring("jms.tempqueue.".length());
    +         result[1] = "tempqueue";
    +      } else if (coreAddress.startsWith("jms.topic.")) {
    +         result[0] = coreAddress.substring("jms.topic.".length());
    +         result[1] = "topic";
    +      } else if (coreAddress.startsWith("jms.temptopic.")) {
    +         result[0] = coreAddress.substring("jms.temptopic.".length());
    +         result[1] = "temptopic";
    +      } else {
    +         logger.debug("Unable to determine the JMS destination of " + coreAddress);
    +         // not related to JMS
    +         result[0] = "";
    +         result[1] = "";
    +      }
    +
    +      obj.add("destinationName", result[0])
    +         .add("destinationType", result[1])
    +         .add("durable", consumer.getQueue().isDurable());
    --- End diff --
   
    This isn't consumer info, this is on the queue level information, please remove


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user iweiss commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    @michaelandrepearce @jbertram Thanks for your comments. Where would be the best place to add this information back to a consumer of these methods?


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    It’s on the address and queue information


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    I think the first thing to clarify is exactly what information is missing which your clients need.


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user iweiss commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    The information missing is some JMS queue information, as follows: destinationName for the name of destination e.g InQueue (not core name or address), destinationType - queue or topic, durable is the durability of consumer and clientId is JMS client ID. Could this be provided on JMSServerControlImpl?


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    The broker no longer makes any special distinction between JMS and non-JMS destinations. There are only core addresses, queues, and routing types.
    * The name of the destination to which a JMS client sends a message should correspond to the name of a core address.
    * The destination type will correspond to the address's routing type - MULTICAST for topic, ANYCAST for queue.
    * The JMS client ID can be pulled from the core session's meta-data (see https://issues.apache.org/jira/browse/ARTEMIS-1769 for more info on that point).


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user iweiss commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r183963414
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
    @@ -1849,7 +1860,9 @@ public String listSessionsAsJSON(final String connectionID) throws Exception {
           try {
              List<ServerSession> sessions = server.getSessions(connectionID);
              for (ServerSession sess : sessions) {
    -            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID", sess.getName()).add("creationTime", sess.getCreationTime()).add("consumerCount", sess.getServerConsumers().size());
    +            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID", sess.getName())
    +               .add("creationTime", sess.getCreationTime())
    --- End diff --
   
    Sorry, but may I ask why? I think increasing legibility is a good thing.


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r184039273
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
    @@ -1849,7 +1860,9 @@ public String listSessionsAsJSON(final String connectionID) throws Exception {
           try {
              List<ServerSession> sessions = server.getSessions(connectionID);
              for (ServerSession sess : sessions) {
    -            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID", sess.getName()).add("creationTime", sess.getCreationTime()).add("consumerCount", sess.getServerConsumers().size());
    +            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID", sess.getName())
    +               .add("creationTime", sess.getCreationTime())
    --- End diff --
   
    I don't want to speak for @michaelandrepearce, but personally I find it much easier to review PRs when all the changes in the PR are directly related. This PR is about adding missing fields to some management methods not increasing code legibility. In general, increasing code legibility is a good thing, but it can be detrimental in non-obvious ways (e.g. when lots of legibility changes make it difficult to back-port fixes to maintenance branches) and IMO changes which are exclusively for improving legibility should be in their own commits. Finally, "legibility" can be subjective. You risk having your functional PR rejected by including non-functional, subjective "improvements."


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user iweiss commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    The failure doesn't look related to this updated PR


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    Unfortunately the updated PR still suffers from importing JMS nomenclature into core management operations. The core broker doesn't have destination names or destination types. It has addresses, queues, and routing types. You should be doing something like this:
   
    `obj.add("addressName", consumer.getQueue().getAddress().toString());`
    `obj.add("queueRoutingType", consumer.getQueueType().toString());`
    `obj.add("queueDurable", consumer.getQueue().isDurable());`
   
    Then on the client you can interpret these values as appropriate for the protocol/API you're using.


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

[GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...

gaohoward-3
In reply to this post by gaohoward-3
Github user asfgit closed the pull request at:

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


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

[GitHub] activemq-artemis issue #2035: [ARTEMIS-1819] Missing fields on listAllConsum...

gaohoward-3
In reply to this post by gaohoward-3
Github user jmesnil commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2035
 
    @jbertram thanks for the suggestion.
   
    In WildFly, we get all these information as a String blob from https://github.com/apache/activemq-artemis/blob/acc34d90881313d5c94a5a377aac9438175e55ef/artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java#L890
   
    In addition to this PR, we would have to unmarshall the JSON string, change it to put back the JMS semantic and marshall it again before to send it to the management client.
    This seems brittle to rely on a JSON string for this information.
   
    @jbertram Would it be possible to enhance ActiveMQServerControl so that it returns a proper DTO containing all the information? In WildFly we can then take care of all the transformation and json serialization but we would have a proper API contract for all consumer and connection data coming from ActiveMQServerControl


---
12