You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Andrii Biletskyi <an...@stealth.ly> on 2015/06/30 15:59:39 UTC
Re: Review Request 34766: Patch for KAFKA-2229
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/
-----------------------------------------------------------
(Updated June 30, 2015, 1:59 p.m.)
Review request for kafka.
Bugs: KAFKA-2229
https://issues.apache.org/jira/browse/KAFKA-2229
Repository: kafka
Description (updated)
-------
KIP-4 Phase 1 Rebase
Diffs (updated)
-----
checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/34766/diff/
Testing
-------
Thanks,
Andrii Biletskyi
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Andrii Biletskyi <an...@stealth.ly>.
> On Сер. 10, 2015, 4:56 після полудня, Grant Henke wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java, line 81
> > <https://reviews.apache.org/r/34766/diff/2/?file=995945#file995945line81>
> >
> > Should these new errors be added to kafka.common.ErrorMapping.scala?
As I understand - no. There was a discussion in mailing list about moving from scala+java request objects to java-only classes (which server and client code should use). These suggestions were also applicable for all other objects used in client-server communication, like Errors which is a counterpart for ErrorMapping.
> On Сер. 10, 2015, 4:56 після полудня, Grant Henke wrote:
> > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java, line 92
> > <https://reviews.apache.org/r/34766/diff/2/?file=995945#file995945line92>
> >
> > Looks like copy paste error in exception message.
Thx, fixed in PR.
> On Сер. 10, 2015, 4:56 після полудня, Grant Henke wrote:
> > core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala, line 20
> > <https://reviews.apache.org/r/34766/diff/2/?file=995958#file995958line20>
> >
> > Is there a time where we would want to pass through a throwable? (Same for the other Exceptions added)
Not sure, this was just to follow a convention - other error classes have the same constructor methods.
> On Сер. 10, 2015, 4:56 після полудня, Grant Henke wrote:
> > core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala, line 22
> > <https://reviews.apache.org/r/34766/diff/2/?file=995960#file995960line22>
> >
> > Is this needed? Should we always pass some message? (Same for the other exceptions added?
Same as above.
- Andrii
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94756
-----------------------------------------------------------
On Червень 30, 2015, 1:59 після полудня, Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated Червень 30, 2015, 1:59 після полудня)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Grant Henke <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94756
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java (line 81)
<https://reviews.apache.org/r/34766/#comment149360>
Should these new errors be added to kafka.common.ErrorMapping.scala?
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java (line 92)
<https://reviews.apache.org/r/34766/#comment149357>
Looks like copy paste error in exception message.
core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala (line 20)
<https://reviews.apache.org/r/34766/#comment149358>
Is there a time where we would want to pass through a throwable? (Same for the other Exceptions added)
core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala (line 22)
<https://reviews.apache.org/r/34766/#comment149359>
Is this needed? Should we always pass some message? (Same for the other exceptions added?
- Grant Henke
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Andrii Biletskyi <an...@stealth.ly>.
> On Сер. 14, 2015, 3:48 до полудня, Grant Henke wrote:
> > core/src/main/scala/kafka/server/TopicCommandHelper.scala, lines 146-153
> > <https://reviews.apache.org/r/34766/diff/2/?file=995963#file995963line146>
> >
> > This logic is fairly complex and difficult to track. I am not sure the best way to resolve it. Breaking it into smaller functions may help.
I agree it's hard to follow this logic, it's true the algorithm itself is complicated. Details and suggestions how exactly we can make this part clearer could be helpful here.
- Andrii
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review95376
-----------------------------------------------------------
On Червень 30, 2015, 1:59 після полудня, Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated Червень 30, 2015, 1:59 після полудня)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Grant Henke <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review95376
-----------------------------------------------------------
core/src/main/scala/kafka/server/TopicCommandHelper.scala (line 57)
<https://reviews.apache.org/r/34766/#comment150326>
This could make a lot of calls. Since it is used in createTopic do we want to filter topics that have not been deleted yet?
core/src/main/scala/kafka/server/TopicCommandHelper.scala (line 84)
<https://reviews.apache.org/r/34766/#comment150323>
Do we need to check? why not?
core/src/main/scala/kafka/server/TopicCommandHelper.scala (line 89)
<https://reviews.apache.org/r/34766/#comment150324>
or Partitions and ReplicationFactor must be defined
core/src/main/scala/kafka/server/TopicCommandHelper.scala (lines 146 - 153)
<https://reviews.apache.org/r/34766/#comment150325>
This logic is fairly complex and difficult to track. I am not sure the best way to resolve it. Breaking it into smaller functions may help.
- Grant Henke
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Andrii Biletskyi <an...@stealth.ly>.
> On Aug. 10, 2015, 3:47 p.m., Grant Henke wrote:
> > My apologies for not looking at this sooner, or suggesting this sooner. Given that this code change and scope is fairly large, would it be too much work to break out the patches & reviews by each new protocol message? Then reviews can be more accessable, focus on one thing at a time, and the simple ones can get done much quicker. I am thinking starting with CreateTopic, then DeleteTopic, then AlterTopic may makes sense. If you disagree feel free to say so and I will review this patch as is.
Hey Grant. Not sure this will work well. First of all this patch is already a part of one bigger feature KIP-4 (which we decided to split into patches :)). Secondly and most importantly, logic for handling all 3 requests relies on some base code. This means we will have to extract that common code (exceptions, some utils other stuff) first and probably submit it as a separate patch, which I'd prefer not to do. But I'm very open to suggestions if it speeds up the review process.
- Andrii
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94749
-----------------------------------------------------------
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Andrii Biletskyi <an...@stealth.ly>.
> On Aug. 10, 2015, 3:47 p.m., Grant Henke wrote:
> > My apologies for not looking at this sooner, or suggesting this sooner. Given that this code change and scope is fairly large, would it be too much work to break out the patches & reviews by each new protocol message? Then reviews can be more accessable, focus on one thing at a time, and the simple ones can get done much quicker. I am thinking starting with CreateTopic, then DeleteTopic, then AlterTopic may makes sense. If you disagree feel free to say so and I will review this patch as is.
>
> Andrii Biletskyi wrote:
> Hey Grant. Not sure this will work well. First of all this patch is already a part of one bigger feature KIP-4 (which we decided to split into patches :)). Secondly and most importantly, logic for handling all 3 requests relies on some base code. This means we will have to extract that common code (exceptions, some utils other stuff) first and probably submit it as a separate patch, which I'd prefer not to do. But I'm very open to suggestions if it speeds up the review process.
>
> Grant Henke wrote:
> Thanks Andrii, thats understandable. I will pull down and start reviewing. Since this patch is over a month old, it does not look like it applies cleanly to trunk. Can you please update it so it applies cleanly?
>
> Ismael Juma wrote:
> Andrii, you may also consider submitting a GitHub PR with the rebased branch, see https://cwiki.apache.org/confluence/display/KAFKA/Contributing+Code+Changes for the instructions. You can stick with Review Board if you prefer it though. Both approaches are still supported.
@Grant, ok, I will rebase and submit pull request tomorrow.
@Ismael, thanks for the suggestion, I will try this out.
- Andrii
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94749
-----------------------------------------------------------
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Ismael Juma <mi...@juma.me.uk>.
> On Aug. 10, 2015, 3:47 p.m., Grant Henke wrote:
> > My apologies for not looking at this sooner, or suggesting this sooner. Given that this code change and scope is fairly large, would it be too much work to break out the patches & reviews by each new protocol message? Then reviews can be more accessable, focus on one thing at a time, and the simple ones can get done much quicker. I am thinking starting with CreateTopic, then DeleteTopic, then AlterTopic may makes sense. If you disagree feel free to say so and I will review this patch as is.
>
> Andrii Biletskyi wrote:
> Hey Grant. Not sure this will work well. First of all this patch is already a part of one bigger feature KIP-4 (which we decided to split into patches :)). Secondly and most importantly, logic for handling all 3 requests relies on some base code. This means we will have to extract that common code (exceptions, some utils other stuff) first and probably submit it as a separate patch, which I'd prefer not to do. But I'm very open to suggestions if it speeds up the review process.
>
> Grant Henke wrote:
> Thanks Andrii, thats understandable. I will pull down and start reviewing. Since this patch is over a month old, it does not look like it applies cleanly to trunk. Can you please update it so it applies cleanly?
Andrii, you may also consider submitting a GitHub PR with the rebased branch, see https://cwiki.apache.org/confluence/display/KAFKA/Contributing+Code+Changes for the instructions. You can stick with Review Board if you prefer it though. Both approaches are still supported.
- Ismael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94749
-----------------------------------------------------------
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Grant Henke <gr...@gmail.com>.
> On Aug. 10, 2015, 3:47 p.m., Grant Henke wrote:
> > My apologies for not looking at this sooner, or suggesting this sooner. Given that this code change and scope is fairly large, would it be too much work to break out the patches & reviews by each new protocol message? Then reviews can be more accessable, focus on one thing at a time, and the simple ones can get done much quicker. I am thinking starting with CreateTopic, then DeleteTopic, then AlterTopic may makes sense. If you disagree feel free to say so and I will review this patch as is.
>
> Andrii Biletskyi wrote:
> Hey Grant. Not sure this will work well. First of all this patch is already a part of one bigger feature KIP-4 (which we decided to split into patches :)). Secondly and most importantly, logic for handling all 3 requests relies on some base code. This means we will have to extract that common code (exceptions, some utils other stuff) first and probably submit it as a separate patch, which I'd prefer not to do. But I'm very open to suggestions if it speeds up the review process.
Thanks Andrii, thats understandable. I will pull down and start reviewing. Since this patch is over a month old, it does not look like it applies cleanly to trunk. Can you please update it so it applies cleanly?
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94749
-----------------------------------------------------------
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Grant Henke <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94749
-----------------------------------------------------------
My apologies for not looking at this sooner, or suggesting this sooner. Given that this code change and scope is fairly large, would it be too much work to break out the patches & reviews by each new protocol message? Then reviews can be more accessable, focus on one thing at a time, and the simple ones can get done much quicker. I am thinking starting with CreateTopic, then DeleteTopic, then AlterTopic may makes sense. If you disagree feel free to say so and I will review this patch as is.
- Grant Henke
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Andrii Biletskyi <an...@stealth.ly>.
> On Aug. 10, 2015, 5:01 p.m., Grant Henke wrote:
> > core/src/main/scala/kafka/server/TopicCommandHelper.scala, line 35
> > <https://reviews.apache.org/r/34766/diff/2/?file=995963#file995963line35>
> >
> > A lot of this code/functionality exists in kafka.admin.TopicCommand. Is this duplicating a lot of code/functionality? Could they both be refactored to use the same core code?
IMO no, they cannot be refactored. The main problem is that code in kafka.admin.TopicCommand is very tightly coupled with CLI logic (like printing results, validation inputs), also error handling logic is very different.
- Andrii
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94759
-----------------------------------------------------------
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>
Re: Review Request 34766: Patch for KAFKA-2229
Posted by Grant Henke <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34766/#review94759
-----------------------------------------------------------
core/src/main/scala/kafka/server/TopicCommandHelper.scala (line 35)
<https://reviews.apache.org/r/34766/#comment149361>
A lot of this code/functionality exists in kafka.admin.TopicCommand. Is this duplicating a lot of code/functionality? Could they both be refactored to use the same core code?
- Grant Henke
On June 30, 2015, 1:59 p.m., Andrii Biletskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34766/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:59 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2229
> https://issues.apache.org/jira/browse/KAFKA-2229
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KIP-4 Phase 1 Rebase
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03
> clients/src/main/java/org/apache/kafka/common/ConfigEntry.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/PartitionReplicaAssignment.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/TopicConfigInfo.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b39e9bb53dd51f1ce931691e23aabc9ebaebf1e7
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java 5b898c8f8ad5d0469f469b600c4b2eb13d1fc662
> clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f
> clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 5d3d52859587ce0981d702f04042b0f6e1bc3704
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/AlterTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/CreateTopicResponse.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicRequest.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/requests/admin/DeleteTopicResponse.java PRE-CREATION
> clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 8b2aca85fa738180e5420985fddc39a4bf9681ea
> core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b
> core/src/main/scala/kafka/api/RequestKeys.scala 155cb650e9cffe2c950326cfc25b1480cda819db
> core/src/main/scala/kafka/common/InvalidPartitionsException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicaAssignmentException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidReplicationFactorException.scala PRE-CREATION
> core/src/main/scala/kafka/common/InvalidTopicConfigurationException.scala PRE-CREATION
> core/src/main/scala/kafka/common/ReassignPartitionsInProgressException.scala PRE-CREATION
> core/src/main/scala/kafka/server/KafkaApis.scala ad6f05807c61c971e5e60d24bc0c87e989115961
> core/src/main/scala/kafka/server/TopicCommandHelper.scala PRE-CREATION
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8
> core/src/test/scala/unit/kafka/server/TopicCommandHelperTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrii Biletskyi
>
>