> On March 19, 2014, 8:25 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemConsumer.scala, > > line 150 > > <https://reviews.apache.org/r/18907/diff/1/?file=513612#file513612line150> > > > > This chunk of code is a little funky. I get that the loop runner will > > forward the interrupted exception, but it's not really immediately obvious > > that we can catch interrupt exceptions but not other kinds of exceptions. > > > > Wonder if we should add a call back for interrupt and closed on > > interrupt exceptions, and default to throwing in cases where callbacks > > aren't passed in? Doesn't seem like a very clean solution either, though. > > Martin Kleppmann wrote: > Agree it's funky -- I was just trying to preserve the existing code's > behaviour. ;) > > I don't know why InterruptedException was special-cased in the first > place. Should we just let it bubble out, without calling `stop`? Or call > `stop` on any kind of exception, not just InterruptedException? Depends on > the expected semantics when SystemConsumer.start throws an exception. Looks > like an exception thrown here will just cause the container to shut down > anyway, so it probably doesn't matter whether or not we call `stop`.
I think the intention was to shutdown the consumer so we don't leak anything. That said, the container is going to catch interrupted exception, and shut everything down anyway, and we don't do this for any of the other classes (producer, checkpoint, etc). I'm inclined to remove it. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18907/#review37736 ----------------------------------------------------------- On March 19, 2014, 10:26 p.m., Martin Kleppmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18907/ > ----------------------------------------------------------- > > (Updated March 19, 2014, 10:26 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > Improvements suggested by Chris in review. > > > SAMZA-174 General-purpose implementation of a retry loop > > > Diffs > ----- > > > samza-core/src/main/scala/org/apache/samza/util/ExponentialSleepStrategy.scala > b3c926310920c2c204a805421259d0fa54213170 > > samza-core/src/test/scala/org/apache/samza/util/TestExponentialSleepStrategy.scala > 3036da93728825d2d37e6791e89aa9801195ad38 > > samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManager.scala > 27b38b25dc6c34f3ef76d400370d1c857834e6a2 > samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala > 10855dcd33133b1b734e500bdf2c1ba85bba13b6 > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala > 53255490dfbcd926a0d9975415ce437f01c29264 > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemConsumer.scala > afbd7cdc1436a0d9a83857c0f410a2977812ee77 > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala > b09ade28d4363a4b70a3e4cf942e7974acac596c > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala > a4197832075dbb67fb68a81c16ee98d6a2b358a0 > > samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemAdmin.scala > eaa9e53db0b641e16b4eafe2be89ae6d7b4e5384 > > samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala > cd0942aae06fa9e358672f0e862711bfb677895d > > Diff: https://reviews.apache.org/r/18907/diff/ > > > Testing > ------- > > > Thanks, > > Martin Kleppmann > >
