gemmellr commented on code in PR #4126:
URL: https://github.com/apache/activemq-artemis/pull/4126#discussion_r909660602
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java:
##########
@@ -1770,8 +1770,26 @@ public String listConsumersAsJSON() throws Exception {
if (consumer instanceof ServerConsumer) {
ServerConsumer serverConsumer = (ServerConsumer) consumer;
-
- JsonObjectBuilder obj =
JsonLoader.createObjectBuilder().add("consumerID",
serverConsumer.getID()).add("connectionID",
serverConsumer.getConnectionID().toString()).add("sessionID",
serverConsumer.getSessionID()).add("browseOnly",
serverConsumer.isBrowseOnly()).add("creationTime",
serverConsumer.getCreationTime());
+ List<MessageReference> deliveringMessages =
consumer.getDeliveringMessages();
+ long deliveringMessageSize = 0;
+ for (int i = 0; i < deliveringMessages.size(); i++) {
+ MessageReference messageReference =
deliveringMessages.get(i);
+ deliveringMessageSize +=
messageReference.getMessage().getEncodeSize();
+ }
+ long currentTime = System.currentTimeMillis();
+ long lastDeliveredTimeElapsed =
serverConsumer.getLastDeliveredTime() == 0 ? 0 : currentTime -
serverConsumer.getLastDeliveredTime();
+ long lastAckTimeElapsed =
serverConsumer.getLastAcknowledgedTime() == 0 ? 0 : currentTime -
serverConsumer.getLastAcknowledgedTime();
+ JsonObjectBuilder obj = JsonLoader.createObjectBuilder()
+ .add("consumerID", serverConsumer.getID())
+ .add("connectionID",
serverConsumer.getConnectionID().toString())
+ .add("sessionID", serverConsumer.getSessionID())
+ .add("browseOnly", serverConsumer.isBrowseOnly())
+ .add("creationTime", serverConsumer.getCreationTime())
+ .add("deliveringCount", deliveringMessages.size())
+ .add("deliveringSize", deliveringMessageSize)
+ .add("messagesAcknowledged",
serverConsumer.getMessagesAcknowledged())
+ .add("lastDeliveredTimeElapsed", lastDeliveredTimeElapsed)
+ .add("lastAcknowledgedTimeElapsed", lastAckTimeElapsed);
Review Comment:
Appears to be a dupe of the bit from ActiveMQServerControlImpl? Meaning both
will be broken in the same way currently. Seems like there is room for a shared
impl.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -2752,7 +2753,27 @@ 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());
+ List<MessageReference> deliveringMessages =
consumer.getDeliveringMessages();
+ long deliveringMessageSize = 0;
+ for (int i = 0; i < deliveringMessages.size(); i++) {
+ MessageReference messageReference = deliveringMessages.get(i);
+ deliveringMessageSize +=
messageReference.getMessage().getEncodeSize();
+ }
+ long currentTime = System.currentTimeMillis();
+ long lastDeliveredTimeElapsed = consumer.getLastDeliveredTime() == 0 ? 0
: currentTime - consumer.getLastDeliveredTime();
+ long lastAckTimeElapsed = consumer.getLastAcknowledgedTime() == 0 ? 0 :
currentTime - consumer.getLastAcknowledgedTime();
+ 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", deliveringMessages.size())
+ .add("deliveringSize", deliveringMessageSize)
+ .add("messagesAcknowledged", consumer.getMessagesAcknowledged())
+ .add("lastDeliveredTimeElapsed", lastDeliveredTimeElapsed)
+ .add("lastAcknowledgedTimeElapsed", lastAckTimeElapsed);
Review Comment:
Screaming out for some constants.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/view/ConsumerView.java:
##########
@@ -111,6 +128,27 @@ public Object getField(ServerConsumer consumer, String
fieldName) {
return consumer.getConnectionRemoteAddress();
case CREATION_TIME:
return new Date(consumer.getCreationTime());
+ case DELIVERING_COUNT:
+ return consumer.getDeliveringMessages().size();
+ case DELIVERING_SIZE:
+ List<MessageReference> deliveringMessages =
consumer.getDeliveringMessages();
+ long deliveringMessageSize = 0;
+ for (int i = 0; i < deliveringMessages.size(); i++) {
+ MessageReference messageReference = deliveringMessages.get(i);
+ deliveringMessageSize +=
messageReference.getMessage().getEncodeSize();
+ }
+ return deliveringMessageSize;
+ case MESSAGES_ACKNOWLEDGED:
+ return consumer.getMessagesAcknowledged();
+ case LAST_DELIVERED_TIME_ELAPSED: {
+ long currentTime = System.nanoTime();
+ return consumer.getLastDeliveredTime() == 0 ? 0 : currentTime -
consumer.getLastDeliveredTime();
Review Comment:
Looks to be mixing nano and millis sourced values in a way thats broken.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ConsumerInfo.java:
##########
@@ -62,4 +62,28 @@ public interface ConsumerInfo {
*/
String getConnectionRemoteAddress();
+ /**
+ * Returns how many messages are out for delivery but not yet acknowledged
+ * @return delivering count
+ */
+ int getDeliveringCount();
+
+ /**
+ * Returns the time in nano seconds that the last message was delivered to
a consumer
Review Comment:
The impl is dealing in milliseconds in various places, but nano in
others...some of the places are wrong and need updated. Ditto for those below.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -2752,7 +2753,27 @@ 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());
+ List<MessageReference> deliveringMessages =
consumer.getDeliveringMessages();
+ long deliveringMessageSize = 0;
+ for (int i = 0; i < deliveringMessages.size(); i++) {
+ MessageReference messageReference = deliveringMessages.get(i);
+ deliveringMessageSize +=
messageReference.getMessage().getEncodeSize();
+ }
+ long currentTime = System.currentTimeMillis();
Review Comment:
This is using currentTimeMillis, which is epoch based millis....elsewhere
consumer.getLastDeliveredTime() and consumer.getLastAcknowledgedTime() are
documented as and sometimes being treated as nanoTime() values which thus have
no defined starting point. Those two cant be used together in the way this
does, one of the area seems like it has to be wrong. Elsewhere still it seems
those values are actually using currentTimeMillis() despite the doc they are
nanos.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -2713,6 +2714,10 @@ public void testListAllConsumersAsJSON() throws
Exception {
Assert.assertEquals(queueName.toString(), first.getString("queueName"));
Assert.assertEquals(false, first.getBoolean("browseOnly"));
Assert.assertEquals(0,
first.getJsonNumber("deliveringCount").longValue());
+ Assert.assertEquals(0,
first.getJsonNumber("deliveringSize").longValue());
+ Assert.assertEquals(0,
first.getJsonNumber("messagesAcknowledged").longValue());
+ Assert.assertEquals(0,
first.getJsonNumber("lastDeliveredTimeElapsed").longValue());
+ Assert.assertEquals(0,
first.getJsonNumber("lastAcknowledgedTimeElapsed").longValue());
Review Comment:
constants would be nice..
##########
artemis-hawtio/artemis-plugin/src/main/webapp/plugin/js/components/consumers.js:
##########
@@ -95,7 +95,12 @@ var Artemis;
{name: "Address", visible: true},
{name: "Remote Address", visible: true},
{name: "Local Address", visible: true},
- {name: "Creation Time", visible: true}
+ {name: "Creation Time", visible: true},
+ {name: "Delivering Count", visible: false},
+ {name: "Delivering Size", visible: false},
+ {name: "Messages Acknowledged", visible: false},
+ {name: "Last Delivered Time Elapsed", visible: false},
+ {name: "Last Acknowledged Time Elapsed", visible: false}
Review Comment:
Seems like a lot of overhead to be added for columns that arent even visible
normally?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
