You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Parth-Brahmbhatt <gi...@git.apache.org> on 2015/02/03 20:56:32 UTC
[GitHub] storm pull request: STORM-634: Storm serialization changed to thri...
GitHub user Parth-Brahmbhatt opened a pull request:
https://github.com/apache/storm/pull/414
STORM-634: Storm serialization changed to thrift to support rolling upgrade.
Converting SupervisorInfo,Assignment,StormBase,TopologyStatus,ZKWorkerHeartbeat,ErrorInfo,Credentials to thrift and defaulting the serialization delegate to thrift serialization. Added class as a param to serialization delegate interface.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Parth-Brahmbhatt/incubator-storm STORM-634
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/414.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 #414
----
commit 639006432c658226bd33dc2ae607f121f3dc02bb
Author: Parth Brahmbhatt <br...@gmail.com>
Date: 2015-01-29T21:46:01Z
STORM-634: Converting SupervisorInfo,Assignment,StormBase,TopologyStatus,ZKWorkerHeartbeat,ErrorInfo,Credentials to thrift and defaulting the serialization delegate to thrift serialization. Added class as a param to serialization delegate interface.
----
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26234849
--- Diff: storm-core/src/jvm/backtype/storm/serialization/DefaultSerializationDelegate.java ---
@@ -17,11 +17,7 @@
*/
package backtype.storm.serialization;
--- End diff --
This is really minor, but it might also be nice to deprecate this, and migrate the core deserialization over to Utils, so we are not calling a deprecated class.
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26229576
--- Diff: storm-core/src/clj/backtype/storm/converter.clj ---
@@ -0,0 +1,200 @@
+(ns backtype.storm.converter
--- End diff --
I think it is OK for 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-634: Storm serialization changed to thri...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/414
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24513164
--- Diff: storm-core/src/jvm/backtype/storm/serialization/ThriftSerializationDelegate.java ---
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package backtype.storm.serialization;
+
+import org.apache.thrift.TBase;
+import org.apache.thrift.TDeserializer;
+import org.apache.thrift.TException;
+import org.apache.thrift.TSerializer;
+
+import java.util.Map;
+
+public class ThriftSerializationDelegate implements SerializationDelegate {
--- End diff --
@Parth-Brahmbhatt Sorry, I was not more specific. You are completely correct in your assumption. The serialized Bolt, Spout, and custom grouping instances placed as byte arrays in the thrift datastructure should always be serialized/deserialized using java serialization, and we should not bother sending them through the serialization delegate. Java serialization is part of the contract that we setup with developers.
---
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-634: Storm serialization changed to thri...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/414#issuecomment-78618488
@revans2 I agree that it would be great to test forward/backward compatibility, but testing that in an automated way would be really difficult since it would likely require the introduction of what in previous versions of storm would have been a breaking change (clojure structures, etc.). I think older client/newer server compatibility is acceptable.
I think this is a big step toward better supporting upgrades, rolling upgrades, and continuity of operations for Storm deployments. Nice work @Parth-Brahmbhatt, and @revans2 for the thorough review.
+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 issue #414: STORM-634: Storm serialization changed to thrift to suppor...
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on the issue:
https://github.com/apache/storm/pull/414
@revans2 ,hi\uff0ci really want to know how to upgrade storm for our cluster now, there are 1500+ topologies now on our cluster, and we backup our existing topologies , kill all of them , perform the upgrade and resubmit all the topologies every time. It's really awful and not suitable for long running tasks, so appreciate your suggestions very mush
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/414#issuecomment-78313719
For the most part it looks good. I had a few minor comments. My only real concern is about maintaining backwards compatibility for clients. I didn't see much that would prevent this, but I would feel more comfortable if it were tested.
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26230152
--- Diff: storm-core/src/jvm/backtype/storm/serialization/Test.java ---
@@ -0,0 +1,17 @@
+package backtype.storm.serialization;
--- End diff --
This file appears to have been added by mistake, if not it should be turned into a true unit test, with apache header etc.
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/414#issuecomment-73543546
I have two big questions/issues with the current patch. I think it is a step in the right direction.
---
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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26250794
--- Diff: storm-core/src/storm.thrift ---
@@ -243,6 +244,55 @@ struct SubmitOptions {
2: optional Credentials creds;
}
+struct SupervisorInfo {
+ 1: required i64 time_secs;
+ 2: required string hostname;
+ 3: optional string assignment_id;
+ 4: optional list<i64> used_ports;
+ 5: optional list<i64> meta;
+ 6: optional map<string, string> scheduler_meta;
+ 7: optional i64 uptime_secs;
+}
+struct NodeInfo {
+ 1: required string node;
+ 2: required set<i64> port;
+}
+
+struct Assignment {
+ 1: required string master_code_dir;
+ 2: optional map<string, string> node_host = {};
+ 3: optional map<list<i64>, NodeInfo> executor_node_port = {};
+ 4: optional map<list<i64>, i64> executor_start_time_secs = {};
+}
+
+enum TopologyStatus {
+ ACTIVE = 1,
+ INACTIVE = 2,
+ REBALANCING = 3,
+ KILLED = 4
+}
+
+union TopologyActionOptions {
+ 1: optional KillOptions kill_options;
+ 2: optional RebalanceOptions rebalance_options;
+}
+
+struct StormBase {
+ 1: required string name;
+ 2: required TopologyStatus status;
+ 3: required i32 num_workers;
+ 4: optional map<string, i32> component_executors;
+ 5: optional i32 launch_time_secs;
+ 6: optional string owner;
+ 7: optional TopologyActionOptions topology_action_options;
+ 8: optional TopologyStatus prev_status;//currently only used during rebalance action.
+}
+
+struct ZKWorkerHeartbeat {
--- End diff --
I agree this is a bad naming choice. Updated to ClusterWorkerHeartbeat.
---
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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/414#issuecomment-77511548
@revans2 @harshach @ptgoetz upmerged again, re-generated all files using thrift- 0.9.2. Appreciate any feedback you guys can provide.
---
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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/414#issuecomment-78574474
@revans2 All comments addressed and I really appreciate the time you are taking to review this.
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24351697
--- Diff: storm-core/src/jvm/backtype/storm/serialization/ThriftSerializationDelegate.java ---
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package backtype.storm.serialization;
+
+import org.apache.thrift.TBase;
+import org.apache.thrift.TDeserializer;
+import org.apache.thrift.TException;
+import org.apache.thrift.TSerializer;
+
+import java.util.Map;
+
+public class ThriftSerializationDelegate implements SerializationDelegate {
--- End diff --
I would rather see the topology serialization moved out to not use the delegate at all, but instead to just use java serialization, or to be configured separately. There is no real reason to try and support thrift or other types of serialization for a topology.
---
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-634: Storm serialization changed to thri...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24358543
--- Diff: storm-core/src/clj/backtype/storm/converter.clj ---
@@ -0,0 +1,200 @@
+(ns backtype.storm.converter
--- End diff --
> Each function also makes certain assumptions like a map is always a clojure map and thus its iterable, or some list is always sorted or the first element of some list is a string and the second is integer. You can see these assumptions being fulfilled by the converter right now
Should this be documented explicitly?
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24351821
--- Diff: storm-core/src/clj/backtype/storm/converter.clj ---
@@ -0,0 +1,200 @@
+(ns backtype.storm.converter
--- End diff --
That seems fine to me. Lets start with this, and then we can do follow on work later.
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24342289
--- Diff: storm-core/src/jvm/backtype/storm/serialization/ThriftSerializationDelegate.java ---
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package backtype.storm.serialization;
+
+import org.apache.thrift.TBase;
+import org.apache.thrift.TDeserializer;
+import org.apache.thrift.TException;
+import org.apache.thrift.TSerializer;
+
+import java.util.Map;
+
+public class ThriftSerializationDelegate implements SerializationDelegate {
--- End diff --
I don't believe that this will ever work Standalone. I don't see how we could ever make a custom grouping that is a subclasses of TBase, and even if we could the code in ThriftSerializationDelegateBridge would break, because we would write it out using thrift, but try to read it back in using java serialization. I am not really sure the best way to handle this though. The only real thing that comes to mind would be to have different serialization delegates for different places we store data. One would be for the topology instances, which we could leave using java serialization because it is part of the contract we made in the API for bolts, spouts, and groupings. The other would be for everything 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 pull request: STORM-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/414#issuecomment-83134133
Sorry again it took me so long to respond. +1 It looks good and I am very happy to see this go 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: STORM-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24459097
--- Diff: storm-core/src/jvm/backtype/storm/serialization/ThriftSerializationDelegate.java ---
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package backtype.storm.serialization;
+
+import org.apache.thrift.TBase;
+import org.apache.thrift.TDeserializer;
+import org.apache.thrift.TException;
+import org.apache.thrift.TSerializer;
+
+import java.util.Map;
+
+public class ThriftSerializationDelegate implements SerializationDelegate {
--- End diff --
@revans2 I am sorry I am not sure what you mean when you say "topology serialization moved out". StormTopology is a thrift structure so I think we can use thriftSerializationDelegate. The Spouts and Bolts have Component structures which are just serialized java bytes. Do you mean every place where we are just calling "get_serialized_java" on thrift instances should use java serialization? I think that makes sense and I can make that change. Let me know if I misunderstood your 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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24342731
--- Diff: storm-core/src/clj/backtype/storm/converter.clj ---
@@ -0,0 +1,200 @@
+(ns backtype.storm.converter
--- End diff --
I personally would rather see the code use the Thrift objects directly. One of the reasons I wanted to have Thrift over just using maps, was that it would force a developer to think about any changes that make. To me having this here means that someone will add something to Assignment for example and not update this. Then it will work just fine in local mode testing, but will not work in a real cluster, and would be a pain to debug.
---
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 #414: STORM-634: Storm serialization changed to thrift to suppor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/414
@danny0405 like @HeartSaVioR said it depends on the versions you are upgrading between. Most of the time we have maintained wire and binary compatibility so you can do the upgrade piecemeal. This should work between versions of storm that have the same major version number. 1.0.0 to 1.1.0, or 1.1.0 to 1.1.2, but not 0.10.x to 1.0.0.
The procedure that we follow when doing an upgrade is to
1) shutdown and upgrade nimbus (we are not currently running HA, but if we were step 1.b would be to upgrade the other nimbus instances one at a time)
2) pick a single node that is not upgraded yet.
2.b) install the new version of storm on the node.
2.c) shoot all the storm processes, supervisor, logviewer, and workers
2.d) clear out all of the state on the node (NOT needed every time, but we are cautious because of bugs in the past)
2.e) relaunch the supervisor and logviewer.
3) repeat until all of the nodes are done.
For our large clusters we actually do a few nodes at a time, not one. This procedure does have a few issues. Primarily the biggest issue is churn in the worker processes. We try to avoid doing the upgrade a lot because it is not truly transparent to all topologies. They recover, but they have had every one of their worker processes shot at least one, and possibly multiple times. This can cause data issues in non-trident topologies, and can slow down the processing in trident.
I would recommend that you do it a little differently, and this is what we want to move to.
for each node in parallel as much as possible install the new version of storm then shoot the supervisor and the logviewer. Wait for them to all come back up, or at least enough that you feel good about it.
Then again as parallel as possible shoot all of the worker processes on all of the nodes.
This still has the disadvantage of having all of the worker processes being shot and slowing things down, but they are guaranteed to only be shot once, and the recovery time should be much faster. The supervisor relaunches them quickly instead of possibly having nimbus time them out and reschedule them on a node that has not been upgraded yet.
---
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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26334368
--- Diff: storm-core/src/jvm/backtype/storm/serialization/DefaultSerializationDelegate.java ---
@@ -17,11 +17,7 @@
*/
package backtype.storm.serialization;
--- End diff --
Done. As part of this I also deleted ThriftBridgeSerializer which depended on the now deprecated DefaultSerializationDelegate. So we now using ThriftSerializationDelegate as the default.
---
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 #414: STORM-634: Storm serialization changed to thrift to suppor...
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on the issue:
https://github.com/apache/storm/pull/414
@revans2 @HeartSaVioR Actually, i have an idea, but i'm not sure if it will work correctly. Mainly because the compatibility of Supervisor LocalState and Zookeeper metadata between different version of storm. Also because supervisors are not start up at the same time, there is a time old and new storm code worker working together of one topology. So, is there any problem?
1. when a supervisor first start up, write its version to LocalState
2. supervisor check if the LS version is same with current code version
3. if it's same do nothing, if it's different, kill all the workers on this node
4. sync worker processes from LocalState and ZooKeeper.
---
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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24351380
--- Diff: storm-core/src/clj/backtype/storm/converter.clj ---
@@ -0,0 +1,200 @@
+(ns backtype.storm.converter
--- End diff --
I actually have a version that attempts to use thrift object directly but in process of doing that I realized I am just writing the converters at several places or I am writing a custom getter method for thrift objects. The code does not use these objects but uses the fields inside these objects and passes the value of the field around different functions. Each function also makes certain assumptions like a map is always a clojure map and thus its iterable, or some list is always sorted or the first element of some list is a string and the second is integer. You can see these assumptions being fulfilled by the converter right now but in absence of it I had to modify a lot of code all over the code base. So I decided to focus on just making rolling upgrade possible first and nailing down the correct thrift structures which it self seemed like a big enough win.
Here is the code that attempts to do what you are suggesting, its not complete but kind of gives you an idea why it might not be a good idea to undertake that task as part of this PR https://github.com/Parth-Brahmbhatt/incubator-storm/compare/thrift-structures?expand=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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/414#issuecomment-74728302
@revans2 I have changed the code to use java serialization , no delegate for all places where we expect only java serialization. I still have both the ThriftDelegate(which should now work in standalone mode) and ThrftDelegateBridge but I don't have a strong preference to keep both of them around.
---
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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r24350530
--- Diff: storm-core/src/jvm/backtype/storm/serialization/ThriftSerializationDelegate.java ---
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package backtype.storm.serialization;
+
+import org.apache.thrift.TBase;
+import org.apache.thrift.TDeserializer;
+import org.apache.thrift.TException;
+import org.apache.thrift.TSerializer;
+
+import java.util.Map;
+
+public class ThriftSerializationDelegate implements SerializationDelegate {
--- End diff --
You are right, it will never work in stand alone mode and that is why I defaulted the config to the bridge implementation. If you think this is going to be confusing I can delete this file and move the code to the Bridge implementation it self.
I don't think making changes to support this stand alone is justified.
---
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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26250717
--- Diff: storm-core/src/jvm/backtype/storm/serialization/Test.java ---
@@ -0,0 +1,17 @@
+package backtype.storm.serialization;
--- End diff --
yes, was testing if thrift support serialization of Null, it does not. Removed.
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26230947
--- Diff: storm-core/src/storm.thrift ---
@@ -193,6 +193,7 @@ struct ExecutorStats {
1: required map<string, map<string, i64>> emitted;
2: required map<string, map<string, i64>> transferred;
3: required ExecutorSpecificStats specific;
+ 4: required double rate;
--- End diff --
Having this be required technically breaks compatibility. I'm not sure if this is a problem or not. ExecutorStats is exposed to the end user, but really only as a consumer. So it makes it impossible for a newer client to interact with an older nimbus instance. I think this is OK, but we should be very careful about these kinds of things going forward.
Have you tested using older clients with a newer nimbus instance?
---
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-634: Storm serialization changed to thri...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26251585
--- Diff: storm-core/src/storm.thrift ---
@@ -193,6 +193,7 @@ struct ExecutorStats {
1: required map<string, map<string, i64>> emitted;
2: required map<string, map<string, i64>> transferred;
3: required ExecutorSpecificStats specific;
+ 4: required double rate;
--- End diff --
Older client newer server works, newer client with older server brakes because rate is required behavior. I think this is acceptable and would hate to change a required param to optional to support this.
---
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 #414: STORM-634: Storm serialization changed to thrift to suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/414
Hi @danny0405, could you describe which version do you use and which version do you want to upgrade?
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/414#discussion_r26231202
--- Diff: storm-core/src/storm.thrift ---
@@ -243,6 +244,55 @@ struct SubmitOptions {
2: optional Credentials creds;
}
+struct SupervisorInfo {
+ 1: required i64 time_secs;
+ 2: required string hostname;
+ 3: optional string assignment_id;
+ 4: optional list<i64> used_ports;
+ 5: optional list<i64> meta;
+ 6: optional map<string, string> scheduler_meta;
+ 7: optional i64 uptime_secs;
+}
+struct NodeInfo {
+ 1: required string node;
+ 2: required set<i64> port;
+}
+
+struct Assignment {
+ 1: required string master_code_dir;
+ 2: optional map<string, string> node_host = {};
+ 3: optional map<list<i64>, NodeInfo> executor_node_port = {};
+ 4: optional map<list<i64>, i64> executor_start_time_secs = {};
+}
+
+enum TopologyStatus {
+ ACTIVE = 1,
+ INACTIVE = 2,
+ REBALANCING = 3,
+ KILLED = 4
+}
+
+union TopologyActionOptions {
+ 1: optional KillOptions kill_options;
+ 2: optional RebalanceOptions rebalance_options;
+}
+
+struct StormBase {
+ 1: required string name;
+ 2: required TopologyStatus status;
+ 3: required i32 num_workers;
+ 4: optional map<string, i32> component_executors;
+ 5: optional i32 launch_time_secs;
+ 6: optional string owner;
+ 7: optional TopologyActionOptions topology_action_options;
+ 8: optional TopologyStatus prev_status;//currently only used during rebalance action.
+}
+
+struct ZKWorkerHeartbeat {
--- End diff --
I'd rather that this is not called ZKWorkerHeartbeat. ZK is a storage solution that could change in the future, I'd rather call it a ClusterWorkerHeartbeat or something that describes the abstraction rather than the storage location.
---
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-634: Storm serialization changed to thri...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/414#issuecomment-73544378
The code also needs an upmerge.
---
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 #414: STORM-634: Storm serialization changed to thrift to suppor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/414
@danny0405 That is very similar to what we want to move towards in our rolling upgrades. We have done a very good job of maintaining compatibility of LS and ZK once we moved to thrift for all of them. There were a few hiccups at the beginning, but none of those were officially released. Because of feature work we do/etc we typically don't run "official" releases of apache storm, but code that is based off of an official release + a bunch of other stuff. This is what bit us once as we were working on adding in support for rolling upgrades and had to start clearing the local state on the node. This should not be a problem, especially if you stick with official releases.
---
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.
---