You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Jakob Homan <jg...@apache.org> on 2014/02/28 04:28:55 UTC
Review Request 18606: Fix warnings during check
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/
-----------------------------------------------------------
Review request for samza.
Bugs: SAMZA-161
https://issues.apache.org/jira/browse/SAMZA-161
Repository: samza
Description
-------
SAMZA-161
Diffs
-----
build.gradle 31c54be
gradle/dependency-versions-scala-2.10.gradle 47de65a
gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
gradle/license.gradle b4b62eb
samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala a60de10
samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
samza-test/src/main/resources/common.properties 971a219
samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
samza-test/src/main/resources/join/checker.samsa e41ffa0
samza-test/src/main/resources/join/emitter.samsa 140d13d
samza-test/src/main/resources/join/joiner.samsa 27655d8
samza-test/src/main/resources/join/watcher.samsa a4cc761
samza-test/src/main/resources/log4j.xml ecaf8a2
samza-test/src/main/resources/perf/counter.samsa cf06c9e
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
Diff: https://reviews.apache.org/r/18606/diff/
Testing
-------
Unit tests
Thanks,
Jakob Homan
Re: Review Request 18606: Fix warnings during check
Posted by Jakob Homan <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/#review35759
-----------------------------------------------------------
gradle/dependency-versions-scala-2.10.gradle
<https://reviews.apache.org/r/18606/#comment66493>
These are warnings we get from 2.10, which we're choosing to use but can't suppress via imports because Scala 2.9 has no such libraries to import.
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala
<https://reviews.apache.org/r/18606/#comment66494>
This is a bug. The fall-through will never get called a far as I can tell.
- Jakob Homan
On Feb. 27, 2014, 7:28 p.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18606/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 7:28 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-161
> https://issues.apache.org/jira/browse/SAMZA-161
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-161
>
>
> Diffs
> -----
>
> build.gradle 31c54be
> gradle/dependency-versions-scala-2.10.gradle 47de65a
> gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
> gradle/license.gradle b4b62eb
> samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
> samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala a60de10
> samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
> samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
> samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
> samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
> samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
> samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
> samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
> samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
> samza-test/src/main/resources/common.properties 971a219
> samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
> samza-test/src/main/resources/join/checker.samsa e41ffa0
> samza-test/src/main/resources/join/emitter.samsa 140d13d
> samza-test/src/main/resources/join/joiner.samsa 27655d8
> samza-test/src/main/resources/join/watcher.samsa a4cc761
> samza-test/src/main/resources/log4j.xml ecaf8a2
> samza-test/src/main/resources/perf/counter.samsa cf06c9e
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
>
> Diff: https://reviews.apache.org/r/18606/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 18606: Fix warnings during check
Posted by Jakob Homan <jg...@apache.org>.
> On March 7, 2014, 5:44 a.m., Martin Kleppmann wrote:
> > I was wondering, rather than changing all those "catch { case _ =>" occurrences to catch Throwable, wouldn't it be better to change them to catch Exception instead? I just got caught out by a "catch Throwable", which was catching scala.runtime.NonLocalReturnControl (which is Throwable but not an Exception). NonLocalReturnControl is used internally by Scala for unusual flow control in some cases, such as jumping out of nested closures (I think). It's not something that normal code should be catching.
>
> Chris Riccomini wrote:
> Yea, in general, I think we need to go through the code and switch all Throwables to Exceptions in the catch clauses. Can we open a separate JIRA to just do an audit and fix everything?
That's fine with me. The exact message scalac gives is to suggest changing to Throwable, but I just tried with Exception and it looks like that silences the warning just as well. I'll update the patch.
- Jakob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/#review36516
-----------------------------------------------------------
On Feb. 28, 2014, 5:19 p.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18606/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2014, 5:19 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-161
> https://issues.apache.org/jira/browse/SAMZA-161
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-161
>
>
> Diffs
> -----
>
> build.gradle 31c54be
> gradle/dependency-versions-scala-2.10.gradle 47de65a
> gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
> gradle/license.gradle b4b62eb
> samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
> samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 0d71582
> samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
> samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
> samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
> samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
> samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
> samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
> samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
> samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
> samza-test/src/main/resources/common.properties 971a219
> samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
> samza-test/src/main/resources/join/checker.samsa e41ffa0
> samza-test/src/main/resources/join/emitter.samsa 140d13d
> samza-test/src/main/resources/join/joiner.samsa 27655d8
> samza-test/src/main/resources/join/watcher.samsa a4cc761
> samza-test/src/main/resources/log4j.xml ecaf8a2
> samza-test/src/main/resources/perf/counter.samsa cf06c9e
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
>
> Diff: https://reviews.apache.org/r/18606/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 18606: Fix warnings during check
Posted by Chris Riccomini <cr...@apache.org>.
> On March 7, 2014, 1:44 p.m., Martin Kleppmann wrote:
> > I was wondering, rather than changing all those "catch { case _ =>" occurrences to catch Throwable, wouldn't it be better to change them to catch Exception instead? I just got caught out by a "catch Throwable", which was catching scala.runtime.NonLocalReturnControl (which is Throwable but not an Exception). NonLocalReturnControl is used internally by Scala for unusual flow control in some cases, such as jumping out of nested closures (I think). It's not something that normal code should be catching.
Yea, in general, I think we need to go through the code and switch all Throwables to Exceptions in the catch clauses. Can we open a separate JIRA to just do an audit and fix everything?
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/#review36516
-----------------------------------------------------------
On March 1, 2014, 1:19 a.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18606/
> -----------------------------------------------------------
>
> (Updated March 1, 2014, 1:19 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-161
> https://issues.apache.org/jira/browse/SAMZA-161
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-161
>
>
> Diffs
> -----
>
> build.gradle 31c54be
> gradle/dependency-versions-scala-2.10.gradle 47de65a
> gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
> gradle/license.gradle b4b62eb
> samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
> samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 0d71582
> samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
> samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
> samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
> samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
> samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
> samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
> samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
> samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
> samza-test/src/main/resources/common.properties 971a219
> samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
> samza-test/src/main/resources/join/checker.samsa e41ffa0
> samza-test/src/main/resources/join/emitter.samsa 140d13d
> samza-test/src/main/resources/join/joiner.samsa 27655d8
> samza-test/src/main/resources/join/watcher.samsa a4cc761
> samza-test/src/main/resources/log4j.xml ecaf8a2
> samza-test/src/main/resources/perf/counter.samsa cf06c9e
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
>
> Diff: https://reviews.apache.org/r/18606/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 18606: Fix warnings during check
Posted by Martin Kleppmann <mk...@linkedin.com>.
> On March 7, 2014, 1:44 p.m., Martin Kleppmann wrote:
> > I was wondering, rather than changing all those "catch { case _ =>" occurrences to catch Throwable, wouldn't it be better to change them to catch Exception instead? I just got caught out by a "catch Throwable", which was catching scala.runtime.NonLocalReturnControl (which is Throwable but not an Exception). NonLocalReturnControl is used internally by Scala for unusual flow control in some cases, such as jumping out of nested closures (I think). It's not something that normal code should be catching.
>
> Chris Riccomini wrote:
> Yea, in general, I think we need to go through the code and switch all Throwables to Exceptions in the catch clauses. Can we open a separate JIRA to just do an audit and fix everything?
>
> Jakob Homan wrote:
> That's fine with me. The exact message scalac gives is to suggest changing to Throwable, but I just tried with Exception and it looks like that silences the warning just as well. I'll update the patch.
Sounds good, thanks. I've created SAMZA-178 for checking any remaining Throwables that are not covered by this patch.
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/#review36516
-----------------------------------------------------------
On March 1, 2014, 1:19 a.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18606/
> -----------------------------------------------------------
>
> (Updated March 1, 2014, 1:19 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-161
> https://issues.apache.org/jira/browse/SAMZA-161
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-161
>
>
> Diffs
> -----
>
> build.gradle 31c54be
> gradle/dependency-versions-scala-2.10.gradle 47de65a
> gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
> gradle/license.gradle b4b62eb
> samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
> samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 0d71582
> samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
> samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
> samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
> samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
> samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
> samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
> samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
> samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
> samza-test/src/main/resources/common.properties 971a219
> samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
> samza-test/src/main/resources/join/checker.samsa e41ffa0
> samza-test/src/main/resources/join/emitter.samsa 140d13d
> samza-test/src/main/resources/join/joiner.samsa 27655d8
> samza-test/src/main/resources/join/watcher.samsa a4cc761
> samza-test/src/main/resources/log4j.xml ecaf8a2
> samza-test/src/main/resources/perf/counter.samsa cf06c9e
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
>
> Diff: https://reviews.apache.org/r/18606/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 18606: Fix warnings during check
Posted by Martin Kleppmann <mk...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/#review36516
-----------------------------------------------------------
I was wondering, rather than changing all those "catch { case _ =>" occurrences to catch Throwable, wouldn't it be better to change them to catch Exception instead? I just got caught out by a "catch Throwable", which was catching scala.runtime.NonLocalReturnControl (which is Throwable but not an Exception). NonLocalReturnControl is used internally by Scala for unusual flow control in some cases, such as jumping out of nested closures (I think). It's not something that normal code should be catching.
- Martin Kleppmann
On March 1, 2014, 1:19 a.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18606/
> -----------------------------------------------------------
>
> (Updated March 1, 2014, 1:19 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-161
> https://issues.apache.org/jira/browse/SAMZA-161
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-161
>
>
> Diffs
> -----
>
> build.gradle 31c54be
> gradle/dependency-versions-scala-2.10.gradle 47de65a
> gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
> gradle/license.gradle b4b62eb
> samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
> samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 0d71582
> samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
> samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
> samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
> samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
> samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
> samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
> samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
> samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
> samza-test/src/main/resources/common.properties 971a219
> samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
> samza-test/src/main/resources/join/checker.samsa e41ffa0
> samza-test/src/main/resources/join/emitter.samsa 140d13d
> samza-test/src/main/resources/join/joiner.samsa 27655d8
> samza-test/src/main/resources/join/watcher.samsa a4cc761
> samza-test/src/main/resources/log4j.xml ecaf8a2
> samza-test/src/main/resources/perf/counter.samsa cf06c9e
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
>
> Diff: https://reviews.apache.org/r/18606/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 18606: Fix warnings during check
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/#review35975
-----------------------------------------------------------
Ship it!
Ship It!
- Chris Riccomini
On March 1, 2014, 1:19 a.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18606/
> -----------------------------------------------------------
>
> (Updated March 1, 2014, 1:19 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-161
> https://issues.apache.org/jira/browse/SAMZA-161
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-161
>
>
> Diffs
> -----
>
> build.gradle 31c54be
> gradle/dependency-versions-scala-2.10.gradle 47de65a
> gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
> gradle/license.gradle b4b62eb
> samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
> samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 0d71582
> samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
> samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
> samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
> samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
> samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
> samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
> samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
> samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
> samza-test/src/main/resources/common.properties 971a219
> samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
> samza-test/src/main/resources/join/checker.samsa e41ffa0
> samza-test/src/main/resources/join/emitter.samsa 140d13d
> samza-test/src/main/resources/join/joiner.samsa 27655d8
> samza-test/src/main/resources/join/watcher.samsa a4cc761
> samza-test/src/main/resources/log4j.xml ecaf8a2
> samza-test/src/main/resources/perf/counter.samsa cf06c9e
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
>
> Diff: https://reviews.apache.org/r/18606/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 18606: Fix warnings during check
Posted by Jakob Homan <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/
-----------------------------------------------------------
(Updated Feb. 28, 2014, 5:19 p.m.)
Review request for samza.
Changes
-------
Added lots of comments. Removed the postfixOps feature as we were only using it in one place. Left the test as it is. I *think* it's correct as is. Will ask Jay.
Bugs: SAMZA-161
https://issues.apache.org/jira/browse/SAMZA-161
Repository: samza
Description
-------
SAMZA-161
Diffs (updated)
-----
build.gradle 31c54be
gradle/dependency-versions-scala-2.10.gradle 47de65a
gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
gradle/license.gradle b4b62eb
samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 0d71582
samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
samza-test/src/main/resources/common.properties 971a219
samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
samza-test/src/main/resources/join/checker.samsa e41ffa0
samza-test/src/main/resources/join/emitter.samsa 140d13d
samza-test/src/main/resources/join/joiner.samsa 27655d8
samza-test/src/main/resources/join/watcher.samsa a4cc761
samza-test/src/main/resources/log4j.xml ecaf8a2
samza-test/src/main/resources/perf/counter.samsa cf06c9e
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
Diff: https://reviews.apache.org/r/18606/diff/
Testing
-------
Unit tests
Thanks,
Jakob Homan
Re: Review Request 18606: Fix warnings during check
Posted by Jakob Homan <jg...@apache.org>.
> On Feb. 28, 2014, 2:45 p.m., Chris Riccomini wrote:
> > samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala, line 36
> > <https://reviews.apache.org/r/18606/diff/1/?file=506697#file506697line36>
> >
> > Yea, this is weird. Should we switch the ordering of these to make it more obvious that the Throwable is caught before falling into the `case e` block?
Throwable would fire before (except for runtimeerrors), no?
- Jakob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/#review35859
-----------------------------------------------------------
On Feb. 27, 2014, 7:28 p.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18606/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 7:28 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-161
> https://issues.apache.org/jira/browse/SAMZA-161
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-161
>
>
> Diffs
> -----
>
> build.gradle 31c54be
> gradle/dependency-versions-scala-2.10.gradle 47de65a
> gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
> gradle/license.gradle b4b62eb
> samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
> samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala a60de10
> samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
> samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
> samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
> samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
> samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
> samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
> samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
> samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
> samza-test/src/main/resources/common.properties 971a219
> samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
> samza-test/src/main/resources/join/checker.samsa e41ffa0
> samza-test/src/main/resources/join/emitter.samsa 140d13d
> samza-test/src/main/resources/join/joiner.samsa 27655d8
> samza-test/src/main/resources/join/watcher.samsa a4cc761
> samza-test/src/main/resources/log4j.xml ecaf8a2
> samza-test/src/main/resources/perf/counter.samsa cf06c9e
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
>
> Diff: https://reviews.apache.org/r/18606/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 18606: Fix warnings during check
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18606/#review35859
-----------------------------------------------------------
Ship it!
Overall looks good. Would be helpful to have some //comments in the gradle files. Regarding TestUtil, I think it might be better to open a separate ticket to clean it up.
build.gradle
<https://reviews.apache.org/r/18606/#comment66641>
Can you add a comment here to describe what this is doing?
gradle/dependency-versions-scala-2.10.gradle
<https://reviews.apache.org/r/18606/#comment66642>
Can you add a comment here to describe what this is doing?
gradle/license.gradle
<https://reviews.apache.org/r/18606/#comment66644>
Can you add a comment here to describe what this is doing?
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala
<https://reviews.apache.org/r/18606/#comment66645>
Yea, this is weird. Should we switch the ordering of these to make it more obvious that the Throwable is caught before falling into the `case e` block?
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala
<https://reviews.apache.org/r/18606/#comment66648>
I *think* the logic for this function is supposed to be:
1. run block()
2. If block throws exception, the exception should be of the same class as "exception".
3. If the block throws exception, and it matches "exception" and msg is defined, then msg should match exception's message.
4. Fail if block doesn't throw exception, or if exception doesn't match "exception" param, or if msg is defined, and doesn't match exception.message.
This method seems pretty jacked up.
- Chris Riccomini
On Feb. 28, 2014, 3:28 a.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18606/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2014, 3:28 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-161
> https://issues.apache.org/jira/browse/SAMZA-161
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-161
>
>
> Diffs
> -----
>
> build.gradle 31c54be
> gradle/dependency-versions-scala-2.10.gradle 47de65a
> gradle/dependency-versions-scala-2.9.2.gradle e7b56a6
> gradle/license.gradle b4b62eb
> samza-core/src/main/scala/org/apache/samza/config/DefaultChooserConfig.scala 9351c66
> samza-core/src/main/scala/org/apache/samza/serializers/CheckpointSerde.scala 0d82183
> samza-core/src/main/scala/org/apache/samza/system/SystemConsumers.scala dfa3cd7
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala 60c9615
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala a60de10
> samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala 2e7459e
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala 2a74ea5
> samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java 973b0ba
> samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java 873de74
> samza-test/src/main/java/org/apache/samza/test/integration/join/Checker.java 22f5e87
> samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java 2989ca7
> samza-test/src/main/java/org/apache/samza/test/integration/join/EpochPartitioner.java d11d300
> samza-test/src/main/java/org/apache/samza/test/integration/join/Joiner.java ca8fed4
> samza-test/src/main/java/org/apache/samza/test/integration/join/Watcher.java fac4ee1
> samza-test/src/main/resources/common.properties 971a219
> samza-test/src/main/resources/hello-stateful-world.samsa 84325d0
> samza-test/src/main/resources/join/checker.samsa e41ffa0
> samza-test/src/main/resources/join/emitter.samsa 140d13d
> samza-test/src/main/resources/join/joiner.samsa 27655d8
> samza-test/src/main/resources/join/watcher.samsa a4cc761
> samza-test/src/main/resources/log4j.xml ecaf8a2
> samza-test/src/main/resources/perf/counter.samsa cf06c9e
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterLifecycle.scala b24f85a
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala 3064c86
>
> Diff: https://reviews.apache.org/r/18606/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Jakob Homan
>
>