gemmellr commented on code in PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#discussion_r1252202278
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,12 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf
in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+
ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(),
cause);
Review Comment:
Including the local address may make sense...broker has multiple acceptors,
which can also potentially be multi-homed on top of that.
Could potentially drop the toString and leave it to the logger.
##########
tests/smoke-tests/src/main/resources/servers/audit-logging2/log4j2.properties:
##########
@@ -40,7 +40,7 @@ logger.audit_base = INFO, audit_log_file
logger.audit_base.name = org.apache.activemq.audit.base
logger.audit_base.additivity = false
-logger.audit_resource = OFF, audit_log_file
+logger.audit_resource = INFO, audit_log_file
Review Comment:
Related to other comment, the resource logger is enabled in the
AuditLoggerResourceTest test config already.
##########
tests/smoke-tests/pom.xml:
##########
@@ -188,6 +188,13 @@
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.activemq.tests</groupId>
+ <artifactId>integration-tests</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ </dependency>
Review Comment:
I've slowly been trying to break all of the ugly chained test-jar
dependencies, and related classpath pollutions coming with them, so rather than
adding/restoring this one...
It looks like you added this to get to a STOMP test-client? The AMQP
test-client lives in the _artemis-test-support_ module for sharing supporting
systest helper bits (specifically,
tests/artemis-test-support/src/main/java/org/apache/activemq/transport/amqp/client/),
as does the CFUtil the new tests also use. Seems like moving the STOMP
test-client alongside them would be nicer than this, assuming its simple.
EDIT: looks like the stomp client innards already use the transport bits
from artemis-test-support, making it seem like an even more appropriate move.
I'll put together a PR after lunch.
##########
tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/logging/AuditLoggerTest.java:
##########
@@ -117,6 +121,56 @@ public void testAuditLog() throws Exception {
checkAuditLogRecord(true, "is sending a message");
}
+ @Test
+ public void testCoreConnectionAuditLog() throws Exception {
+ testConnectionAuditLog("CORE");
+ }
+
+ @Test
+ public void testAMQPConnectionAuditLog() throws Exception {
+ testConnectionAuditLog("AMQP");
+ }
+
+ @Test
+ public void testOpenWireConnectionAuditLog() throws Exception {
+ testConnectionAuditLog("OPENWIRE");
+ }
Review Comment:
Since these are considered 'resource' logs, they might be better in
AuditLoggerResourceTest
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,12 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf
in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+
ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(),
cause);
+ ctx.close();
Review Comment:
Since this is the important bit perhaps whacking it in a try-finally would
make sense.
--
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]
