Kafka
  1. Kafka
  2. KAFKA-696

Fix toString() API for all requests to make logging easier to read

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      It will be useful to have consistent logging styles for all requests. Right now, we depend on the default toString implementation and the problem is that it is very hard to read and prints out unnecessary information like the ByteBuffer.

      1. cleanup-v3.patch
        30 kB
        Sriram Subramanian
      2. cleanup-v2.patch
        25 kB
        Sriram Subramanian
      3. cleanup-v1.patch
        15 kB
        Sriram Subramanian

        Activity

        Neha Narkhede created issue -
        Sriram Subramanian made changes -
        Field Original Value New Value
        Assignee Neha Narkhede [ nehanarkhede ] Sriram Subramanian [ sriramsub ]
        Hide
        Sriram Subramanian added a comment -
        • Added ToString overrides to all the request object to make logging more readable and ignore things that we dont care
        • Errors are handled by each request and removed the case statements in kafkaapi.
        Show
        Sriram Subramanian added a comment - Added ToString overrides to all the request object to make logging more readable and ignore things that we dont care Errors are handled by each request and removed the case statements in kafkaapi.
        Sriram Subramanian made changes -
        Attachment cleanup-v1.patch [ 12565044 ]
        Hide
        Jun Rao added a comment -

        Thanks for the patch. Some comments:

        1. RequestOrResponse.handleError: We probably shouldn't couple the error response generation and the sending of the response through RequestChannel. It's probably better if we change handleError() to sth like generateErrorResponse() that only takes an error code and generates a response. The sending of the response will be done in KafkaApis. Similarly, I think the error logging should be done in KafkaApis.handle() too.

        2. ProducerRequest: When printing out data, in addition to the key, it would be useful to print out messageSet.sizeInBytes().

        3. KafkaApis:
        3.1 Doesn't compile.
        3.2 There are statements like the following in the handling of each type of request. Could we move them to handle() and remove the special printing for versionId, correlationId, and clientId?
        if(requestLogger.isTraceEnabled)
        requestLogger.trace("Handling LeaderAndIsrRequest v%d with correlation id %d from client %s: %s"
        .format(leaderAndIsrRequest.versionId, leaderAndIsrRequest.correlationId, leaderAndIsrRequest.clientId, leaderAndIsrRequest.toString))

        4. RequestChannel.Request: Could you remove versionId, correlationId, clientId in the trace statement in updateRequestMetrics() and remove the corresponding deserialization logic?

        Show
        Jun Rao added a comment - Thanks for the patch. Some comments: 1. RequestOrResponse.handleError: We probably shouldn't couple the error response generation and the sending of the response through RequestChannel. It's probably better if we change handleError() to sth like generateErrorResponse() that only takes an error code and generates a response. The sending of the response will be done in KafkaApis. Similarly, I think the error logging should be done in KafkaApis.handle() too. 2. ProducerRequest: When printing out data, in addition to the key, it would be useful to print out messageSet.sizeInBytes(). 3. KafkaApis: 3.1 Doesn't compile. 3.2 There are statements like the following in the handling of each type of request. Could we move them to handle() and remove the special printing for versionId, correlationId, and clientId? if(requestLogger.isTraceEnabled) requestLogger.trace("Handling LeaderAndIsrRequest v%d with correlation id %d from client %s: %s" .format(leaderAndIsrRequest.versionId, leaderAndIsrRequest.correlationId, leaderAndIsrRequest.clientId, leaderAndIsrRequest.toString)) 4. RequestChannel.Request: Could you remove versionId, correlationId, clientId in the trace statement in updateRequestMetrics() and remove the corresponding deserialization logic?
        Hide
        Sriram Subramanian added a comment -

        1. The reason why I did not do that was because ReplicaRequest does not extend from RequestReponse. I kept what was there as is but moved the logging of the request outside to kafkaapi.
        2. ProducerRequest logs the size for each message set.
        3.1 It was an old patch and looks like there were conflicts.
        3.2 Each request also prints the request name now. Hence moved these lines to the parent method.
        4. Removed the deserializations and just log the request object.

        Show
        Sriram Subramanian added a comment - 1. The reason why I did not do that was because ReplicaRequest does not extend from RequestReponse. I kept what was there as is but moved the logging of the request outside to kafkaapi. 2. ProducerRequest logs the size for each message set. 3.1 It was an old patch and looks like there were conflicts. 3.2 Each request also prints the request name now. Hence moved these lines to the parent method. 4. Removed the deserializations and just log the request object.
        Sriram Subramanian made changes -
        Attachment cleanup-v2.patch [ 12565881 ]
        Neha Narkhede made changes -
        Priority Major [ 3 ] Critical [ 2 ]
        Hide
        Neha Narkhede added a comment -

        Sorry for a late review. Following are a few review suggestions on patch v2 -
        1. FetchRequest
        1.1 Remove unused imports "Response" and "BoundedByteBufferSend"
        1.2 In the toString() API, for MaxWait, lets append "ms" after the value. Also for MinBytes, lets append "bytes" after the value

        2. KafkaApis
        2.1 In handle() APIs, let's move the .format(request.requestObj)) to the previous line

        3. LeaderAndIsrRequest
        Let's include "ms" after the value for AckTimeoutMs

        4. ProducerRequest
        Let's include "ms" after the value for AckTimeoutMs

        5. StopReplicaRequest
        5.1 Let's include "ms" after the value for AckTimeoutMs
        5.2 Looks like we haven't standardized StopReplicaRequest to use TopicAndPartition instead of a custom Tuple for representing partitions. This messes up the log4j logging style and the toString() API will not print partitions the way rest of the code does. You can either include it in your patch or leave it for KAFKA-649

        Show
        Neha Narkhede added a comment - Sorry for a late review. Following are a few review suggestions on patch v2 - 1. FetchRequest 1.1 Remove unused imports "Response" and "BoundedByteBufferSend" 1.2 In the toString() API, for MaxWait, lets append "ms" after the value. Also for MinBytes, lets append "bytes" after the value 2. KafkaApis 2.1 In handle() APIs, let's move the .format(request.requestObj)) to the previous line 3. LeaderAndIsrRequest Let's include "ms" after the value for AckTimeoutMs 4. ProducerRequest Let's include "ms" after the value for AckTimeoutMs 5. StopReplicaRequest 5.1 Let's include "ms" after the value for AckTimeoutMs 5.2 Looks like we haven't standardized StopReplicaRequest to use TopicAndPartition instead of a custom Tuple for representing partitions. This messes up the log4j logging style and the toString() API will not print partitions the way rest of the code does. You can either include it in your patch or leave it for KAFKA-649
        Hide
        Sriram Subramanian added a comment -
        • Added units to the logging
        • Did not change StopReplicaRequest to using TopicAndPartition. Leaving it to 649.
        Show
        Sriram Subramanian added a comment - Added units to the logging Did not change StopReplicaRequest to using TopicAndPartition. Leaving it to 649.
        Sriram Subramanian made changes -
        Attachment cleanup-v3.patch [ 12566355 ]
        Hide
        Neha Narkhede added a comment -

        Committed v3

        Show
        Neha Narkhede added a comment - Committed v3
        Neha Narkhede made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Neha Narkhede made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Sriram Subramanian
            Reporter:
            Neha Narkhede
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development