[ 
https://issues.apache.org/jira/browse/KAFKA-683?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13544506#comment-13544506
 ] 

Jun Rao commented on KAFKA-683:
-------------------------------

Thanks for the patch. Some comments:

1. DefaultEventHandler.send(): Instead of
      val currentCorrelationId = correlationId.get()-1
it's probably better to
      val currentCorrelationId = correlationId.getAndIncrement()
at the beginning and then reuse it when needed.

2. FileMessageSet: If initChannelPositionToEnd is true, we could be either 
creating a new segment or loading an existing segment during startup. So, we 
should rephrase the info message a bit.

3. KafkaMigrationTool: Currently, the tool requires exactly one of whitelist or 
blacklist. So, we will not be able to use the default value of whitelist. We 
can probably leave whitelist as a required argument, but put in the description 
of how to specify all topics correctly (ie, .*).

4. Log:
4.1 maybeRoll(): Rechecking the condition has the problem that time-based 
condition may not return the same value. We can probably check each condition 
once and if the condition is true, log the cause and set a boolean var 
shouldRoll to true.
4.2 markDeletedWhile(): For the new logging, should we somehow indicate that 
those logs are from this function? Also, it seems that we log whether all 
current index/data files exist. Should we log the name of the index/data files 
too so that we know which ones are missing?

5. javaapi.TopicMetadataRequest: The scala optional parameter for correlationId 
won't work for java. We will have to manually create two constructors, one with 
correlation id and the other without.

6. RequestChannel: We are already logging the whole request which includes 
clientid, correlationid and versionid. So, there is no need to log them 
explicitly.

7. config/log4j.properties: All scripts in bin/ currently uses this file. The 
changes are really intended for Kafka broker. Perhaps we can create a new log4j 
file just for the broker and change the kafka broker scripts accordingly. Also, 
for kafka broker, should we log to both file and console? Finally, I got the 
following warning when running the kafka server startup script.
log4j:WARN No such property [maxBackupIndex] in org.apache.log4j.FileAppender.
log4j:WARN No such property [maxFileSize] in org.apache.log4j.FileAppender.
log4j:WARN No such property [maxBackupIndex] in org.apache.log4j.FileAppender.
log4j:WARN No such property [maxFileSize] in org.apache.log4j.FileAppender.

8. Was the state change log added? I didn't see the change in the scala code or 
the log4j property file.

                
> Fix correlation ids in all requests sent to kafka
> -------------------------------------------------
>
>                 Key: KAFKA-683
>                 URL: https://issues.apache.org/jira/browse/KAFKA-683
>             Project: Kafka
>          Issue Type: Improvement
>    Affects Versions: 0.8
>            Reporter: Neha Narkhede
>            Assignee: Neha Narkhede
>            Priority: Critical
>              Labels: improvement, replication
>         Attachments: kafka-683-v1.patch
>
>
> We should fix the correlation ids in every request sent to Kafka and fix the 
> request log on the broker to specify not only the type of request and who 
> sent it, but also the correlation id. This will be very helpful while 
> troubleshooting problems in production.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to