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 2015/07/21 22:11:10 UTC
Review Request 36652: Patch for KAFKA-2351
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2351
https://issues.apache.org/jira/browse/KAFKA-2351
Repository: kafka
Description
-------
Added a try-catch to catch any exceptions thrown by the nioSelector
Diffs
-----
core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
Diff: https://reviews.apache.org/r/36652/diff/
Testing
-------
Thanks,
Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92488
-----------------------------------------------------------
Ship it!
Latest patch looks good to me.
- Jiangjie Qin
On July 21, 2015, 9:58 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 9:58 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Jun Rao <ju...@gmail.com>.
> On July 24, 2015, 5:01 p.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 264
> > <https://reviews.apache.org/r/36652/diff/3/?file=1020607#file1020607line264>
> >
> > Hi Jun,
> > Cleaning up in finally is actually nice. I will make the necessary change and upload a new patch.
> >
> > I was looking at the patch for :
> > https://issues.apache.org/jira/browse/KAFKA-2353
> > as per the suggestions on the jira ticket.
> >
> > Was just curious if we can do the same there as well. We are catching all the Throwables and allowing the thread to continue processing. Is there something I am missing here.
Mayuresh,
Yes, saw the changes in KAFKA-2353. We can leave this the way that you did for now and revisit the catch throwable issue later.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92928
-----------------------------------------------------------
On July 24, 2015, 4:36 a.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 4:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92928
-----------------------------------------------------------
core/src/main/scala/kafka/network/SocketServer.scala (line 264)
<https://reviews.apache.org/r/36652/#comment147193>
Hi Jun,
Cleaning up in finally is actually nice. I will make the necessary change and upload a new patch.
I was looking at the patch for :
https://issues.apache.org/jira/browse/KAFKA-2353
as per the suggestions on the jira ticket.
Was just curious if we can do the same there as well. We are catching all the Throwables and allowing the thread to continue processing. Is there something I am missing here.
- Mayuresh Gharat
On July 24, 2015, 4:36 a.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 4:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review96463
-----------------------------------------------------------
Ship it!
Ship It!
- Joel Koshy
On Aug. 24, 2015, 10:50 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2015, 10:50 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Addressed Joel's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 649812d9f8014edbd9e99113a0f9eaf186360bcc
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/
-----------------------------------------------------------
(Updated Aug. 24, 2015, 10:50 p.m.)
Review request for kafka.
Bugs: KAFKA-2351
https://issues.apache.org/jira/browse/KAFKA-2351
Repository: kafka
Description (updated)
-------
Addressed Joel's comments
Diffs (updated)
-----
core/src/main/scala/kafka/network/SocketServer.scala 649812d9f8014edbd9e99113a0f9eaf186360bcc
Diff: https://reviews.apache.org/r/36652/diff/
Testing
-------
Thanks,
Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On Aug. 19, 2015, 5:48 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 265
> > <https://reviews.apache.org/r/36652/diff/4/?file=1039545#file1039545line265>
> >
> > I'm also unclear at this point on what the right thing to do here would be - i.e., log and continue or make it fatal as Becket suggested. I'm leaning toward the latter but I agree we could revisit this.
By fatal do you mean that we should shutdown the broker?
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review95824
-----------------------------------------------------------
On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Addressed Jun's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review95824
-----------------------------------------------------------
Patch needs a rebase.
core/src/main/scala/kafka/network/SocketServer.scala (line 263)
<https://reviews.apache.org/r/36652/#comment150967>
Per earlier comment - we don't need this right?
core/src/main/scala/kafka/network/SocketServer.scala (line 264)
<https://reviews.apache.org/r/36652/#comment150968>
Can you clarify why we need this? i.e., catch and rethrow without any logging?
core/src/main/scala/kafka/network/SocketServer.scala (line 265)
<https://reviews.apache.org/r/36652/#comment150970>
I'm also unclear at this point on what the right thing to do here would be - i.e., log and continue or make it fatal as Becket suggested. I'm leaning toward the latter but I agree we could revisit this.
- Joel Koshy
On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Addressed Jun's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On Aug. 21, 2015, 6:14 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 264
> > <https://reviews.apache.org/r/36652/diff/4/?file=1039545#file1039545line264>
> >
> > Yes that's exactly what we need. Any reason why we shouldn't use `NonFatal` as is? i.e., if we hit VM errors such as those in `NonFatal` then we should probably let it propagate anyway. Can you file a jira to do this across the board?
>
> Ismael Juma wrote:
> There have been a few cases where we wanted to catch some fatal errors too (in `SocketServer`, if I remember correctly). In many (most?) cases `NonFatal` is probably OK though. Sure, I'll file a JIRA.
Cool. I will upload a new patch for this.
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review96069
-----------------------------------------------------------
On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Addressed Jun's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Ismael Juma <mi...@juma.me.uk>.
> On Aug. 21, 2015, 6:14 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 264
> > <https://reviews.apache.org/r/36652/diff/4/?file=1039545#file1039545line264>
> >
> > Yes that's exactly what we need. Any reason why we shouldn't use `NonFatal` as is? i.e., if we hit VM errors such as those in `NonFatal` then we should probably let it propagate anyway. Can you file a jira to do this across the board?
There have been a few cases where we wanted to catch some fatal errors too (in `SocketServer`, if I remember correctly). In many (most?) cases `NonFatal` is probably OK though. Sure, I'll file a JIRA.
- Ismael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review96069
-----------------------------------------------------------
On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Addressed Jun's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review96069
-----------------------------------------------------------
core/src/main/scala/kafka/network/SocketServer.scala (line 264)
<https://reviews.apache.org/r/36652/#comment151273>
Yes that's exactly what we need. Any reason why we shouldn't use `NonFatal` as is? i.e., if we hit VM errors such as those in `NonFatal` then we should probably let it propagate anyway. Can you file a jira to do this across the board?
- Joel Koshy
On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Addressed Jun's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Ismael Juma <mi...@juma.me.uk>.
> On Aug. 21, 2015, 5:57 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 264
> > <https://reviews.apache.org/r/36652/diff/4/?file=1039545#file1039545line264>
> >
> > Thanks for the reference - we currently have this pattern all over the place. We can keep what you have, but this should probably become a utility say `CoreUtils.catchThrowable` (similar to `CoreUtils.swallow*`)
I would suggest creating a `NonControl` extractor that is similar to `NonFatal` (https://github.com/scala/scala/blob/v2.11.7/src/library/scala/util/control/NonFatal.scala), but more limited.
- Ismael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review96061
-----------------------------------------------------------
On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Addressed Jun's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review96061
-----------------------------------------------------------
core/src/main/scala/kafka/network/SocketServer.scala (line 264)
<https://reviews.apache.org/r/36652/#comment151266>
Thanks for the reference - we currently have this pattern all over the place. We can keep what you have, but this should probably become a utility say `CoreUtils.catchThrowable` (similar to `CoreUtils.swallow*`)
core/src/main/scala/kafka/network/SocketServer.scala (line 265)
<https://reviews.apache.org/r/36652/#comment151267>
Yes, but we don't need to do that now.
- Joel Koshy
On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Addressed Jun's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review95868
-----------------------------------------------------------
core/src/main/scala/kafka/network/SocketServer.scala (line 264)
<https://reviews.apache.org/r/36652/#comment151020>
I referred this : https://www.sumologic.com/2014/05/05/why-you-should-never-catch-throwable-in-scala/ from the kafka 2353 patch.
http://www.tzavellas.com/techblog/2010/09/20/catching-throwable-in-scala/
- Mayuresh Gharat
On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Addressed Jun's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/
-----------------------------------------------------------
(Updated Aug. 13, 2015, 8:10 p.m.)
Review request for kafka.
Bugs: KAFKA-2351
https://issues.apache.org/jira/browse/KAFKA-2351
Repository: kafka
Description (updated)
-------
Added a try-catch to catch any exceptions thrown by the nioSelector
Addressed comments on the Jira ticket
Addressed Jun's comments
Diffs (updated)
-----
core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7
Diff: https://reviews.apache.org/r/36652/diff/
Testing
-------
Thanks,
Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Jiangjie Qin <be...@gmail.com>.
> On July 24, 2015, 4:13 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 264
> > <https://reviews.apache.org/r/36652/diff/3/?file=1020607#file1020607line264>
> >
> > Not sure if it's better to keep the thread alive on any throwable. For unexpected exceptions, it seems it's better to just propagate the exception, log it and then kill the thread. This is already done through Utils.newThread. If we want to clean things up (e.g. countdown) before propagating the exception, we can do that in a finally clause.
Hi Jun, I think only letting the acceptor thread die might cause more issue. The broker in that case will not serve new connections. This essentially makes all the partitions on this broker become unavailable. If we let the accpetor thread exit, maybe we should shutdown the broker completely.
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92920
-----------------------------------------------------------
On July 24, 2015, 4:36 a.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 4:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92920
-----------------------------------------------------------
Thanks for the patch. A few comments below.
core/src/main/scala/kafka/network/SocketServer.scala (line 262)
<https://reviews.apache.org/r/36652/#comment147188>
We don't interrupt the Acceptor thread. So this is unnecessary.
core/src/main/scala/kafka/network/SocketServer.scala (line 264)
<https://reviews.apache.org/r/36652/#comment147192>
Not sure if it's better to keep the thread alive on any throwable. For unexpected exceptions, it seems it's better to just propagate the exception, log it and then kill the thread. This is already done through Utils.newThread. If we want to clean things up (e.g. countdown) before propagating the exception, we can do that in a finally clause.
- Jun Rao
On July 24, 2015, 4:36 a.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 4:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Addressed comments on the Jira ticket
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/
-----------------------------------------------------------
(Updated July 24, 2015, 4:36 a.m.)
Review request for kafka.
Bugs: KAFKA-2351
https://issues.apache.org/jira/browse/KAFKA-2351
Repository: kafka
Description (updated)
-------
Added a try-catch to catch any exceptions thrown by the nioSelector
Addressed comments on the Jira ticket
Diffs (updated)
-----
core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
Diff: https://reviews.apache.org/r/36652/diff/
Testing
-------
Thanks,
Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/
-----------------------------------------------------------
(Updated July 21, 2015, 9:58 p.m.)
Review request for kafka.
Bugs: KAFKA-2351
https://issues.apache.org/jira/browse/KAFKA-2351
Repository: kafka
Description
-------
Added a try-catch to catch any exceptions thrown by the nioSelector
Diffs (updated)
-----
core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
Diff: https://reviews.apache.org/r/36652/diff/
Testing
-------
Thanks,
Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
On July 21, 2015, 8:18 p.m., Mayuresh Gharat wrote:
> > T
Yes. Got it, I thought that we should be catching all exceptions and exit. But doing the above will catch the exception and exit when its shutting down and thats the only thing that this ticket considers.
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92465
-----------------------------------------------------------
On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 8:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92465
-----------------------------------------------------------
Thanks for the patch, some comments.
core/src/main/scala/kafka/network/SocketServer.scala (lines 234 - 235)
<https://reviews.apache.org/r/36652/#comment146660>
We probably want to keep the try-catch inside the while loop.
core/src/main/scala/kafka/network/SocketServer.scala (line 235)
<https://reviews.apache.org/r/36652/#comment146661>
Open source Kafka convention is to put the bracket on the same line.
T
- Jiangjie Qin
On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 8:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On July 21, 2015, 8:26 p.m., Grant Henke wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 266
> > <https://reviews.apache.org/r/36652/diff/1/?file=1018073#file1018073line266>
> >
> > What errors were seen that should be caught here? Can we catch a more specific exception and provide a better message?
The nioSelector can throw different exceptions : IOException, ClosedSelectorException, IllegalArgumentException. We can have different catch for each of them. But we thought that the log will telll us what exception was thrown when we pass it to error()
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92470
-----------------------------------------------------------
On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 8:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Grant Henke <gr...@gmail.com>.
> On July 21, 2015, 8:26 p.m., Grant Henke wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 266
> > <https://reviews.apache.org/r/36652/diff/1/?file=1018073#file1018073line266>
> >
> > What errors were seen that should be caught here? Can we catch a more specific exception and provide a better message?
>
> Mayuresh Gharat wrote:
> The nioSelector can throw different exceptions : IOException, ClosedSelectorException, IllegalArgumentException. We can have different catch for each of them. But we thought that the log will telll us what exception was thrown when we pass it to error()
I assumed you are adding this because you saw a specific error. I wasn't sure what error you saw and if more context could be given to the user. Perhaps the error you saw is fairly common during shutdown and should be ignored, and not logged at the error level. But all others should be handle as you are here.
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92470
-----------------------------------------------------------
On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 8:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36652: Patch for KAFKA-2351
Posted by Grant Henke <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36652/#review92470
-----------------------------------------------------------
core/src/main/scala/kafka/network/SocketServer.scala (line 266)
<https://reviews.apache.org/r/36652/#comment146669>
What errors were seen that should be caught here? Can we catch a more specific exception and provide a better message?
- Grant Henke
On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36652/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 8:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2351
> https://issues.apache.org/jira/browse/KAFKA-2351
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Added a try-catch to catch any exceptions thrown by the nioSelector
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9
>
> Diff: https://reviews.apache.org/r/36652/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mayuresh Gharat
>
>