You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Mayuresh Gharat <gh...@gmail.com> on 2014/12/07 20:43:11 UTC
Review Request 28793: Patch for KAFKA-1784
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1784
https://issues.apache.org/jira/browse/KAFKA-1784
Repository: kafka
Description
-------
OffsetClient config for the OffsetClient
OffsetClient for fetching and importing offsets from kafka
Diffs
-----
core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
Diff: https://reviews.apache.org/r/28793/diff/
Testing
-------
Thanks,
Mayuresh Gharat
Re: Review Request 28793: Patch for KAFKA-1784
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On Dec. 10, 2014, 12:37 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/OffsetClient.scala, line 142
> > <https://reviews.apache.org/r/28793/diff/1/?file=785102#file785102line142>
> >
> > This and a lot of the rest of the code exists in ClientUtils. Until the refactoring is complete, your admin tool can just utilize the existing APIst to expose commit/fetch through the tool. This class may be unnecessary.
>
> Mayuresh Gharat wrote:
> I can pull out the code and use client utils as of now and once we get the other patch with refactored code we can use this. Does that sound ok? I will upload a new patch accordingly.
>
> Mayuresh Gharat wrote:
> So do we remove this class completely and use it only when patch for Kafka-1013 has been reviewed and ready to be checked in?
>
> Neha Narkhede wrote:
> Yes. I'm wondering if you can write the admin tool using the current APIs as is. It will be most convenient to refactor this correctly in one go (as part of KAFKA-1013).
Cool. I have made the necessary changes as per Joel's suggestion for the patch on Kafka-1013. Once that gets reviewed by Joel, we should be good to go. Currently I am facing some issues with apache reviewboard returning me a 500 internal server error when trying to publish the review.
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/#review64477
-----------------------------------------------------------
On Dec. 7, 2014, 7:43 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28793/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2014, 7:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1784
> https://issues.apache.org/jira/browse/KAFKA-1784
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Offset Client for fetching and commiting offsets to kafka
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
> core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28793/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 28793: Patch for KAFKA-1784
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On Dec. 10, 2014, 12:37 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/OffsetClient.scala, line 142
> > <https://reviews.apache.org/r/28793/diff/1/?file=785102#file785102line142>
> >
> > This and a lot of the rest of the code exists in ClientUtils. Until the refactoring is complete, your admin tool can just utilize the existing APIst to expose commit/fetch through the tool. This class may be unnecessary.
I can pull out the code and use client utils as of now and once we get the other patch with refactored code we can use this. Does that sound ok? I will upload a new patch accordingly.
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/#review64477
-----------------------------------------------------------
On Dec. 7, 2014, 7:43 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28793/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2014, 7:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1784
> https://issues.apache.org/jira/browse/KAFKA-1784
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Offset Client for fetching and commiting offsets to kafka
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
> core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28793/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 28793: Patch for KAFKA-1784
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On Dec. 10, 2014, 12:37 a.m., Neha Narkhede wrote:
> > Can you add unit tests for this?
Sure. I will try and add unit tests for this.
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/#review64477
-----------------------------------------------------------
On Dec. 7, 2014, 7:43 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28793/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2014, 7:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1784
> https://issues.apache.org/jira/browse/KAFKA-1784
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Offset Client for fetching and commiting offsets to kafka
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
> core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28793/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 28793: Patch for KAFKA-1784
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On Dec. 10, 2014, 12:37 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/OffsetClient.scala, line 142
> > <https://reviews.apache.org/r/28793/diff/1/?file=785102#file785102line142>
> >
> > This and a lot of the rest of the code exists in ClientUtils. Until the refactoring is complete, your admin tool can just utilize the existing APIst to expose commit/fetch through the tool. This class may be unnecessary.
>
> Mayuresh Gharat wrote:
> I can pull out the code and use client utils as of now and once we get the other patch with refactored code we can use this. Does that sound ok? I will upload a new patch accordingly.
So do we remove this class completely and use it only when patch for Kafka-1013 has been reviewed and ready to be checked in?
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/#review64477
-----------------------------------------------------------
On Dec. 7, 2014, 7:43 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28793/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2014, 7:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1784
> https://issues.apache.org/jira/browse/KAFKA-1784
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Offset Client for fetching and commiting offsets to kafka
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
> core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28793/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 28793: Patch for KAFKA-1784
Posted by Neha Narkhede <ne...@gmail.com>.
> On Dec. 10, 2014, 12:37 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/OffsetClient.scala, line 142
> > <https://reviews.apache.org/r/28793/diff/1/?file=785102#file785102line142>
> >
> > This and a lot of the rest of the code exists in ClientUtils. Until the refactoring is complete, your admin tool can just utilize the existing APIst to expose commit/fetch through the tool. This class may be unnecessary.
>
> Mayuresh Gharat wrote:
> I can pull out the code and use client utils as of now and once we get the other patch with refactored code we can use this. Does that sound ok? I will upload a new patch accordingly.
>
> Mayuresh Gharat wrote:
> So do we remove this class completely and use it only when patch for Kafka-1013 has been reviewed and ready to be checked in?
Yes. I'm wondering if you can write the admin tool using the current APIs as is. It will be most convenient to refactor this correctly in one go (as part of KAFKA-1013).
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/#review64477
-----------------------------------------------------------
On Dec. 7, 2014, 7:43 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28793/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2014, 7:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1784
> https://issues.apache.org/jira/browse/KAFKA-1784
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Offset Client for fetching and commiting offsets to kafka
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
> core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28793/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 28793: Patch for KAFKA-1784
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On Dec. 10, 2014, 12:37 a.m., Neha Narkhede wrote:
> > Can you add unit tests for this?
>
> Mayuresh Gharat wrote:
> Sure. I will try and add unit tests for this.
This part of the code was tested by Clark here, when had encountered an issue where we had to fetch offsets from kafka.
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/#review64477
-----------------------------------------------------------
On Dec. 7, 2014, 7:43 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28793/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2014, 7:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1784
> https://issues.apache.org/jira/browse/KAFKA-1784
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Offset Client for fetching and commiting offsets to kafka
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
> core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28793/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 28793: Patch for KAFKA-1784
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/#review64477
-----------------------------------------------------------
Can you add unit tests for this?
core/src/main/scala/kafka/tools/OffsetClient.scala
<https://reviews.apache.org/r/28793/#comment107194>
This and a lot of the rest of the code exists in ClientUtils. Until the refactoring is complete, your admin tool can just utilize the existing APIst to expose commit/fetch through the tool. This class may be unnecessary.
- Neha Narkhede
On Dec. 7, 2014, 7:43 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28793/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2014, 7:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1784
> https://issues.apache.org/jira/browse/KAFKA-1784
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Offset Client for fetching and commiting offsets to kafka
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
> core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28793/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 28793: Patch for KAFKA-1784
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28793/
-----------------------------------------------------------
(Updated Dec. 7, 2014, 7:43 p.m.)
Review request for kafka.
Bugs: KAFKA-1784
https://issues.apache.org/jira/browse/KAFKA-1784
Repository: kafka
Description (updated)
-------
Offset Client for fetching and commiting offsets to kafka
Diffs
-----
core/src/main/scala/kafka/tools/OffsetClient.scala PRE-CREATION
core/src/main/scala/kafka/tools/OffsetClientConfig.scala PRE-CREATION
Diff: https://reviews.apache.org/r/28793/diff/
Testing
-------
Thanks,
Mayuresh Gharat