You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/05/22 16:19:05 UTC

[GitHub] [kafka] splett2 opened a new pull request, #12194: MINOR: Include listener name in SocketServer acceptor and processor log context

splett2 opened a new pull request, #12194:
URL: https://github.com/apache/kafka/pull/12194

   Includes the listener name when logging out messages in the various network threads.
   
   Before:
   ```
   [2022-05-22 09:01:01,331] DEBUG Processor 2 listening to new connection from /127.0.0.1:64381 (kafka.network.Processor:62)
   [2022-05-22 09:01:01,331] DEBUG Processor 1 listening to new connection from /127.0.0.1:64380 (kafka.network.Processor:62)
   [2022-05-22 09:01:01,331] DEBUG Processor 0 listening to new connection from /127.0.0.1:64379 (kafka.network.Processor:62)
   ```
   
   After:
   ```
   [2022-05-22 09:15:47,772] DEBUG [SocketServer listenerType=ZK_BROKER, nodeId=0, listener=PLAINTEXT] Processor 0 listening to new connection from /127.0.0.1:64611 (kafka.network.Processor:62)
   ```
   
   Tested by running a SocketServer unit test and verifying that messages were logged with an appropriate log prefix
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #12194: MINOR: Include listener name in SocketServer acceptor and processor log context

Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12194:
URL: https://github.com/apache/kafka/pull/12194#issuecomment-1158913451

   I would also change the processor log to move the "Processor 0" part into the prefix. The log message would be "Listening to new connection from...".


-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] splett2 commented on a diff in pull request #12194: MINOR: Include listener name in SocketServer acceptor and processor log context

Posted by GitBox <gi...@apache.org>.
splett2 commented on code in PR #12194:
URL: https://github.com/apache/kafka/pull/12194#discussion_r895227215


##########
core/src/main/scala/kafka/network/SocketServer.scala:
##########
@@ -242,13 +241,14 @@ class SocketServer(val config: KafkaConfig,
   }
 
   private def endpoints = config.listeners.map(l => l.listenerName -> l).toMap
+  private def endpointLogContext(endpoint: EndPoint): LogContext = new LogContext(s"[$logPrefix, listener=${endpoint.listenerName.value}] ")
 
   protected def createDataPlaneAcceptor(endPoint: EndPoint, isPrivilegedListener: Boolean, requestChannel: RequestChannel): DataPlaneAcceptor = {
-    new DataPlaneAcceptor(this, endPoint, config, nodeId, connectionQuotas, time, isPrivilegedListener, requestChannel, metrics, credentialProvider, logContext, memoryPool, apiVersionManager)
+    new DataPlaneAcceptor(this, endPoint, config, nodeId, connectionQuotas, time, isPrivilegedListener, requestChannel, metrics, credentialProvider, endpointLogContext(endPoint), memoryPool, apiVersionManager)

Review Comment:
   It's not immediately obvious to me how this would work.
   We still need to pass down some form of log prefix from the SocketServer to the acceptor, assuming we want similar formatting between SocketServer logs and Acceptor/Processor logging.



-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #12194: MINOR: Include listener name in SocketServer acceptor and processor log context

Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12194:
URL: https://github.com/apache/kafka/pull/12194#issuecomment-1158911152

   Thanks for the PR. The log context is meant to encapsulate properties. If we want to include `SocketServer` there, it would ideally follow the key=value convention.


-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] splett2 commented on pull request #12194: MINOR: Include listener name in SocketServer acceptor and processor log context

Posted by GitBox <gi...@apache.org>.
splett2 commented on PR #12194:
URL: https://github.com/apache/kafka/pull/12194#issuecomment-1153279565

   Thank you for the reviews.
   
   @mimaison 
   technically we don't, but the reason I chose to preserve that part of the log context is because it is inherited by `NioSelector` created by the Processor. If we enable DEBUG logging for the `Selector` class, we'd want to be able to distinguish between SocketServer `Selector` log statements and replica fetcher `Selector` log statements. It didn't seem worth the effort to disentangle the Processor log context from the Selector log context, so it seemed reasonable to just keep the SocketServer, listenerType, etc. prefixing.


-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] mimaison commented on pull request #12194: MINOR: Include listener name in SocketServer acceptor and processor log context

Posted by GitBox <gi...@apache.org>.
mimaison commented on PR #12194:
URL: https://github.com/apache/kafka/pull/12194#issuecomment-1141312130

   Thanks for the PR!
   
   Do we need all of `[SocketServer listenerType=ZK_BROKER, nodeId=0, listener=PLAINTEXT]` ? I guess the only thing currently missing is the listener name.


-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming commented on a diff in pull request #12194: MINOR: Include listener name in SocketServer acceptor and processor log context

Posted by GitBox <gi...@apache.org>.
dengziming commented on code in PR #12194:
URL: https://github.com/apache/kafka/pull/12194#discussion_r879997413


##########
core/src/main/scala/kafka/network/SocketServer.scala:
##########
@@ -242,13 +241,14 @@ class SocketServer(val config: KafkaConfig,
   }
 
   private def endpoints = config.listeners.map(l => l.listenerName -> l).toMap
+  private def endpointLogContext(endpoint: EndPoint): LogContext = new LogContext(s"[$logPrefix, listener=${endpoint.listenerName.value}] ")
 
   protected def createDataPlaneAcceptor(endPoint: EndPoint, isPrivilegedListener: Boolean, requestChannel: RequestChannel): DataPlaneAcceptor = {
-    new DataPlaneAcceptor(this, endPoint, config, nodeId, connectionQuotas, time, isPrivilegedListener, requestChannel, metrics, credentialProvider, logContext, memoryPool, apiVersionManager)
+    new DataPlaneAcceptor(this, endPoint, config, nodeId, connectionQuotas, time, isPrivilegedListener, requestChannel, metrics, credentialProvider, endpointLogContext(endPoint), memoryPool, apiVersionManager)

Review Comment:
   We have 2 abstract methods in Acceptor:
   ```
     def metricPrefix(): String
     def threadPrefix(): String
   ```
   how about adding another similar method: `def logPrefix(): String`
   



-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org