gemmellr commented on code in PR #5303:
URL: https://github.com/apache/activemq-artemis/pull/5303#discussion_r1804621128
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServerLogger.java:
##########
@@ -1584,8 +1439,6 @@ void
noQueueIdDefined(org.apache.activemq.artemis.api.core.Message message,
@LogMessage(id = 224126, value = "Failure during protocol handshake on
connection to {} from {}", level = LogMessage.Level.ERROR)
void failureDuringProtocolHandshake(SocketAddress localAddress,
SocketAddress remoteAddress, Throwable e);
- // notice loggerID=224127 is reserved as it's been used at
ActiveMQQueueLogger
Review Comment:
This seems like a notice worth keeping?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServerLogger.java:
##########
@@ -35,30 +32,26 @@
import org.apache.activemq.artemis.core.config.Configuration;
import org.apache.activemq.artemis.core.io.IOCallback;
import org.apache.activemq.artemis.core.io.SequentialFile;
-import org.apache.activemq.artemis.core.paging.cursor.PagePosition;
-import org.apache.activemq.artemis.core.paging.cursor.PageSubscription;
-import org.apache.activemq.artemis.core.persistence.OperationContext;
import org.apache.activemq.artemis.core.protocol.core.Packet;
import
org.apache.activemq.artemis.core.protocol.core.impl.wireformat.BackupReplicationStartFailedMessage;
import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
-import org.apache.activemq.artemis.core.server.routing.targets.Target;
import org.apache.activemq.artemis.core.server.cluster.Bridge;
import org.apache.activemq.artemis.core.server.cluster.impl.BridgeImpl;
import
org.apache.activemq.artemis.core.server.cluster.impl.ClusterConnectionImpl;
import
org.apache.activemq.artemis.core.server.cluster.quorum.ServerConnectVote;
import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl;
import org.apache.activemq.artemis.core.server.impl.ServerSessionImpl;
import org.apache.activemq.artemis.core.server.management.Notification;
+import org.apache.activemq.artemis.core.server.routing.targets.Target;
+import org.apache.activemq.artemis.logs.BundleFactory;
import org.apache.activemq.artemis.logs.annotation.LogBundle;
import org.apache.activemq.artemis.logs.annotation.LogMessage;
-import org.apache.activemq.artemis.logs.BundleFactory;
import org.apache.activemq.artemis.spi.core.remoting.Connection;
-import org.w3c.dom.Node;
/**
* Logger Code 22
*/
-@LogBundle(projectCode = "AMQ", regexID = "22[0-9]{4}")
+@LogBundle(projectCode = "AMQ", regexID = "22[0-9]{4}", retiredIDs = "221026,
221052, 222012, 222020, 222021, 222022, 222024, 222027, 222028, 222029, 222052,
222071, 222078, 222079, 222083, 222084, 222088, 222090, 222102, 222128, 222134,
222135, 222152, 222159, 222163, 222167, 222170, 222171, 222182, 222192, 222193,
222204, 222252, 222255, 222257, 224001, 224002, 224003, 224005, 224035, 224100,
224121, 224127, 222260, 222259, 222288")
Review Comment:
This is getting a bit ugly. I wonder about maybe a static method could be
named, or annotated separately, to provide the values to separate the detail
from the basic bundle definition? Could perhaps avoid using a String that way
also (though maybe even the way it is).
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/integration/bootstrap/ActiveMQBootstrapLogger.java:
##########
@@ -23,23 +23,14 @@
/**
* Logger Code 10
*/
-@LogBundle(projectCode = "AMQ", regexID = "10[0-9]{4}")
+@LogBundle(projectCode = "AMQ", regexID = "10[0-9]{4}", retiredIDs = "101001,
101002, 101003")
Review Comment:
This list (and other classes similarly) is likely to be incomplete. I recall
Clebert removed some when we worked on switching things to SLF4J+Log4j. So I
expect there will be several others in amongst
9873fccf744c0cb0a25dd905fab67ea52ef7aa7d (or
[9873fccf744c0cb0a25dd905fab67ea52ef7aa7d.diff](https://github.com/apache/activemq-artemis/commit/9873fccf744c0cb0a25dd905fab67ea52ef7aa7d.diff)
since it is too large for the UI ) that could be marked retired, either now or
separately.
##########
artemis-log-annotation-processor/src/main/java/org/apache/activemq/artemis/logs/annotation/LogBundle.java:
##########
@@ -27,7 +27,13 @@
String projectCode();
- /** if set, every code generated must match this regular expression.
- * this is useful to validate ranges.*/
+ /* If set, every code generated must match this regular expression.
+ * This is useful to validate ranges.
+ */
Review Comment:
This seems like it should stay Javadoc? Similar for new one below.
##########
artemis-log-annotation-processor/src/main/java/org/apache/activemq/artemis/logs/annotation/processor/LogAnnotationProcessor.java:
##########
@@ -204,6 +208,17 @@ void validateRegexID(LogBundle bundleAnnotation, long id) {
}
}
+ void checkRetiredIDs(LogBundle bundleAnnotation, long id) {
+ if (bundleAnnotation.retiredIDs() != null &&
!bundleAnnotation.retiredIDs().isEmpty()) {
+ if (Arrays.stream(bundleAnnotation.retiredIDs().split(","))
+ .map(String::trim)
+ .map(Long::valueOf)
+ .collect(Collectors.toList()).contains(id)) {
+ throw new IllegalArgumentException("Code ID " + id + " has been
retired. Select a different ID.");
+ }
+ }
+ }
Review Comment:
Although not performance-critical, repeatedly splitting the retiredIDs
String, trimming them, creating the Long's, and Collecting them to the same
ultimate List over-and-over again for every LogMessage/Message annotation seems
especially inefficient. It could be done once upfront for each LogBundle and
the result reused?
There are a lot of sequential retired IDs. Someone hitting one seems likely
to just try the next one, and find the same error, before then having to look
at the bundle definition and consider the list and find the next non-retired
and not-in-use one.
This bit of code has the retired IDs, meaning it can easily indicate what
they are so anyone hitting the failure can immediately see the retired ones
they cant use. (It could even indicate the next ID that isnt retired and could
also further look at whether that is already used and suggest the next that
isnt...but that would be a big change, so just printing the retired ones would
be an easy help instead hehe).
--
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-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
