You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by BuDongDong <gi...@git.apache.org> on 2014/11/13 06:34:52 UTC
[GitHub] storm pull request: Update "get-task-object" function, change the ...
GitHub user BuDongDong opened a pull request:
https://github.com/apache/storm/pull/312
Update "get-task-object" function, change the type of first param "topology" from ^TopologyContext to ^StormTopology
update "get-task-object" function, change the type of first param "topology" from ^TopologyContext to ^StormTopology
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/BuDongDong/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/312.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 #312
----
commit 98a1c73d3a090635772131bccf621d384158c36c
Author: zhangjinlong <zh...@126.com>
Date: 2014-11-13T04:02:10Z
Update task.clj
update "get-task-object" function, change the type of first param "topology" from ^TopologyContext to ^StormTopology
----
---
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: Update "get-task-object" function, change the ...
Posted by BuDongDong <gi...@git.apache.org>.
Github user BuDongDong commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-63206771
@harshach the "get-task-object" is called by "mk-task-data" function in task.clj, "mk-task-data" is defined as following:
(defn mk-task-data [executor-data task-id]
(recursive-map
:executor-data executor-data
:task-id task-id
:system-context (system-topology-context (:worker executor-data) executor-data task-id)
:user-context (user-topology-context (:worker executor-data) executor-data task-id)
:builtin-metrics (builtin-metrics/make-data (:type executor-data))
:tasks-fn (mk-tasks-fn <>)
:object (get-task-object (.getRawTopology ^TopologyContext (:system-context <>)) (:component-id executor-data))))
(:system-context <>) return TopologyContext instance, TopologyContext extends GeneralTopologyContext, the TopologyContext instance has StormTopology _topology. “getRawTopology” method of TopologyContext must return StormTopology _topology. (.getRawTopology ^TopologyContext (:system-context <>) return StormTopology instance not TopologyContext instance. so the type of param topology is StormTopology not TopologyContext.
at the same time, "get-task-object" call "get_spouts" and "get_bolts" of the param topology. you can find the "get_spouts" and "get_bolts" function are only defined in StormTopology, and StormTopology is not a subclass of TopologyContext. so i think the type of param topology is StormTopology not TopologyContext.
---
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: Update "get-task-object" function, change the ...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/312#discussion_r25610339
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -67,7 +67,7 @@
(defrecord RollingWindowSet [updater extractor windows all-time])
(defn rolling-window-set [updater merger extractor num-buckets & bucket-sizes]
- (RollingWindowSet. updater extractor (dofor [s bucket-sizes] (rolling-window updater merger extractor s num-buckets)) nil)
+ (RollingWindowSet. updater extractor (dofor [s bucket-sizes] (rolling-window updater merger extractor num-buckets s)) nil)
--- End diff --
This breaks the call to `rolling-window`. Please revert.
---
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: Update "get-task-object" function, change the ...
Posted by BuDongDong <gi...@git.apache.org>.
Github user BuDongDong commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-74463198
@revans2 I create a new repository, and pull this request again. see https://github.com/apache/storm/pull/433.
I have filed a JIRA, https://issues.apache.org/jira/browse/STORM-554.
---
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: Update "get-task-object" function, change the ...
Posted by BuDongDong <gi...@git.apache.org>.
Github user BuDongDong closed the pull request at:
https://github.com/apache/storm/pull/312
---
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: Update "get-task-object" function, change the ...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-63085407
@BuDongDong can you please file a JIRA. Thanks
---
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: Update "get-task-object" function, change the ...
Posted by BuDongDong <gi...@git.apache.org>.
Github user BuDongDong commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-63165315
@harshach I have filed a JIRA, https://issues.apache.org/jira/browse/STORM-554.
---
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: Update "get-task-object" function, change the ...
Posted by xiaokang <gi...@git.apache.org>.
Github user xiaokang commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-63223579
@BuDongDong , your explanation sounds reasonable. But the following clojure REPL shows that type check will raise error if the real argument does not match type declaration.
user=> (defn tostring [^String obj] (.toString obj))
#'user/tostring
user=> (tostring "123")
"123"
user=> (tostring 123)
ClassCastException java.lang.Long cannot be cast to java.lang.String user/tostring (NO_SOURCE_FILE:1)
user=> (tostring (Object.))
ClassCastException java.lang.Object cannot be cast to java.lang.String user/tostring (NO_SOURCE_FILE: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: Update "get-task-object" function, change the ...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/312#discussion_r25610832
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -67,7 +67,7 @@
(defrecord RollingWindowSet [updater extractor windows all-time])
(defn rolling-window-set [updater merger extractor num-buckets & bucket-sizes]
- (RollingWindowSet. updater extractor (dofor [s bucket-sizes] (rolling-window updater merger extractor s num-buckets)) nil)
+ (RollingWindowSet. updater extractor (dofor [s bucket-sizes] (rolling-window updater merger extractor num-buckets s)) nil)
--- End diff --
Oh, never mind, I should be looking at #433.
@BuDongDong , thanks for the update. Would you close this pull request?
---
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: Update "get-task-object" function, change the ...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-63176742
@BuDongDong can you explain why this needs to be changed to StormTopology. I think use TopologyContext is fine .
---
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: Update "get-task-object" function, change the ...
Posted by BuDongDong <gi...@git.apache.org>.
Github user BuDongDong commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-63250946
@xiaokang the type of real argument is "StormTopology", however the "get-task-object" function use a error type declaration "TopologyContext", clojure REPL will not raise error.
you can see this example:
; SLIME 20100404
user> (defn get_date_time [^String date] (.getTime date))
#'user/get_date_time
user> (def today (java.util.Date.))
#'user/today
user> (get_date_time today)
1416187016658
user> (def ss (String. "today"))
#'user/ss
user> (get_date_time ss)
the "get_date_time" function use a error type declaration "String" to param date, however, there is no “getTime” method in class “String”. in fact, the "getTime" method is defined by "java.util.Date". (get_date_time today), clojure REPL will not raise error; but (get_date_time ss), clojure REPL must raise error "No matching field found: getTime for class java.lang.String". the question of "get-task-object" function is same as "get_date_time".
because the type of real argument is "StormTopology", "get-task-object" calls "get_spouts" and "get_bolts" not raise error, but, if the type of real argument is "TopologyContext", "get-task-object" calls "get_spouts" and "get_bolts" must raise error. class TopologyContext do not have "get_spouts" and "get_bolts" method.
in such a situation, clojure REPL type check will not raise error.
---
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: Update "get-task-object" function, change the ...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-73540020
@BuDongDong any update? Should we get a new pull request for this, or can we fix the Unknown Repository Problem another way?
---
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: Update "get-task-object" function, change the ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-69380814
@BuDongDong the changes you made look good to me. Would it be possible to get you to file a JIRA for them? This is mostly for tracking to make it simpler to see what has gone in.
Also I am not totally sure why, but it looks like your repository for this pull was deleted or something. The Repository is showing up as Unknown. Without a repo name/branch I can apply a patch, but the merge may not register appropriately when I check it in.
---
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: Update "get-task-object" function, change the ...
Posted by BuDongDong <gi...@git.apache.org>.
Github user BuDongDong commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-62844422
(defn- get-task-object [^TopologyContext topology component-id] ... ... )
i think the type of param topology is StormTopology not TopologyContext
---
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: Update "get-task-object" function, change the ...
Posted by BuDongDong <gi...@git.apache.org>.
Github user BuDongDong commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-76868454
@d2r ok, i have closed this pull request.
---
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: Update "get-task-object" function, change the ...
Posted by BuDongDong <gi...@git.apache.org>.
Github user BuDongDong commented on the pull request:
https://github.com/apache/storm/pull/312#issuecomment-76647927
@d2r I create a new repository, and pull this request again. see #433.
I have filed a JIRA, https://issues.apache.org/jira/browse/STORM-554.
you can merge #433.
---
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.
---