gemmellr commented on code in PR #4183:
URL: https://github.com/apache/activemq-artemis/pull/4183#discussion_r1066901799
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java:
##########
@@ -101,6 +101,8 @@ public final class ClientSessionImpl implements
ClientSessionInternal, FailureLi
private final boolean blockOnAcknowledge;
+ private final boolean sendProducerID = true;
+
Review Comment:
This appears unused
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/Message.java:
##########
@@ -408,6 +410,8 @@ default Message setValidatedUserID(String validatedUserID) {
String getAddress();
+ String getProducerID();
+
Review Comment:
Given the ID stuff we discussed, I wasnt really expecting the Message to
have this, but just any broker-internals needing the detail to use the unique
mapped broker-internal long producer ID, rather than the possibly-not-unique
'name' strings. It seems the long id is really only confined to the metrics
tracking ServerProducerImpl tracking bit, used for returning the list to
management, rather than being known by the bits actually producing/ingesting
the messages in the broker which is more what I was thinking, e.g
ProtonServerReceiverContext for the AMQP bits, and possibly some of the Session
bits for other protocols.
If going with the Message having "String getProducerID();" I think it needs
to be documented clearly that this is not ensured to be a unique value per
producer. I'd probably change it to getProducerName also to assist with that.
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenwireMessage.java:
##########
@@ -137,6 +137,11 @@ public String getAddress() {
return null;
}
+ @Override
+ public String getProducerID() {
+ return null;
Review Comment:
Always null? Seems off.
EDIT: The entire interface has a TODO suggesting it unused, and looking that
appears true. Delete..?
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java:
##########
@@ -392,7 +392,7 @@ public void send(final ProducerInfo producerInfo,
assert
clientId.toString().equals(this.connection.getState().getInfo().getClientId())
: "Session cached clientId must be the same of the connection";
originalCoreMsg.putStringProperty(MessageUtil.CONNECTION_ID_PROPERTY_NAME,
clientId);
-
+
originalCoreMsg.putStringProperty(org.apache.activemq.artemis.api.core.Message.PRODUCER_ID,
producerInfo.getProducerId().toString());
Review Comment:
This looks like it would modify the message and perhaps then transit to a
consumer? I dont think it should really. Would perhaps also save the
expensive-looking toString'ing of the PropducerId being done for every message
(as might using a broker-only conceptual long id for the producer, the
producerInfo for which seems to be coming from a maintained lookup mapping of
AMQProducerBrokerExchange objects originally) .
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerProducerImpl.java:
##########
@@ -18,23 +18,50 @@
import org.apache.activemq.artemis.core.server.ServerProducer;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
+
public class ServerProducerImpl implements ServerProducer {
- private final String ID;
+
+ private static final AtomicLong PRODUCER_ID_GENERATOR = new AtomicLong();
+
+ private final long ID;
+ private String name;
private final String protocol;
private final long creationTime;
+ private volatile long messagesSent = 0;
+ private volatile long messagesSentSize = 0;
+
+ AtomicLongFieldUpdater<ServerProducerImpl> messagesSentUpdater =
AtomicLongFieldUpdater.newUpdater(ServerProducerImpl.class, "messagesSent");
+ AtomicLongFieldUpdater<ServerProducerImpl> messagesSentSizeUpdater =
AtomicLongFieldUpdater.newUpdater(ServerProducerImpl.class, "messagesSentSize");
Review Comment:
The updaters should be static or else it defeats the benefit (memory) of
using them vs AtomicLong instances.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerProducerImpl.java:
##########
@@ -18,23 +18,50 @@
import org.apache.activemq.artemis.core.server.ServerProducer;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
+
public class ServerProducerImpl implements ServerProducer {
- private final String ID;
+
+ private static final AtomicLong PRODUCER_ID_GENERATOR = new AtomicLong();
+
+ private final long ID;
+ private String name;
Review Comment:
final like the rest from constructor?
--
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]
