You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Yan Fang <ya...@gmail.com> on 2014/04/25 03:07:38 UTC
Review Request 20697: SAMZA-151
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20697/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
Fail early when a consumer is misconfigured.
1. wraped the consumer register with try and catch
2. added SystemConsumerException extends SamzaException
3. added test for checking throwing the correct exception
Diffs
-----
samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 7624aef
samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala e1b211d
Diff: https://reviews.apache.org/r/20697/diff/
Testing
-------
Thanks,
Yan Fang
Re: Review Request 20697: SAMZA-151
Posted by Jakob Homan <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20697/#review41956
-----------------------------------------------------------
Ship it!
Ship It!
- Jakob Homan
On April 25, 2014, 2:27 p.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20697/
> -----------------------------------------------------------
>
> (Updated April 25, 2014, 2:27 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Fail early when a consumer is misconfigured.
>
> 1. wraped the consumer register with try and catch
> 2. added SystemConsumerException extends SamzaException
> 3. added test for checking throwing the correct exception
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 7624aef
> samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala e1b211d
>
> Diff: https://reviews.apache.org/r/20697/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yan Fang
>
>
Re: Review Request 20697: SAMZA-151
Posted by Martin Kleppmann <mk...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20697/#review41603
-----------------------------------------------------------
samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala
<https://reviews.apache.org/r/20697/#comment75131>
It's dangerous to catch Throwable -- this should probably be catching Exception. (See https://issues.apache.org/jira/browse/SAMZA-178) Or use the "intercept" feature of ScalaTest, as mentioned in another comment.
Also, nit: looks like you're using tab characters here for indentation. I think we're using spaces everywhere else, and different people have different settings for the width of tab characters (2, 4, 8, ...) in their editors, so I'd prefer if we could stick with spaces everywhere.
- Martin Kleppmann
On April 25, 2014, 9:27 p.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20697/
> -----------------------------------------------------------
>
> (Updated April 25, 2014, 9:27 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Fail early when a consumer is misconfigured.
>
> 1. wraped the consumer register with try and catch
> 2. added SystemConsumerException extends SamzaException
> 3. added test for checking throwing the correct exception
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 7624aef
> samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala e1b211d
>
> Diff: https://reviews.apache.org/r/20697/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yan Fang
>
>
Re: Review Request 20697: SAMZA-151
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20697/
-----------------------------------------------------------
(Updated April 25, 2014, 9:27 p.m.)
Review request for samza.
Changes
-------
1. moved unit test to a seperate method. have to repeat some code that is already in testSystemConumersShouldRegisterStartAndStopChooser, which I think should be fine.
2. catch _. And use the pattern you provided. (I like this pattern more).
Repository: samza
Description
-------
Fail early when a consumer is misconfigured.
1. wraped the consumer register with try and catch
2. added SystemConsumerException extends SamzaException
3. added test for checking throwing the correct exception
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 7624aef
samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala e1b211d
Diff: https://reviews.apache.org/r/20697/diff/
Testing
-------
Thanks,
Yan Fang
Re: Review Request 20697: SAMZA-151
Posted by Martin Kleppmann <mk...@linkedin.com>.
> On April 25, 2014, 7:54 p.m., Jakob Homan wrote:
> > samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala, line 68
> > <https://reviews.apache.org/r/20697/diff/2/?file=568247#file568247line68>
> >
> > We need to catch _ here and fail as well. Otherwise some problem could happen in register but this would silently swallow it. Alternatively, the
> >
> > var caughtRightException = false
> > try {
> > blah
> > } catch {
> > case e: caughtRightException = true
> > }
> > assertTrue(caughtRightException)
> >
> > pattern will catch this as well.
More idiomatically, you can also use ScalaTest's "intercept" feature (which asserts that an exception of a particular type is thrown):
intercept[SystemConsumersException] {
consumers.register(systemStreamPartition_2, "0")
}
See TestStreamMetadataCache for an example of intercept in action.
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20697/#review41514
-----------------------------------------------------------
On April 25, 2014, 9:27 p.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20697/
> -----------------------------------------------------------
>
> (Updated April 25, 2014, 9:27 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Fail early when a consumer is misconfigured.
>
> 1. wraped the consumer register with try and catch
> 2. added SystemConsumerException extends SamzaException
> 3. added test for checking throwing the correct exception
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 7624aef
> samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala e1b211d
>
> Diff: https://reviews.apache.org/r/20697/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yan Fang
>
>
Re: Review Request 20697: SAMZA-151
Posted by Jakob Homan <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20697/#review41514
-----------------------------------------------------------
samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala
<https://reviews.apache.org/r/20697/#comment74962>
Can we move this into its own test for cleaner reporting?
samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala
<https://reviews.apache.org/r/20697/#comment74963>
We need to catch _ here and fail as well. Otherwise some problem could happen in register but this would silently swallow it. Alternatively, the
var caughtRightException = false
try {
blah
} catch {
case e: caughtRightException = true
}
assertTrue(caughtRightException)
pattern will catch this as well.
- Jakob Homan
On April 24, 2014, 6:07 p.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20697/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 6:07 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Fail early when a consumer is misconfigured.
>
> 1. wraped the consumer register with try and catch
> 2. added SystemConsumerException extends SamzaException
> 3. added test for checking throwing the correct exception
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala 7624aef
> samza-core/src/test/scala/org/apache/samza/system/TestSystemConsumers.scala e1b211d
>
> Diff: https://reviews.apache.org/r/20697/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yan Fang
>
>