You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by kishorvpatil <gi...@git.apache.org> on 2015/10/22 21:04:25 UTC
[GitHub] storm pull request: [STORM-1125] Adding separate ZK client for rea...
GitHub user kishorvpatil opened a pull request:
https://github.com/apache/storm/pull/813
[STORM-1125] Adding separate ZK client for read in Nimbus ZK State
Separate reader and writer for nimbus to avoid longer waits for reads to finish.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kishorvpatil/incubator-storm STORM-1125
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/813.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 #813
----
commit 0726ef00ec68f88ac6deccb6d3950c0d4805de6a
Author: Kishor Patil <kp...@yahoo-inc.com>
Date: 2015-10-22T18:51:53Z
Adding separate ZK client for read in Nimbus ZK State
----
---
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-1125] Adding separate ZK client for rea...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/813#issuecomment-150377545
Looks good overall.
I left some comments to make code cleaner. We can discuss / address these, and after addressing I'll vote.
---
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-1125] Adding separate ZK client for rea...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on a diff in the pull request:
https://github.com/apache/storm/pull/813#discussion_r42792906
--- Diff: storm-core/src/clj/backtype/storm/cluster.clj ---
@@ -89,68 +107,69 @@
(set-ephemeral-node
[this path data acls]
- (zk/mkdirs zk (parent-path path) acls)
- (if (zk/exists zk path false)
+ (zk/mkdirs zk-writer (parent-path path) acls)
+ (if (zk/exists zk-writer path false)
(try-cause
- (zk/set-data zk path data) ; should verify that it's ephemeral
+ (zk/set-data zk-writer path data) ; should verify that it's ephemeral
(catch KeeperException$NoNodeException e
(log-warn-error e "Ephemeral node disappeared between checking for existing and setting data")
- (zk/create-node zk path data :ephemeral acls)))
- (zk/create-node zk path data :ephemeral acls)))
+ (zk/create-node zk-writer path data :ephemeral acls)))
+ (zk/create-node zk-writer path data :ephemeral acls)))
(create-sequential
[this path data acls]
- (zk/create-node zk path data :sequential acls))
+ (zk/create-node zk-writer path data :sequential acls))
(set-data
[this path data acls]
;; note: this does not turn off any existing watches
- (if (zk/exists zk path false)
- (zk/set-data zk path data)
+ (if (zk/exists zk-writer path false)
+ (zk/set-data zk-writer path data)
(do
- (zk/mkdirs zk (parent-path path) acls)
- (zk/create-node zk path data :persistent acls))))
+ (zk/mkdirs zk-writer (parent-path path) acls)
+ (zk/create-node zk-writer path data :persistent acls))))
(delete-node
[this path]
- (zk/delete-node zk path))
+ (zk/delete-node zk-writer path))
(get-data
[this path watch?]
- (zk/get-data zk path watch?))
+ (zk/get-data zk-reader path watch?))
(get-data-with-version
[this path watch?]
- (zk/get-data-with-version zk path watch?))
+ (zk/get-data-with-version zk-reader path watch?))
(get-version
[this path watch?]
- (zk/get-version zk path watch?))
+ (zk/get-version zk-reader path watch?))
(get-children
[this path watch?]
- (zk/get-children zk path watch?))
+ (zk/get-children zk-reader path watch?))
(mkdirs
[this path acls]
- (zk/mkdirs zk path acls))
+ (zk/mkdirs zk-writer path acls))
(exists-node?
[this path watch?]
- (zk/exists-node? zk path watch?))
+ (zk/exists-node? zk-reader path watch?))
(close
[this]
(reset! active false)
- (.close zk))
+ (.close zk-writer)
+ (if (is-nimbus?) (.close zk-writer)))
--- End diff --
I think here it should be: (if (is-nimbus?) (.close zk-reader)))
---
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-1125] Adding separate ZK client for rea...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/813#discussion_r42814836
--- Diff: storm-core/src/clj/backtype/storm/cluster.clj ---
@@ -54,6 +54,10 @@
(when (Utils/isZkAuthenticationConfiguredTopology topo-conf)
[(first ZooDefs$Ids/CREATOR_ALL_ACL)
(ACL. ZooDefs$Perms/READ (Id. "digest" (DigestAuthenticationProvider/generateDigest payload)))])))
+
+(defn is-nimbus?
--- End diff --
How about removing this and let the caller of mk-distributed-cluster-state decides whether writer and reader should be separated? (I mean additional argument or multimethod.)
I think it can achieve loose coupling since ClusterState doesn't need to know current daemon is nimbus or not. It just wants to know whether writer and reader should be separated or not.
---
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-1125] Adding separate ZK client for rea...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/813#discussion_r42815363
--- Diff: storm-core/src/clj/backtype/storm/cluster.clj ---
@@ -89,68 +107,69 @@
(set-ephemeral-node
[this path data acls]
- (zk/mkdirs zk (parent-path path) acls)
- (if (zk/exists zk path false)
+ (zk/mkdirs zk-writer (parent-path path) acls)
+ (if (zk/exists zk-writer path false)
(try-cause
- (zk/set-data zk path data) ; should verify that it's ephemeral
+ (zk/set-data zk-writer path data) ; should verify that it's ephemeral
(catch KeeperException$NoNodeException e
(log-warn-error e "Ephemeral node disappeared between checking for existing and setting data")
- (zk/create-node zk path data :ephemeral acls)))
- (zk/create-node zk path data :ephemeral acls)))
+ (zk/create-node zk-writer path data :ephemeral acls)))
+ (zk/create-node zk-writer path data :ephemeral acls)))
(create-sequential
[this path data acls]
- (zk/create-node zk path data :sequential acls))
+ (zk/create-node zk-writer path data :sequential acls))
(set-data
[this path data acls]
;; note: this does not turn off any existing watches
- (if (zk/exists zk path false)
- (zk/set-data zk path data)
+ (if (zk/exists zk-writer path false)
+ (zk/set-data zk-writer path data)
(do
- (zk/mkdirs zk (parent-path path) acls)
- (zk/create-node zk path data :persistent acls))))
+ (zk/mkdirs zk-writer (parent-path path) acls)
+ (zk/create-node zk-writer path data :persistent acls))))
(delete-node
[this path]
- (zk/delete-node zk path))
+ (zk/delete-node zk-writer path))
(get-data
[this path watch?]
- (zk/get-data zk path watch?))
+ (zk/get-data zk-reader path watch?))
(get-data-with-version
[this path watch?]
- (zk/get-data-with-version zk path watch?))
+ (zk/get-data-with-version zk-reader path watch?))
(get-version
[this path watch?]
- (zk/get-version zk path watch?))
+ (zk/get-version zk-reader path watch?))
(get-children
[this path watch?]
- (zk/get-children zk path watch?))
+ (zk/get-children zk-reader path watch?))
(mkdirs
[this path acls]
- (zk/mkdirs zk path acls))
+ (zk/mkdirs zk-writer path acls))
(exists-node?
[this path watch?]
- (zk/exists-node? zk path watch?))
+ (zk/exists-node? zk-reader path watch?))
(close
[this]
(reset! active false)
- (.close zk))
+ (.close zk-writer)
+ (if (is-nimbus?) (.close zk-reader)))
--- End diff --
nit: If we can replace ```is-nimbus?``` to check object identity between zk-writer and zk-reader, it would be more readable.
---
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-1125] Adding separate ZK client for rea...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/813
---
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-1125] Adding separate ZK client for rea...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/813#discussion_r42795412
--- Diff: storm-core/src/clj/backtype/storm/cluster.clj ---
@@ -89,68 +107,69 @@
(set-ephemeral-node
[this path data acls]
- (zk/mkdirs zk (parent-path path) acls)
- (if (zk/exists zk path false)
+ (zk/mkdirs zk-writer (parent-path path) acls)
+ (if (zk/exists zk-writer path false)
(try-cause
- (zk/set-data zk path data) ; should verify that it's ephemeral
+ (zk/set-data zk-writer path data) ; should verify that it's ephemeral
(catch KeeperException$NoNodeException e
(log-warn-error e "Ephemeral node disappeared between checking for existing and setting data")
- (zk/create-node zk path data :ephemeral acls)))
- (zk/create-node zk path data :ephemeral acls)))
+ (zk/create-node zk-writer path data :ephemeral acls)))
+ (zk/create-node zk-writer path data :ephemeral acls)))
(create-sequential
[this path data acls]
- (zk/create-node zk path data :sequential acls))
+ (zk/create-node zk-writer path data :sequential acls))
(set-data
[this path data acls]
;; note: this does not turn off any existing watches
- (if (zk/exists zk path false)
- (zk/set-data zk path data)
+ (if (zk/exists zk-writer path false)
+ (zk/set-data zk-writer path data)
(do
- (zk/mkdirs zk (parent-path path) acls)
- (zk/create-node zk path data :persistent acls))))
+ (zk/mkdirs zk-writer (parent-path path) acls)
+ (zk/create-node zk-writer path data :persistent acls))))
(delete-node
[this path]
- (zk/delete-node zk path))
+ (zk/delete-node zk-writer path))
(get-data
[this path watch?]
- (zk/get-data zk path watch?))
+ (zk/get-data zk-reader path watch?))
(get-data-with-version
[this path watch?]
- (zk/get-data-with-version zk path watch?))
+ (zk/get-data-with-version zk-reader path watch?))
(get-version
[this path watch?]
- (zk/get-version zk path watch?))
+ (zk/get-version zk-reader path watch?))
(get-children
[this path watch?]
- (zk/get-children zk path watch?))
+ (zk/get-children zk-reader path watch?))
(mkdirs
[this path acls]
- (zk/mkdirs zk path acls))
+ (zk/mkdirs zk-writer path acls))
(exists-node?
[this path watch?]
- (zk/exists-node? zk path watch?))
+ (zk/exists-node? zk-reader path watch?))
(close
[this]
(reset! active false)
- (.close zk))
+ (.close zk-writer)
+ (if (is-nimbus?) (.close zk-writer)))
--- End diff --
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 pull request: [STORM-1125] Adding separate ZK client for rea...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/813#issuecomment-150699413
Nice, +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-1125] Adding separate ZK client for rea...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/813#issuecomment-150674032
@HeartSaVioR I have updated the the code to use flag instead of is-nimbus? method. Thank you for suggesting this loose-coupling. Please revisit the changes and comment.
---
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-1125] Adding separate ZK client for rea...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/813#discussion_r42795392
--- Diff: storm-core/src/clj/backtype/storm/cluster.clj ---
@@ -89,68 +107,69 @@
(set-ephemeral-node
[this path data acls]
- (zk/mkdirs zk (parent-path path) acls)
- (if (zk/exists zk path false)
+ (zk/mkdirs zk-writer (parent-path path) acls)
+ (if (zk/exists zk-writer path false)
(try-cause
- (zk/set-data zk path data) ; should verify that it's ephemeral
+ (zk/set-data zk-writer path data) ; should verify that it's ephemeral
(catch KeeperException$NoNodeException e
(log-warn-error e "Ephemeral node disappeared between checking for existing and setting data")
- (zk/create-node zk path data :ephemeral acls)))
- (zk/create-node zk path data :ephemeral acls)))
+ (zk/create-node zk-writer path data :ephemeral acls)))
+ (zk/create-node zk-writer path data :ephemeral acls)))
(create-sequential
[this path data acls]
- (zk/create-node zk path data :sequential acls))
+ (zk/create-node zk-writer path data :sequential acls))
(set-data
[this path data acls]
;; note: this does not turn off any existing watches
- (if (zk/exists zk path false)
- (zk/set-data zk path data)
+ (if (zk/exists zk-writer path false)
+ (zk/set-data zk-writer path data)
(do
- (zk/mkdirs zk (parent-path path) acls)
- (zk/create-node zk path data :persistent acls))))
+ (zk/mkdirs zk-writer (parent-path path) acls)
+ (zk/create-node zk-writer path data :persistent acls))))
(delete-node
[this path]
- (zk/delete-node zk path))
+ (zk/delete-node zk-writer path))
(get-data
[this path watch?]
- (zk/get-data zk path watch?))
+ (zk/get-data zk-reader path watch?))
(get-data-with-version
[this path watch?]
- (zk/get-data-with-version zk path watch?))
+ (zk/get-data-with-version zk-reader path watch?))
(get-version
[this path watch?]
- (zk/get-version zk path watch?))
+ (zk/get-version zk-reader path watch?))
(get-children
[this path watch?]
- (zk/get-children zk path watch?))
+ (zk/get-children zk-reader path watch?))
(mkdirs
[this path acls]
- (zk/mkdirs zk path acls))
+ (zk/mkdirs zk-writer path acls))
(exists-node?
[this path watch?]
- (zk/exists-node? zk path watch?))
+ (zk/exists-node? zk-reader path watch?))
(close
[this]
(reset! active false)
- (.close zk))
+ (.close zk-writer)
+ (if (is-nimbus?) (.close zk-writer)))
--- End diff --
good catch!
---
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-1125] Adding separate ZK client for rea...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/813#issuecomment-150702940
LGTM. +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.
---