KAFKA-5919: Adding checks on "version" field for tools using it#5126
KAFKA-5919: Adding checks on "version" field for tools using it#5126ppatierno wants to merge 3 commits into
Conversation
|
@lindong28 this is the simple version of the #3887 which I closed due to a mess with rebase (and then merge). Sorry for that but now it's simpler to review and it's aligned with current trunk. |
lindong28
left a comment
There was a problem hiding this comment.
Thanks for the patch. Looks good overall. Left some minor comments.
| val offset = partitionJs("offset").to[Long] | ||
| new TopicPartition(topic, partition) -> offset | ||
| }.toBuffer | ||
| Json.parseFull(jsonData) match { |
There was a problem hiding this comment.
nits: can you remove the { here?
There was a problem hiding this comment.
I searched for other places where match is used at it seems that in those places, the { is always used. Or am I wrong?
There was a problem hiding this comment.
Ah, I comment in the wrong line. I actually mean the line below.
More specifically,
case Some(js) => {
val version = js.asJsonObject.get("version") match {
case Some(jsonValue) => jsonValue.to[Int]
case None => EarliestVersion
}
parseJsonData(version, js)
}
can be:
case Some(js) =>
val version = js.asJsonObject.get("version") match {
case Some(jsonValue) => jsonValue.to[Int]
case None => EarliestVersion
}
parseJsonData(version, js)
There was a problem hiding this comment.
For example, we don't use { after all usage of case in LogManager.scala.
There was a problem hiding this comment.
Yeah you are right. Done!
| new TopicPartition(topic, partition) -> offset | ||
| }.toBuffer | ||
| Json.parseFull(jsonData) match { | ||
| case Some(js) => { |
There was a problem hiding this comment.
nits: can you remove the { here so that we following the existing code style? There are a few other places where we can reduce { and } similarly. Also, could we also follow the existing code style and remove some empty lines?
| ).asJava) | ||
| } | ||
|
|
||
| def parseTopicsJsonString(jsonData: String): Seq[String] = { |
There was a problem hiding this comment.
The patch is trying to differentiate between parseTopicsJsonString and parseTopicsData by using different names. Since the parameter types are already different and explanatory, would it be simpler to keep the existing name, e.g. parseTopicsData?
|
|
||
| def generateAssignment(zkClient: KafkaZkClient, brokerListToReassign: Seq[Int], topicsToMoveJsonString: String, disableRackAware: Boolean): (Map[TopicPartition, Seq[Int]], Map[TopicPartition, Seq[Int]]) = { | ||
| val topicsToReassign = ZkUtils.parseTopicsData(topicsToMoveJsonString) | ||
| val topicsToReassign = parseTopicsJsonString(topicsToMoveJsonString) |
There was a problem hiding this comment.
Should we remove the method in ZkUtils since that is no longer used?
Renamed parsing methods Removed unused parsing json method from ZkUtils
|
I fixed your request changes, just a comment/doubts about removing |
|
Thanks for the patch @ppatierno. LGTM. Merged to the trunk. |
Adding checks on "version" field for tools using it. This is a new version of the closed PR #3887 (to see for more comments and related discussion). Author: Paolo Patierno <ppatierno@live.com> Reviewers: Dong Lin <lindong28@gmail.com> Closes #5126 from ppatierno/kafka-5919-update
Adding checks on "version" field for tools using it. This is a new version of the closed PR apache#3887 (to see for more comments and related discussion). Author: Paolo Patierno <ppatierno@live.com> Reviewers: Dong Lin <lindong28@gmail.com> Closes apache#5126 from ppatierno/kafka-5919-update
Adding checks on "version" field for tools using it.
This is a new version of the closed PR #3887 (to see for more comments and related discussion).
Committer Checklist (excluded from commit message)