You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/12/16 16:15:36 UTC
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4183: ARTEMIS-3875 - adding consumer and producer metrics
gemmellr commented on code in PR #4183:
URL: https://github.com/apache/activemq-artemis/pull/4183#discussion_r1050833137
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java:
##########
@@ -115,6 +114,7 @@ public class ServerSessionImpl implements ServerSession, FailureListener {
private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private static final int MAX_PRODUCER_METRIC_SIZE = 100;
Review Comment:
Looks unused
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java:
##########
@@ -2010,14 +1979,21 @@ public StorageManager getStorageManager() {
@Override
public void describeProducersInfo(JsonArrayBuilder array) throws Exception {
- Map<SimpleString, Pair<Object, AtomicLong>> targetCopy = cloneTargetAddresses();
-
- for (Map.Entry<SimpleString, Pair<Object, AtomicLong>> entry : targetCopy.entrySet()) {
+ Map<String, ServerProducer> targetCopy = cloneProducers();
+ for (Map.Entry<String, ServerProducer> entry : targetCopy.entrySet()) {
String uuid = null;
- if (entry.getValue().getA() != null) {
- uuid = entry.getValue().getA().toString();
+ if (entry.getValue().getLastProducedMessageID() != null) {
+ uuid = entry.getValue().getLastProducedMessageID().toString();
}
- JsonObjectBuilder producerInfo = JsonLoader.createObjectBuilder().add("connectionID", this.getConnectionID().toString()).add("sessionID", this.getName()).add("destination", entry.getKey().toString()).add("lastUUIDSent", uuid, JsonValue.NULL).add("msgSent", entry.getValue().getB().longValue());
+ JsonObjectBuilder producerInfo = JsonLoader.createObjectBuilder()
+ .add(ProducerField.ID.getName(), getName())
+ .add(ProducerField.CONNECTION_ID.getName(), this.getConnectionID().toString())
+ .add(ProducerField.SESSION.getAlternativeName(), this.getName())
Review Comment:
The ID and SESSION keys are being given the same value, which will be the same for every producer on the session, seems off.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/CreateProducerMessage.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.
+ */
Review Comment:
Should be a comment rather than javadoc, also remove \<p\> tags and and fix indents. E.g see https://github.com/apache/activemq-artemis/blob/main/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java#L1-L16
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java:
##########
@@ -2395,10 +2364,11 @@ public Pair<SimpleString, EnumSet<RoutingType>> getAddressAndRoutingTypes(Simple
}
@Override
- public void addProducer(ServerProducer serverProducer) {
- serverProducer.setSessionID(getName());
- serverProducer.setConnectionID(getConnectionID().toString());
- producers.put(serverProducer.getID(), serverProducer);
+ public void addProducer(String id, String protocol, String address) {
+ ServerProducer producer = new ServerProducerImpl(id.toString(), protocol, address != null ? address.toString() : "ANONYMOUS");
Review Comment:
id.toString() is redundant since it is passed a String...similarly with address.toString()
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/RemoveProducerMessage.java:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.
+ */
Review Comment:
Ditto
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java:
##########
@@ -490,6 +489,7 @@ public void serverSend(ProtonServerReceiverContext context,
}
} else {
message.setConnectionID(receiver.getSession().getConnection().getRemoteContainer());
+ message.setproducerID(receiver.getName());
Review Comment:
The link name is only required to be unique between any two container ID's [in a given direction, so there can be one sender and receiver with same name], so e.g different clients can use the same link names on different connections.
It might be better for the broker to assign a conceptual ID to each producer entity for its own tracking purposes, as it does for the messages, which would then also be able to be handled consistently across protocols (may take up less memory also). The 'names' where they exist could still be given in the producer details still for reference purposes.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/view/ProducerField.java:
##########
@@ -21,15 +21,19 @@
public enum ProducerField {
ID("id"),
- SESSION("session"),
+ SESSION("session", "sessionID"),
CONNECTION_ID("connectionID"),
- ADDRESS("address"), USER("user"),
+ ADDRESS("address", "destination"),
+ USER("user"),
VALIDATED_USER("validatedUser"),
PROTOCOL("protocol"),
CLIENT_ID("clientID"),
LOCAL_ADDRESS("localAddress"),
REMOTE_ADDRESS("remoteAddress"),
- CREATION_TIME("creationTime");
+ CREATION_TIME("creationTime"),
+ MESSAGE_SENT("msgSent"),
+ MESSAGE_SENT_SIZE("msgSizeSent"),
+ LAST_PRODUCED_MESSAGE_ID_SENT("lastProducedMessageID");
Review Comment:
having both produced and sent seems redundant
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java:
##########
@@ -2010,14 +1979,21 @@ public StorageManager getStorageManager() {
@Override
public void describeProducersInfo(JsonArrayBuilder array) throws Exception {
- Map<SimpleString, Pair<Object, AtomicLong>> targetCopy = cloneTargetAddresses();
-
- for (Map.Entry<SimpleString, Pair<Object, AtomicLong>> entry : targetCopy.entrySet()) {
+ Map<String, ServerProducer> targetCopy = cloneProducers();
+ for (Map.Entry<String, ServerProducer> entry : targetCopy.entrySet()) {
String uuid = null;
- if (entry.getValue().getA() != null) {
- uuid = entry.getValue().getA().toString();
+ if (entry.getValue().getLastProducedMessageID() != null) {
+ uuid = entry.getValue().getLastProducedMessageID().toString();
}
- JsonObjectBuilder producerInfo = JsonLoader.createObjectBuilder().add("connectionID", this.getConnectionID().toString()).add("sessionID", this.getName()).add("destination", entry.getKey().toString()).add("lastUUIDSent", uuid, JsonValue.NULL).add("msgSent", entry.getValue().getB().longValue());
+ JsonObjectBuilder producerInfo = JsonLoader.createObjectBuilder()
+ .add(ProducerField.ID.getName(), getName())
+ .add(ProducerField.CONNECTION_ID.getName(), this.getConnectionID().toString())
+ .add(ProducerField.SESSION.getAlternativeName(), this.getName())
+ .add(ProducerField.CREATION_TIME.getName(), "" + creationTime)
Review Comment:
Concating this on every execution seems potentially wasteful when the value is fixed....which on thinking about it, also seems off, as it will then also be the same for every producer on the session.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java:
##########
@@ -279,7 +279,7 @@ public void testForTempQueueTargetInfosLeak() throws Exception {
producer.send(s.createMessage());
temporaryQueue.delete();
for (ServerSession serverSession : server.getSessions()) {
- assertFalse(((ServerSessionImpl)serverSession).cloneTargetAddresses().containsKey(SimpleString.toSimpleString(temporaryQueue.getQueueName())));
+ assertFalse(((ServerSessionImpl)serverSession).cloneProducers().containsKey(temporaryQueue.getQueueName()));
Review Comment:
The producers map isnt keyed by address as the old map was, so it probably isnt really asserting anything useful here now with the change made. It would need to check the actual producer addresses.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org