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

[michaelandrepearce]

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


---

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

[michaelandrepearce]

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 


---

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

[michaelandrepearce]

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 


---

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

[michaelandrepearce]

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 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 


---

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

[michaelandrepearce]

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 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 


---

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

[michaelandrepearce]

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 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 


---

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

[jbertram]

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.


---

[GitHub] activemq-artemis pull request #2036: ARTEMIS-1821 LDAPLoginModule always ret...

[jbertram]

GitHub user jbertram opened a pull request:

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

ARTEMIS-1821 LDAPLoginModule always returns true on commit()



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

$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-1821

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

https://github.com/apache/activemq-artemis/pull/2036.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 #2036


commit 92e55fce2635c0609abd355be4f6ea4072574959
Author: Justin Bertram 
Date:   2018-04-20T18:58:53Z

ARTEMIS-1821 LDAPLoginModule always returns true on commit()




---

[GitHub] activemq-artemis pull request #2031: ARTEMIS-1816 OpenWire should avoid Byte...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[tabish121]

Github user tabish121 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
Ok, I'll be "the decider".  Will check with franz to see how much it's 
worth to him


---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
@tabish121  no regressions on the testsuites.. I will let you decide about 
merging this or not.


---

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

[iweiss]

Github user iweiss commented on the issue:

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


---

Re: [DISCUSS] Separate commit for Test and Fix on PRs

[Martyn Taylor]

I think having test that don't compile due to code changes is an
exception.  I think we should strive for this approach.  Clebert, I follow
the same process you described, I undo the change then run the test to see
what was wrong and that the test is valid.  In my opinion this is a
positive thing to strive to.  Obviously common sense should prevail, and if
there are some reason it's not possible or very difficult then we can relax
the rule.

On Thu, Apr 19, 2018 at 10:13 PM, Clebert Suconic  wrote:

> will I sound crazy if I change my mind here?
>
> I thought it would be improving the PR process.. but on a second
> thought... it won't improve things actually and i will agree with
> Robbie here.
>
>  So. .just ignore me.. and apologize for spamming  you guys..
>
> On Thu, Apr 19, 2018 at 10:48 AM, Timothy Bish 
> wrote:
> > On 04/19/2018 10:32 AM, Robbie Gemmell wrote:
> >>
> >> I dont think its unreasonable or unexpected in many cases that a test
> >> fails to compile without the changes in relates to. It depends what
> >> type of test it is and what the changes actually are though.
> >>
> >> I agree it wont hugely change the PR though. Possibly why I prefer
> >> them being in the same commit is I tend to use the commit to look over
> >> things rather than the PR.
> >
> >
> > The other thing to keep in mind is that when two or more commits for the
> > same bit of work go in, the process of reverting changes becomes more
> error
> > prone as the person doing the reverts has to always be looking for the
> cases
> > where there was more than one related to the same scope of work.
> >
> >
> >> On 19 April 2018 at 15:10, Clebert Suconic 
> >> wrote:
> >>>
> >>> I have seen a few cases where the test would not compile.. that is the
> >>> test depends on the changes itself. What is not really Test Driven
> >>> Development.
> >>>
> >>> Both tests and fixes are part of the same PR.. so it doesn't really
> >>> change much.
> >>>
> >>> On Thu, Apr 19, 2018 at 9:41 AM, Robbie Gemmell
> >>>  wrote:
> 
>  In general I think having tests and changes in the same commit is
>  nicer, especially for looking back at later.
> 
>  I'll also often apply a test on its own or revert the non-test changes
>  to ensure tests fail, I've not really found it slow/annoying enough to
>  specifically seperate tests out in their own commits to facilitate it.
> 
>  Robbie
> 
>  On 18 April 2018 at 18:27, Clebert Suconic  >
>  wrote:
> >
> > I would appreciate if we separated fixes and tests on Pull Requests.
> >
> >
> > A lot of times i will revert the fixes to validate if the test is
> good
> > (if it fails without a fix) and how it failed. (not that I don't
> trust
> > the committer, just part of the validation as sometimes I want to see
> > what was the semantic change and fix). I may eventually play a better
> > fix in the process.. and I am sure that would apply to anyone else
> > helping on reviewing commits.
> >
> >
> > I had at some point gone back in history and needed to apply the test
> > without a fix to find a better fix.
> >
> > I know eventually it's not possible to separate these.. but if you
> > could as much as possible separate them:?
> >
> >
> > I recently did that into PR #2004...
> >
> >
> > https://github.com/apache/activemq-artemis/commit/
> a72046a0e32fd47cad988a8d71512927f74c8585
> >
> >
> > https://github.com/apache/activemq-artemis/commit/
> a72046a0e32fd47cad988a8d71512927f74c8585
> >
> >
> >
> >
> > I may update the hacking guide with this.. WDYT?
> >
> >
> > --
> > Clebert Suconic
> >>>
> >>>
> >>>
> >>> --
> >>> Clebert Suconic
> >
> >
> >
> > --
> > Tim Bish
> > twitter: @tabish121
> > blog: http://timbish.blogspot.com/
> >
>
>
>
> --
> Clebert Suconic
>

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

[iweiss]

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 
Date:   2018-04-20T07:44:07Z

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

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




---

[GitHub] activemq-artemis issue #2034: ARTEMIS-1818 re-create auto-created queue on J...

[Snurppa]

Github user Snurppa commented on the issue:

https://github.com/apache/activemq-artemis/pull/2034
  
Wow that was fast, big thanks @jbertram 👍


---