You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by HeartSaVioR <gi...@git.apache.org> on 2017/01/25 09:49:07 UTC
[GitHub] storm pull request #1895: STORM-2323 Precondition for Leader Nimbus should c...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/1895
STORM-2323 Precondition for Leader Nimbus should check all topology blobs and also corresponding dependencies
* change the precondition for leader Nimbus
** it should have all active topology blobs locally
** it should also have all corresponding dependency blobs locally
** corresponding dependencies will be extracted from topology codes (via getBlob)
Since it tries to get blobs, probably leader listener can try to access other Nimbuses, and someone might think this is a bit dangerous.
I also have other branch which only checks all active topology blobs, not corresponding dependency blobs, so please feel free to share your opinions.
https://github.com/HeartSaVioR/storm/tree/STORM-2323-no-check-dependencies
I'll create pull request for 1.x when we select one of these branch and review and ready to merge.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-2323
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1895.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 #1895
----
commit c747a99b3344a7ede4da5898ed35a3d1e23b67fd
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2017-01-25T04:16:44Z
STORM-2323 Precondition for Leader Nimbus should check all topology blobs and also corresponding dependencies
* change the precondition for leader Nimbus
** it should have all active topology blobs and corresponding dependencies locally
----
---
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 #1895: STORM-2323 Precondition for Leader Nimbus should c...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1895#discussion_r97818183
--- Diff: storm-core/src/jvm/org/apache/storm/zookeeper/Zookeeper.java ---
@@ -336,29 +340,53 @@ public static NimbusInfo toNimbusInfo(Participant participant) {
public static LeaderLatchListener leaderLatchListenerImpl(final Map conf, final CuratorFramework zk, final BlobStore blobStore, final LeaderLatch leaderLatch) throws UnknownHostException {
final String hostName = InetAddress.getLocalHost().getCanonicalHostName();
return new LeaderLatchListener() {
+ final String STORM_JAR_SUFFIX = "-stormjar.jar";
+ final String STORM_CODE_SUFFIX = "-stormcode.ser";
+ final String STORM_CONF_SUFFIX = "-stormconf.ser";
+
@Override
public void isLeader() {
- Set<String> activeTopologyIds = new HashSet<>(Zookeeper.getChildren(zk, conf.get(Config.STORM_ZOOKEEPER_ROOT) + ClusterUtils.STORMS_SUBTREE, false));
- Set<String> localTopologyIds = blobStore.filterAndListKeys(new KeyFilter<String>() {
- @Override
- public String filter(String key) {
- return ConfigUtils.getIdFromBlobKey(key);
- }
- });
- Sets.SetView<String> diffTopology = Sets.difference(activeTopologyIds, localTopologyIds);
- LOG.info("active-topology-ids [{}] local-topology-ids [{}] diff-topology [{}]",
- generateJoinedString(activeTopologyIds), generateJoinedString(localTopologyIds),
+ Set<String> activeTopologyIds = new TreeSet<>(Zookeeper.getChildren(zk, conf.get(Config.STORM_ZOOKEEPER_ROOT) + ClusterUtils.STORMS_SUBTREE, false));
+
+ Set<String> activeTopologyBlobKeys = populateTopologyBlobKeys(activeTopologyIds);
+ Set<String> activeTopologyCodeKeys = filterTopologyCodeKeys(activeTopologyBlobKeys);
+ Set<String> allLocalBlobKeys = Sets.newHashSet(blobStore.listKeys());
+ Set<String> allLocalTopologyBlobKeys = filterTopologyBlobKeys(allLocalBlobKeys);
+
+ // this finds all active topologies blob keys from all local topology blob keys
+ Sets.SetView<String> diffTopology = Sets.difference(activeTopologyBlobKeys, allLocalTopologyBlobKeys);
+ LOG.info("active-topology-blobs [{}] local-topology-blobs [{}] diff-topology-blobs [{}]",
+ generateJoinedString(activeTopologyIds), generateJoinedString(allLocalTopologyBlobKeys),
generateJoinedString(diffTopology));
- if (diffTopology.isEmpty()) {
- LOG.info("Accepting leadership, all active topology found locally.");
- } else {
+ if (!diffTopology.isEmpty()) {
LOG.info("code for all active topologies not available locally, giving up leadership.");
try {
leaderLatch.close();
} catch (IOException e) {
throw new RuntimeException(e);
}
+
+ return;
--- End diff --
Also if the nimbus has incomplete set of topology resources, wouldn't it be better to cleanup the resources and let it sync from the newly elected leader.
---
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 #1895: STORM-2323 Precondition for Leader Nimbus should c...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1895#discussion_r97902409
--- Diff: storm-core/src/jvm/org/apache/storm/zookeeper/Zookeeper.java ---
@@ -336,29 +340,53 @@ public static NimbusInfo toNimbusInfo(Participant participant) {
public static LeaderLatchListener leaderLatchListenerImpl(final Map conf, final CuratorFramework zk, final BlobStore blobStore, final LeaderLatch leaderLatch) throws UnknownHostException {
final String hostName = InetAddress.getLocalHost().getCanonicalHostName();
return new LeaderLatchListener() {
+ final String STORM_JAR_SUFFIX = "-stormjar.jar";
+ final String STORM_CODE_SUFFIX = "-stormcode.ser";
+ final String STORM_CONF_SUFFIX = "-stormconf.ser";
+
@Override
public void isLeader() {
- Set<String> activeTopologyIds = new HashSet<>(Zookeeper.getChildren(zk, conf.get(Config.STORM_ZOOKEEPER_ROOT) + ClusterUtils.STORMS_SUBTREE, false));
- Set<String> localTopologyIds = blobStore.filterAndListKeys(new KeyFilter<String>() {
- @Override
- public String filter(String key) {
- return ConfigUtils.getIdFromBlobKey(key);
- }
- });
- Sets.SetView<String> diffTopology = Sets.difference(activeTopologyIds, localTopologyIds);
- LOG.info("active-topology-ids [{}] local-topology-ids [{}] diff-topology [{}]",
- generateJoinedString(activeTopologyIds), generateJoinedString(localTopologyIds),
+ Set<String> activeTopologyIds = new TreeSet<>(Zookeeper.getChildren(zk, conf.get(Config.STORM_ZOOKEEPER_ROOT) + ClusterUtils.STORMS_SUBTREE, false));
+
+ Set<String> activeTopologyBlobKeys = populateTopologyBlobKeys(activeTopologyIds);
+ Set<String> activeTopologyCodeKeys = filterTopologyCodeKeys(activeTopologyBlobKeys);
+ Set<String> allLocalBlobKeys = Sets.newHashSet(blobStore.listKeys());
+ Set<String> allLocalTopologyBlobKeys = filterTopologyBlobKeys(allLocalBlobKeys);
+
+ // this finds all active topologies blob keys from all local topology blob keys
+ Sets.SetView<String> diffTopology = Sets.difference(activeTopologyBlobKeys, allLocalTopologyBlobKeys);
+ LOG.info("active-topology-blobs [{}] local-topology-blobs [{}] diff-topology-blobs [{}]",
+ generateJoinedString(activeTopologyIds), generateJoinedString(allLocalTopologyBlobKeys),
generateJoinedString(diffTopology));
- if (diffTopology.isEmpty()) {
- LOG.info("Accepting leadership, all active topology found locally.");
- } else {
+ if (!diffTopology.isEmpty()) {
LOG.info("code for all active topologies not available locally, giving up leadership.");
try {
leaderLatch.close();
} catch (IOException e) {
throw new RuntimeException(e);
}
+
+ return;
--- End diff --
OK I'll try to remove `return` at this place.
Btw, synchronizing blobs are done for each, so I don't see benefit to clean up. If we're worried about inconsistency between versions of the topology blobs, I think it should be handled properly from BlobStore.
---
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 #1895: STORM-2323 Precondition for Leader Nimbus should check al...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1895
Thanks @harshach for reviewing. I also created PR (#1903) for 1.x (and possibly 1.0.x) branch.
---
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 #1895: STORM-2323 Precondition for Leader Nimbus should c...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1895#discussion_r97907878
--- Diff: storm-core/src/jvm/org/apache/storm/zookeeper/Zookeeper.java ---
@@ -336,29 +340,53 @@ public static NimbusInfo toNimbusInfo(Participant participant) {
public static LeaderLatchListener leaderLatchListenerImpl(final Map conf, final CuratorFramework zk, final BlobStore blobStore, final LeaderLatch leaderLatch) throws UnknownHostException {
final String hostName = InetAddress.getLocalHost().getCanonicalHostName();
return new LeaderLatchListener() {
+ final String STORM_JAR_SUFFIX = "-stormjar.jar";
+ final String STORM_CODE_SUFFIX = "-stormcode.ser";
+ final String STORM_CONF_SUFFIX = "-stormconf.ser";
+
@Override
public void isLeader() {
- Set<String> activeTopologyIds = new HashSet<>(Zookeeper.getChildren(zk, conf.get(Config.STORM_ZOOKEEPER_ROOT) + ClusterUtils.STORMS_SUBTREE, false));
- Set<String> localTopologyIds = blobStore.filterAndListKeys(new KeyFilter<String>() {
- @Override
- public String filter(String key) {
- return ConfigUtils.getIdFromBlobKey(key);
- }
- });
- Sets.SetView<String> diffTopology = Sets.difference(activeTopologyIds, localTopologyIds);
- LOG.info("active-topology-ids [{}] local-topology-ids [{}] diff-topology [{}]",
- generateJoinedString(activeTopologyIds), generateJoinedString(localTopologyIds),
+ Set<String> activeTopologyIds = new TreeSet<>(Zookeeper.getChildren(zk, conf.get(Config.STORM_ZOOKEEPER_ROOT) + ClusterUtils.STORMS_SUBTREE, false));
+
+ Set<String> activeTopologyBlobKeys = populateTopologyBlobKeys(activeTopologyIds);
+ Set<String> activeTopologyCodeKeys = filterTopologyCodeKeys(activeTopologyBlobKeys);
+ Set<String> allLocalBlobKeys = Sets.newHashSet(blobStore.listKeys());
+ Set<String> allLocalTopologyBlobKeys = filterTopologyBlobKeys(allLocalBlobKeys);
+
+ // this finds all active topologies blob keys from all local topology blob keys
+ Sets.SetView<String> diffTopology = Sets.difference(activeTopologyBlobKeys, allLocalTopologyBlobKeys);
+ LOG.info("active-topology-blobs [{}] local-topology-blobs [{}] diff-topology-blobs [{}]",
+ generateJoinedString(activeTopologyIds), generateJoinedString(allLocalTopologyBlobKeys),
generateJoinedString(diffTopology));
- if (diffTopology.isEmpty()) {
- LOG.info("Accepting leadership, all active topology found locally.");
- } else {
+ if (!diffTopology.isEmpty()) {
LOG.info("code for all active topologies not available locally, giving up leadership.");
try {
leaderLatch.close();
} catch (IOException e) {
throw new RuntimeException(e);
}
+
+ return;
--- End diff --
Addressed.
---
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 #1895: STORM-2323 Precondition for Leader Nimbus should c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1895
---
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 #1895: STORM-2323 Precondition for Leader Nimbus should c...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1895#discussion_r97818016
--- Diff: storm-core/src/jvm/org/apache/storm/zookeeper/Zookeeper.java ---
@@ -336,29 +340,53 @@ public static NimbusInfo toNimbusInfo(Participant participant) {
public static LeaderLatchListener leaderLatchListenerImpl(final Map conf, final CuratorFramework zk, final BlobStore blobStore, final LeaderLatch leaderLatch) throws UnknownHostException {
final String hostName = InetAddress.getLocalHost().getCanonicalHostName();
return new LeaderLatchListener() {
+ final String STORM_JAR_SUFFIX = "-stormjar.jar";
+ final String STORM_CODE_SUFFIX = "-stormcode.ser";
+ final String STORM_CONF_SUFFIX = "-stormconf.ser";
+
@Override
public void isLeader() {
- Set<String> activeTopologyIds = new HashSet<>(Zookeeper.getChildren(zk, conf.get(Config.STORM_ZOOKEEPER_ROOT) + ClusterUtils.STORMS_SUBTREE, false));
- Set<String> localTopologyIds = blobStore.filterAndListKeys(new KeyFilter<String>() {
- @Override
- public String filter(String key) {
- return ConfigUtils.getIdFromBlobKey(key);
- }
- });
- Sets.SetView<String> diffTopology = Sets.difference(activeTopologyIds, localTopologyIds);
- LOG.info("active-topology-ids [{}] local-topology-ids [{}] diff-topology [{}]",
- generateJoinedString(activeTopologyIds), generateJoinedString(localTopologyIds),
+ Set<String> activeTopologyIds = new TreeSet<>(Zookeeper.getChildren(zk, conf.get(Config.STORM_ZOOKEEPER_ROOT) + ClusterUtils.STORMS_SUBTREE, false));
+
+ Set<String> activeTopologyBlobKeys = populateTopologyBlobKeys(activeTopologyIds);
+ Set<String> activeTopologyCodeKeys = filterTopologyCodeKeys(activeTopologyBlobKeys);
+ Set<String> allLocalBlobKeys = Sets.newHashSet(blobStore.listKeys());
+ Set<String> allLocalTopologyBlobKeys = filterTopologyBlobKeys(allLocalBlobKeys);
+
+ // this finds all active topologies blob keys from all local topology blob keys
+ Sets.SetView<String> diffTopology = Sets.difference(activeTopologyBlobKeys, allLocalTopologyBlobKeys);
+ LOG.info("active-topology-blobs [{}] local-topology-blobs [{}] diff-topology-blobs [{}]",
+ generateJoinedString(activeTopologyIds), generateJoinedString(allLocalTopologyBlobKeys),
generateJoinedString(diffTopology));
- if (diffTopology.isEmpty()) {
- LOG.info("Accepting leadership, all active topology found locally.");
- } else {
+ if (!diffTopology.isEmpty()) {
LOG.info("code for all active topologies not available locally, giving up leadership.");
try {
leaderLatch.close();
} catch (IOException e) {
throw new RuntimeException(e);
}
+
+ return;
--- End diff --
instead of adding a return can't we make this as if else
---
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 #1895: STORM-2323 Precondition for Leader Nimbus should check al...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1895
+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.
---