You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by knusbaum <gi...@git.apache.org> on 2016/06/17 18:32:29 UTC
[GitHub] storm pull request #1500: STORM-1913: Additions and Improvements for Trident...
GitHub user knusbaum opened a pull request:
https://github.com/apache/storm/pull/1500
STORM-1913: Additions and Improvements for Trident RAS API
New behavior:
- Added a way to specify default values for operations in Trident
- New behavior requires all components in group have same types of resources set (or defaults)
- Added a way to specify resources for master-coord Spouts.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/knusbaum/incubator-storm STORM-1913
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1500.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 #1500
----
commit 50444bc15da839f0f88d94bcbd160601a6bdb373
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-17T17:59:06Z
Working on cherry-pick
commit e197b0183c6a364b09f4d1b4c13694a4a6af1246
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-14T18:53:26Z
Almost there.
commit 526178924e722bc9390c9453225a7811f1aa6e2e
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-17T18:10:04Z
Doing pick.
commit 05b4aff8951265e5cfc3f58a7431db1a702aeddc
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-17T18:11:02Z
Removing intermediate files.
commit 82e67600fc0cfb9c2110816508f4971e2e3f15ab
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-17T18:26:16Z
Fixing stuff.
----
---
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 issue #1500: STORM-1913: Additions and Improvements for Trident RAS AP...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/1500
+1 looks good to me.
@HeartSaVioR any other comments?
---
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 #1500: STORM-1913: Additions and Improvements for Trident...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1500#discussion_r67725802
--- Diff: storm-core/src/jvm/org/apache/storm/topology/BaseConfigurationDeclarer.java ---
@@ -56,7 +56,11 @@ public T setNumTasks(Number val) {
@Override
public T setMemoryLoad(Number onHeap) {
- return setMemoryLoad(onHeap, Utils.getDouble(conf.get(Config.TOPOLOGY_COMPONENT_RESOURCES_OFFHEAP_MEMORY_MB)));
+ if (onHeap != null) {
+ onHeap = onHeap.doubleValue();
+ return addConfiguration(Config.TOPOLOGY_COMPONENT_RESOURCES_ONHEAP_MEMORY_MB, onHeap);
+ }
+ return null;
--- End diff --
Maybe it would make more sense for `setMemoryLoad(Number onHeap, Number offHeap)` to call into this one instead.
---
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 issue #1500: STORM-1913: Additions and Improvements for Trident RAS AP...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1500
@knusbaum Please rebase and squash the commits before merging. It would be better to have meaningful log message.
---
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 issue #1500: STORM-1913: Additions and Improvements for Trident RAS AP...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/1500
OK, looks like one of the tests is failing a check for sane resource assignments per-stream.
---
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 #1500: STORM-1913: Additions and Improvements for Trident...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1500#discussion_r69755010
--- Diff: storm-core/src/jvm/org/apache/storm/topology/BaseConfigurationDeclarer.java ---
@@ -56,7 +56,11 @@ public T setNumTasks(Number val) {
@Override
public T setMemoryLoad(Number onHeap) {
- return setMemoryLoad(onHeap, Utils.getDouble(conf.get(Config.TOPOLOGY_COMPONENT_RESOURCES_OFFHEAP_MEMORY_MB)));
+ if (onHeap != null) {
+ onHeap = onHeap.doubleValue();
+ return addConfiguration(Config.TOPOLOGY_COMPONENT_RESOURCES_ONHEAP_MEMORY_MB, onHeap);
+ }
+ return null;
--- End diff --
@HeartSaVioR fixed.
---
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 issue #1500: STORM-1913: Additions and Improvements for Trident RAS AP...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/1500
Overall it looks OK, but it seems there are some issues with the build.
---
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 #1500: STORM-1913: Additions and Improvements for Trident...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1500
---
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 #1500: STORM-1913: Additions and Improvements for Trident...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1500#discussion_r67821953
--- Diff: storm-core/src/jvm/org/apache/storm/topology/BaseConfigurationDeclarer.java ---
@@ -56,7 +56,11 @@ public T setNumTasks(Number val) {
@Override
public T setMemoryLoad(Number onHeap) {
- return setMemoryLoad(onHeap, Utils.getDouble(conf.get(Config.TOPOLOGY_COMPONENT_RESOURCES_OFFHEAP_MEMORY_MB)));
+ if (onHeap != null) {
+ onHeap = onHeap.doubleValue();
+ return addConfiguration(Config.TOPOLOGY_COMPONENT_RESOURCES_ONHEAP_MEMORY_MB, onHeap);
+ }
+ return null;
--- End diff --
Yes that might be better.
---
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 issue #1500: STORM-1913: Additions and Improvements for Trident RAS AP...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1500
@d2r Nope, since I don't have background on RAS implementation yet. Please go ahead.
---
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 #1500: STORM-1913: Additions and Improvements for Trident...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1500#discussion_r67634689
--- Diff: storm-core/src/jvm/org/apache/storm/topology/BaseConfigurationDeclarer.java ---
@@ -56,7 +56,11 @@ public T setNumTasks(Number val) {
@Override
public T setMemoryLoad(Number onHeap) {
- return setMemoryLoad(onHeap, Utils.getDouble(conf.get(Config.TOPOLOGY_COMPONENT_RESOURCES_OFFHEAP_MEMORY_MB)));
+ if (onHeap != null) {
+ onHeap = onHeap.doubleValue();
+ return addConfiguration(Config.TOPOLOGY_COMPONENT_RESOURCES_ONHEAP_MEMORY_MB, onHeap);
+ }
+ return null;
--- End diff --
Minor: This can be replaced with `setMemoryLoad(onHeap, null)` but I'm OK to split two methods completely.
---
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.
---