You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Tong Li <li...@us.ibm.com> on 2015/02/16 17:37:24 UTC
Review Request 31088: Patch for KAFKA-1959
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31088/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1959
https://issues.apache.org/jira/browse/KAFKA-1959
Repository: kafka
Description
-------
The original code clearly wants to define a string but override the ThreadGroup of
the super class member. The patchset will rename the private variable to be group_id
as it intended.
Diffs
-----
core/src/test/scala/other/kafka/TestOffsetManager.scala 41f334d48897b3027ed54c58bbf4811487d3b191
Diff: https://reviews.apache.org/r/31088/diff/
Testing
-------
Thanks,
Tong Li
Re: Review Request 31088: Patch for KAFKA-1959
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31088/#review72910
-----------------------------------------------------------
Ship it!
Ship It!
- Joel Koshy
On Feb. 16, 2015, 4:37 p.m., Tong Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31088/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2015, 4:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1959
> https://issues.apache.org/jira/browse/KAFKA-1959
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The original code clearly wants to define a string but override the ThreadGroup of
> the super class member. The patchset will rename the private variable to be group_id
> as it intended.
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestOffsetManager.scala 41f334d48897b3027ed54c58bbf4811487d3b191
>
> Diff: https://reviews.apache.org/r/31088/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tong Li
>
>
Re: Review Request 31088: Patch for KAFKA-1959
Posted by Gwen Shapira <gs...@cloudera.com>.
> On Feb. 16, 2015, 6:22 p.m., Gwen Shapira wrote:
> > The rename does make things a bit clearer, so I don't object to committing it.
> > But the test does not override "group" in superclas Thread. Thread.group is a private variable, so it cannot be overriden (and the classes that inherit from it can't use it) so there's no conflict here.
>
> Tong Li wrote:
> Thanks Gwen, completely agreed. What puzzled me is that the compile error. I do not know how to explain that. I have uploaded the screen shot of the error against the jira issue, I wonder if you can give a bit of opinion on that? Here is the link https://issues.apache.org/jira/secure/attachment/12699149/compileError.png
interesting. I didn't see this error on my setup.
Which Scala / Java versions are you using?
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31088/#review72637
-----------------------------------------------------------
On Feb. 16, 2015, 4:37 p.m., Tong Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31088/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2015, 4:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1959
> https://issues.apache.org/jira/browse/KAFKA-1959
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The original code clearly wants to define a string but override the ThreadGroup of
> the super class member. The patchset will rename the private variable to be group_id
> as it intended.
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestOffsetManager.scala 41f334d48897b3027ed54c58bbf4811487d3b191
>
> Diff: https://reviews.apache.org/r/31088/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tong Li
>
>
Re: Review Request 31088: Patch for KAFKA-1959
Posted by Gwen Shapira <gs...@cloudera.com>.
> On Feb. 16, 2015, 6:22 p.m., Gwen Shapira wrote:
> > The rename does make things a bit clearer, so I don't object to committing it.
> > But the test does not override "group" in superclas Thread. Thread.group is a private variable, so it cannot be overriden (and the classes that inherit from it can't use it) so there's no conflict here.
>
> Tong Li wrote:
> Thanks Gwen, completely agreed. What puzzled me is that the compile error. I do not know how to explain that. I have uploaded the screen shot of the error against the jira issue, I wonder if you can give a bit of opinion on that? Here is the link https://issues.apache.org/jira/secure/attachment/12699149/compileError.png
>
> Gwen Shapira wrote:
> interesting. I didn't see this error on my setup.
>
> Which Scala / Java versions are you using?
>
> Tong Li wrote:
> Gwen, thanks again. This error really bugs me. I am using IBM JDK 7 (can not use oracle jdk because I am an IBMer). Scala version 2.10.4.
>
> Tong Li wrote:
> I replaced the IBM JDK7 with Oracle JDK 7. Not seeing that compile error. I am pretty sure it is related to the jdk. Would you still like the patch to be merged? I think it still helps if some other people running IBM JDK.
Yeah, I suspect that Oracle's Thread class has "group" as private, but IBM's has it as public or protected.
Perhaps you'll want to let the IBM JDK team know about it issue?
Agree it won't hurt to change the variable name and may help some people.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31088/#review72637
-----------------------------------------------------------
On Feb. 16, 2015, 4:37 p.m., Tong Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31088/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2015, 4:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1959
> https://issues.apache.org/jira/browse/KAFKA-1959
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The original code clearly wants to define a string but override the ThreadGroup of
> the super class member. The patchset will rename the private variable to be group_id
> as it intended.
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestOffsetManager.scala 41f334d48897b3027ed54c58bbf4811487d3b191
>
> Diff: https://reviews.apache.org/r/31088/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tong Li
>
>
Re: Review Request 31088: Patch for KAFKA-1959
Posted by Tong Li <li...@us.ibm.com>.
> On Feb. 16, 2015, 6:22 p.m., Gwen Shapira wrote:
> > The rename does make things a bit clearer, so I don't object to committing it.
> > But the test does not override "group" in superclas Thread. Thread.group is a private variable, so it cannot be overriden (and the classes that inherit from it can't use it) so there's no conflict here.
Thanks Gwen, completely agreed. What puzzled me is that the compile error. I do not know how to explain that. I have uploaded the screen shot of the error against the jira issue, I wonder if you can give a bit of opinion on that? Here is the link https://issues.apache.org/jira/secure/attachment/12699149/compileError.png
- Tong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31088/#review72637
-----------------------------------------------------------
On Feb. 16, 2015, 4:37 p.m., Tong Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31088/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2015, 4:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1959
> https://issues.apache.org/jira/browse/KAFKA-1959
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The original code clearly wants to define a string but override the ThreadGroup of
> the super class member. The patchset will rename the private variable to be group_id
> as it intended.
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestOffsetManager.scala 41f334d48897b3027ed54c58bbf4811487d3b191
>
> Diff: https://reviews.apache.org/r/31088/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tong Li
>
>
Re: Review Request 31088: Patch for KAFKA-1959
Posted by Tong Li <li...@us.ibm.com>.
> On Feb. 16, 2015, 6:22 p.m., Gwen Shapira wrote:
> > The rename does make things a bit clearer, so I don't object to committing it.
> > But the test does not override "group" in superclas Thread. Thread.group is a private variable, so it cannot be overriden (and the classes that inherit from it can't use it) so there's no conflict here.
>
> Tong Li wrote:
> Thanks Gwen, completely agreed. What puzzled me is that the compile error. I do not know how to explain that. I have uploaded the screen shot of the error against the jira issue, I wonder if you can give a bit of opinion on that? Here is the link https://issues.apache.org/jira/secure/attachment/12699149/compileError.png
>
> Gwen Shapira wrote:
> interesting. I didn't see this error on my setup.
>
> Which Scala / Java versions are you using?
Gwen, thanks again. This error really bugs me. I am using IBM JDK 7 (can not use oracle jdk because I am an IBMer). Scala version 2.10.4.
- Tong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31088/#review72637
-----------------------------------------------------------
On Feb. 16, 2015, 4:37 p.m., Tong Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31088/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2015, 4:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1959
> https://issues.apache.org/jira/browse/KAFKA-1959
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The original code clearly wants to define a string but override the ThreadGroup of
> the super class member. The patchset will rename the private variable to be group_id
> as it intended.
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestOffsetManager.scala 41f334d48897b3027ed54c58bbf4811487d3b191
>
> Diff: https://reviews.apache.org/r/31088/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tong Li
>
>
Re: Review Request 31088: Patch for KAFKA-1959
Posted by Tong Li <li...@us.ibm.com>.
> On Feb. 16, 2015, 6:22 p.m., Gwen Shapira wrote:
> > The rename does make things a bit clearer, so I don't object to committing it.
> > But the test does not override "group" in superclas Thread. Thread.group is a private variable, so it cannot be overriden (and the classes that inherit from it can't use it) so there's no conflict here.
>
> Tong Li wrote:
> Thanks Gwen, completely agreed. What puzzled me is that the compile error. I do not know how to explain that. I have uploaded the screen shot of the error against the jira issue, I wonder if you can give a bit of opinion on that? Here is the link https://issues.apache.org/jira/secure/attachment/12699149/compileError.png
>
> Gwen Shapira wrote:
> interesting. I didn't see this error on my setup.
>
> Which Scala / Java versions are you using?
>
> Tong Li wrote:
> Gwen, thanks again. This error really bugs me. I am using IBM JDK 7 (can not use oracle jdk because I am an IBMer). Scala version 2.10.4.
I replaced the IBM JDK7 with Oracle JDK 7. Not seeing that compile error. I am pretty sure it is related to the jdk. Would you still like the patch to be merged? I think it still helps if some other people running IBM JDK.
- Tong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31088/#review72637
-----------------------------------------------------------
On Feb. 16, 2015, 4:37 p.m., Tong Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31088/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2015, 4:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1959
> https://issues.apache.org/jira/browse/KAFKA-1959
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The original code clearly wants to define a string but override the ThreadGroup of
> the super class member. The patchset will rename the private variable to be group_id
> as it intended.
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestOffsetManager.scala 41f334d48897b3027ed54c58bbf4811487d3b191
>
> Diff: https://reviews.apache.org/r/31088/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tong Li
>
>
Re: Review Request 31088: Patch for KAFKA-1959
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31088/#review72637
-----------------------------------------------------------
The rename does make things a bit clearer, so I don't object to committing it.
But the test does not override "group" in superclas Thread. Thread.group is a private variable, so it cannot be overriden (and the classes that inherit from it can't use it) so there's no conflict here.
- Gwen Shapira
On Feb. 16, 2015, 4:37 p.m., Tong Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31088/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2015, 4:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1959
> https://issues.apache.org/jira/browse/KAFKA-1959
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The original code clearly wants to define a string but override the ThreadGroup of
> the super class member. The patchset will rename the private variable to be group_id
> as it intended.
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestOffsetManager.scala 41f334d48897b3027ed54c58bbf4811487d3b191
>
> Diff: https://reviews.apache.org/r/31088/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tong Li
>
>