You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by mjsax <gi...@git.apache.org> on 2015/09/16 11:30:59 UTC
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
GitHub user mjsax opened a pull request:
https://github.com/apache/storm/pull/740
[STORM-1044] Setting dop to zero does not raise an error
- added IllegalArgumentException to .setBolt(...), .setSpout(...), and .setStateSpout(...) in TopologyBuilder
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mjsax/storm storm-1044-dop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/740.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #740
----
commit 8ed57abb5a13cf59a66fb7eb3339b27ec2fe3e60
Author: mjsax <mj...@informatik.hu-berlin.de>
Date: 2015-09-15T11:56:56Z
[STORM-1044] Setting dop to zero does not raise an error
- added IllegalArgumentException to .setBolt(...), .setSpout(...), and .setStateSpout(...) in TopologyBuilder
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/740#issuecomment-141841832
@mjsax
Looks good overall.
I just want to know why one of nimbus unit test is removed, though TopologyBuilder prevents topology with zero dop.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/740#discussion_r39936239
--- Diff: storm-core/test/clj/backtype/storm/nimbus_test.clj ---
@@ -846,16 +846,6 @@
NIMBUS-SLOTS-PER-TOPOLOGY 8}]
(letlocals
(bind topology (thrift/mk-topology
- {"1" (thrift/mk-spout-spec (TestPlannerSpout. true) :parallelism-hint 0 :conf {TOPOLOGY-TASKS 1})}
--- End diff --
Because thrift/mk-topology actually throws IllegalArgumentException, not submit-local-topology.
We should wrap bind statement to check IllegalArgumentException.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/storm/pull/740#discussion_r39936261
--- Diff: storm-core/test/clj/backtype/storm/nimbus_test.clj ---
@@ -846,16 +846,6 @@
NIMBUS-SLOTS-PER-TOPOLOGY 8}]
(letlocals
(bind topology (thrift/mk-topology
- {"1" (thrift/mk-spout-spec (TestPlannerSpout. true) :parallelism-hint 0 :conf {TOPOLOGY-TASKS 1})}
--- End diff --
Thx. Got it now :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/740#issuecomment-141843718
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/storm/pull/740#discussion_r39936186
--- Diff: storm-core/test/clj/backtype/storm/nimbus_test.clj ---
@@ -846,16 +846,6 @@
NIMBUS-SLOTS-PER-TOPOLOGY 8}]
(letlocals
(bind topology (thrift/mk-topology
- {"1" (thrift/mk-spout-spec (TestPlannerSpout. true) :parallelism-hint 0 :conf {TOPOLOGY-TASKS 1})}
--- End diff --
I agree on this.
However, I still don't understand why changing the expected exception type does not "fix" the test... If I put `thrown? IllegalArgumentException` instead of `thrown? InvalidTopologyException` the test does not pass but fails. Can you explain?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on a diff in the pull request:
https://github.com/apache/storm/pull/740#discussion_r39935812
--- Diff: storm-core/test/clj/backtype/storm/nimbus_test.clj ---
@@ -846,16 +846,6 @@
NIMBUS-SLOTS-PER-TOPOLOGY 8}]
(letlocals
(bind topology (thrift/mk-topology
- {"1" (thrift/mk-spout-spec (TestPlannerSpout. true) :parallelism-hint 0 :conf {TOPOLOGY-TASKS 1})}
--- End diff --
Removed this test because it now throws different exception and this is tested in the new unit test I added for `TopologyBuilder`. Actually, I tried to change the expected exception, but this did not fix this test for me (the test fails if I change `InvalidTopologyException` to `IllegalArgumentException`). Don't know why. But I am still new to Clojure and Clojure tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/740#discussion_r39936113
--- Diff: storm-core/test/clj/backtype/storm/nimbus_test.clj ---
@@ -846,16 +846,6 @@
NIMBUS-SLOTS-PER-TOPOLOGY 8}]
(letlocals
(bind topology (thrift/mk-topology
- {"1" (thrift/mk-spout-spec (TestPlannerSpout. true) :parallelism-hint 0 :conf {TOPOLOGY-TASKS 1})}
--- End diff --
@mjsax
thrift/mk-spout-spec throws IllegalArgumentException since it actually uses TopologyBuilder.
Removing this test makes sense since we cannot even try to submit topology with dop 0 now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/740
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1044] Setting dop to zero does not rais...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/740#discussion_r39935701
--- Diff: storm-core/test/clj/backtype/storm/nimbus_test.clj ---
@@ -846,16 +846,6 @@
NIMBUS-SLOTS-PER-TOPOLOGY 8}]
(letlocals
(bind topology (thrift/mk-topology
- {"1" (thrift/mk-spout-spec (TestPlannerSpout. true) :parallelism-hint 0 :conf {TOPOLOGY-TASKS 1})}
--- End diff --
@mjsax
Is it intended to remove this test?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---