KAFKA-9435: DescribeLogDirs automated protocol#7972
Conversation
|
@cmccabe are you able to review this? |
mimaison
left a comment
mimaison
left a comment
There was a problem hiding this comment.
Thanks for the PR. I've made a very quick first pass and left a few comments
tombentley
force-pushed
the
KAFKA-9435-DescribeLogDirs-automated-protocol
branch
from
f3b8edb to
de57806
Compare
|
@mimaison done, if you want to do another pass. |
mimaison
left a comment
mimaison
left a comment
There was a problem hiding this comment.
I've made a quick pass and left a few comments.
Can you also rebase on trunk?
There was a problem hiding this comment.
We can remove a bunch of brackets in this statement
There was a problem hiding this comment.
I think it's slightly more readable if each setter is called on a new line. There're a few places where setLogDir() is called inline in this file.
There was a problem hiding this comment.
Is this comment still acurate?
tombentley
force-pushed
the
KAFKA-9435-DescribeLogDirs-automated-protocol
branch
from
de57806 to
39fb997
Compare
|
@mimaison fixed review comments and rebased. |
mimaison
left a comment
mimaison
left a comment
There was a problem hiding this comment.
I've made another pass and spotted a couple of tiny issues. Happy to merge once fixed
| this.data = new DescribeLogDirsRequestData(struct, version); | ||
| } | ||
|
|
||
| // topicPartitions == null indicates requesting all partitions, and an empty list indicates requesting no partitions. |
There was a problem hiding this comment.
This comment can now be removed
| import org.apache.kafka.common.replica.ReplicaView.DefaultReplicaView | ||
| import org.apache.kafka.common.replica.{ClientMetadata, _} | ||
| import org.apache.kafka.common.requests.DescribeLogDirsResponse.{LogDirInfo, ReplicaInfo} | ||
| import org.apache.kafka.common.message.DescribeLogDirsResponseData._ |
There was a problem hiding this comment.
nit: Can we put this with the other import from the same package
|
@mimaison done |
mimaison
left a comment
mimaison
left a comment
There was a problem hiding this comment.
I've left a couple of new comments for issues we should fix
| @@ -49,105 +43,54 @@ public class DescribeLogDirsRequest extends AbstractRequest { | |||
| TOPIC_NAME, | |||
There was a problem hiding this comment.
This V0 schema should be deleted too
|
|
||
| // topicPartitions == null indicates requesting all partitions, and an empty list indicates requesting no partitions. | ||
| public DescribeLogDirsRequest(Set<TopicPartition> topicPartitions, short version) { | ||
| public DescribeLogDirsRequest(DescribeLogDirsRequestData data, short version) { |
There was a problem hiding this comment.
The comment above is out of date
|
Sloppy! Thanks, now fixed. |
2 participants
Also add version 2 to make use of flexible versions, per KIP-482.
Committer Checklist (excluded from commit message)