Thanks for the patch, reviewed the one that applies on trunk.
I guess we can delete the debug statement in sizeInBytes(). It's wierd that we have it.
2.1 All responses objects besides OffsetRequest, LeaderIsrResponse and StopReplicaResponse have a correlation id. I'm guessing correlation id can be a request level thing that is included on every Kafka request-response object. Another benefit of doing that is the ability to log the correlation id as part of the trace statement in RequestChannel -
trace("Completed request: %s totalTime:%d queueTime:%d localTime:%d remoteTime:%d sendTime:%d"
.format(requestObj, totalTime, queueTime, apiLocalTime, apiRemoteTime, responseSendTime))
This will greatly simplify troubleshooting. Thoughts ?
2.2 The OffsetRequest takes in correlation id but we don't return it as part of the OffsetResponse.
3.1 The correlationId is not passed into the constructor after being read from the byte buffer.
3.2 Probably better to define a DefaultCorrelationId somewhere. It will useful elsewhere in the code.
4. correlation id is a per request level id, does it make sense for it to be of type long instead ?
I think we should get rid of producer.request.correlation_id
6.1 Remove unused import import java.net.InetSocketAddress
7.1 It probably makes sense to allow earlierOrLatestOffset to take in the correlation id that defaults to 0. This is because ZookeeperConsumerConnector should use a correlation id that increments every time it sends a fetch/offset reques
t to the Kafka brokers.
8. ZookeeperConsumerConnector & DefaultEventHandler
It will be nice if these classes set an ever increasing correlation id as part of every request they send to Kafka
9.1 Remove unused import KafkaException
9.2 The documentation of the topic metadata request format is broken.
9.3 Rename TopicMetadata API to ClusterMetadata ?
10.1 In the constructor, it makes sense for the list of replicas to just be the broker ids, not the full Broker object. This will simplify PartitionMetadata.readFrom() and TopicMetadata.readFrom() as well.
11.1 Can we let the following constructor take in the correlation id instead of hardcoding to 0 ?
def this(topics: Seq[String])
The reason is this is used by ClientUtils.fetchTopicMetadata, which in truen, is used elsewhere in ZookeeperConsumerConnector and Producer, where we'd like to track the requests by correlation id.
11.2 The order of the constructor arguments in the scala & java api TopicMetadataRequest is very different. Elsewhere, in the code, versionId is first, followed by correlationId.