gemmellr commented on code in PR #4183:
URL: https://github.com/apache/activemq-artemis/pull/4183#discussion_r952714533
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerConsumerImpl.java:
##########
@@ -506,8 +510,10 @@ public void proceedDeliver(MessageReference reference)
throws Exception {
// The deliverer was prepared during handle, as we can't have more
than one pending large message
// as it would return busy if there is anything pending
largeMessageDeliverer.deliver();
+ //lastDeliveredTime = System.currentTimeMillis();
} else {
deliverStandardMessage(reference, message);
+ // lastDeliveredTime = System.currentTimeMillis();
Review Comment:
Leftover?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/AbstractControl.java:
##########
@@ -50,8 +51,15 @@ public AbstractControl(final Class<?> clazz, final
StorageManager storageManager
this.storageManager = storageManager;
}
+ public long getLastDeliveredTimeElapsed(ServerConsumer consumer) {
+ long currentTime = System.currentTimeMillis();
+ return consumer.getLastDeliveredTime() == 0 ? 0 : currentTime -
consumer.getLastDeliveredTime();
Review Comment:
This (and one below) seems like it will be subject to wall clock movement
between the last delivery(/ack) and the getter being called. You could either
just accept that, or gate it so it cant be negative, or do some nanoTime
dancing to avoid it and report a true elapsed, or perhaps could just punt it by
reporting a millis 'Time' value directly like e.g various message header values
do for e.g timestamp/expiration.
The latter would make it clear whether getting a value of 0 means 'never
delivered' or 'delivered in last millisecond' which cant be distinguished when
providing 'elapsed'...though could be inferred from observing the other metrics
or their change over time.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerConsumerImpl.java:
##########
@@ -506,8 +510,10 @@ public void proceedDeliver(MessageReference reference)
throws Exception {
// The deliverer was prepared during handle, as we can't have more
than one pending large message
// as it would return busy if there is anything pending
largeMessageDeliverer.deliver();
+ //lastDeliveredTime = System.currentTimeMillis();
Review Comment:
Leftover?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -2631,7 +2633,7 @@ public void testListConsumersAsJSON() throws Exception {
Assert.assertEquals(queueName.toString(), first.getString("queueName"));
Assert.assertEquals(false, first.getBoolean("browseOnly"));
Assert.assertTrue(first.getJsonNumber("creationTime").longValue() > 0);
- Assert.assertEquals(0,
first.getJsonNumber("deliveringCount").longValue());
+ Assert.assertEquals(0,
first.getJsonNumber("messagesInTransit").longValue());
Review Comment:
Per earlier comment, should probably be a check for the 'maintained for
compatibility' name, otherwise it might not be. Also test could use the
constants.
(Same seems to apply a few times further down)
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -2752,7 +2754,24 @@ 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();
+ JsonObjectBuilder obj = JsonLoader.createObjectBuilder()
+ .add(ConsumerField.ID.getAlternativeName(), consumer.getID())
+ .add(ConsumerField.CONNECTION.getAlternativeName(),
consumer.getConnectionID().toString())
+ .add(ConsumerField.SESSION.getAlternativeName(),
consumer.getSessionID())
+ .add(ConsumerField.QUEUE.getAlternativeName(),
consumer.getQueue().getName().toString())
+ .add(ConsumerField.BROWSE_ONLY.getName(), consumer.isBrowseOnly())
+ .add(ConsumerField.CREATION_TIME.getName(),
consumer.getCreationTime())
+ // deliveringCount is renamed as MESSAGES_IN_TRANSIT but left in
json for backward compatibility
+ .add(ConsumerField.MESSAGES_IN_TRANSIT.getAlternativeName(),
consumer.getMessagesInTransit())
+ .add(ConsumerField.MESSAGES_IN_TRANSIT.getName(),
consumer.getMessagesInTransit())
Review Comment:
There should be a test for that if its considered important for
compatibility. Looks more like the existing test references for the name were
removed?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java:
##########
@@ -2242,13 +2246,14 @@ public synchronized RoutingStatus doSend(final
Transaction tx,
result = postOffice.route(msg, routingContext, direct);
- Pair<Object, AtomicLong> value =
targetAddressInfos.get(msg.getAddressSimpleString());
+ Pair<Object, TargetAddressInfo> value =
targetAddressInfos.get(msg.getAddressSimpleString());
if (value == null) {
- targetAddressInfos.put(msg.getAddressSimpleString(), new
Pair<>(msg.getUserID(), new AtomicLong(1)));
+ targetAddressInfos.put(msg.getAddressSimpleString(), new
Pair<>(msg.getUserID(), new TargetAddressInfo()));
Review Comment:
This relies on the constructor setting 'sent' to 1, but doesnt seem to
increase the 'size' at all, like the 'else' branch does, which suggests it will
be incorrect.
If so also points to a testing hole, e.g sending multiple messages and only
checking for >0 and so not noticing it is out.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -3586,6 +4115,14 @@ public void testListConsumers() throws Exception {
Assert.assertNotEquals("localAddress", "",
jsonConsumer.getString("localAddress"));
Assert.assertNotEquals("remoteAddress", "",
jsonConsumer.getString("remoteAddress"));
Assert.assertNotEquals("creationTime", "",
jsonConsumer.getString("creationTime"));
+ Assert.assertNotEquals("messagesInTransit", "",
jsonConsumer.getString("messagesInTransit"));
+ Assert.assertNotEquals("messagesInTransitSize", "",
jsonConsumer.getString("messagesInTransitSize"));
+ Assert.assertNotEquals("messageDelivered", "",
jsonConsumer.getString("messageDelivered"));
+ Assert.assertNotEquals("messagesDeliveredSize", "",
jsonConsumer.getString("messagesDeliveredSize"));
+ Assert.assertNotEquals("messagesAcknowledged", "",
jsonConsumer.getString("messagesAcknowledged"));
+ Assert.assertNotEquals("lastDeliveredTimeElapsed", "",
jsonConsumer.getString("lastDeliveredTimeElapsed"));
+ Assert.assertNotEquals("lastAcknowledgedTimeElapsed", "",
jsonConsumer.getString("lastAcknowledgedTimeElapsed"));
Review Comment:
Constants as per other tests?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/TargetAddressInfo.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.server.impl;
+
+
+import java.util.concurrent.atomic.AtomicLong;
+
+class TargetAddressInfo {
+ private AtomicLong messagesSent = new AtomicLong(1);
+
+ private AtomicLong messagesSizeSent = new AtomicLong(0);
Review Comment:
Might be worth a comment (or a fix instead?) explaining why sent starts at 1?
--
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]
