----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18907/#review37736 -----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/util/ExponentialSleepStrategy.scala <https://reviews.apache.org/r/18907/#comment69379> Should add some javadocs for class, all constructor parameters, and all public methods. samza-core/src/main/scala/org/apache/samza/util/ExponentialSleepStrategy.scala <https://reviews.apache.org/r/18907/#comment69384> What do you think about using blocks instead of method parameters here (like we do with TestUtil.expect)? I think it might make the code a little cleaner: run { // do some stuff } Not quite sure how to make it work with multiple blocks (to handle onException). Probably requires some fiddling around. samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManager.scala <https://reviews.apache.org/r/18907/#comment69402> Specify which partition in the log line. samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala <https://reviews.apache.org/r/18907/#comment69497> Comment here explaining why we reset would be useful. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala <https://reviews.apache.org/r/18907/#comment69516> I think we should call assembleMetadata first, then loop.done, then return the results. The reason for this is that we might throw an exception in assembleMetadata, but loop.done would already have stop the run loop. Alternatively, maybe we should set loop.notDone or something in the exception closure, which would overwrite loop.done here. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemConsumer.scala <https://reviews.apache.org/r/18907/#comment69505> 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. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala <https://reviews.apache.org/r/18907/#comment69511> Do we need a reset if we call done right after? The next time run is called, it should reset, right? samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemAdmin.scala <https://reviews.apache.org/r/18907/#comment69512> Should we verify retryBackoff.sleepCount here? - Chris Riccomini On March 7, 2014, 4:21 p.m., Martin Kleppmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18907/ > ----------------------------------------------------------- > > (Updated March 7, 2014, 4:21 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > 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 > >
