You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by satishd <gi...@git.apache.org> on 2016/03/29 07:38:21 UTC
[GitHub] storm pull request: STORM-1649 kryo serialization in windowing
GitHub user satishd opened a pull request:
https://github.com/apache/storm/pull/1273
STORM-1649 kryo serialization in windowing
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/satishd/storm STORM-1649
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1273.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 #1273
----
commit 576499baba14926a564a51e0018f3ead86809ab1
Author: Satish Duggana <sd...@hortonworks.com>
Date: 2016-03-29T04:32:28Z
STORM-1649 kryo serialization in windowing
----
---
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-1649 Optimize Kryo instaces creation in ...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1273#discussion_r58004089
--- Diff: storm-core/src/jvm/org/apache/storm/trident/windowing/WindowKryoSerializer.java ---
@@ -0,0 +1,87 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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 org.apache.storm.trident.windowing;
+
+import com.esotericsoftware.kryo.Kryo;
+import com.esotericsoftware.kryo.io.Input;
+import com.esotericsoftware.kryo.io.Output;
+import org.apache.storm.serialization.SerializationFactory;
+
+import java.nio.ByteBuffer;
+import java.util.Map;
+
+/**
+ * Kryo serializer/deserializer for values that are stored as part of windowing. This can be used in {@link WindowsStore}.
+ * This class is not thread safe.
+ *
+ */
+public class WindowKryoSerializer {
--- End diff --
This class is created for using in `WindowsStore` implementations. That is why it is named as `WindowKryoSerializer`.
---
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-1649 Optimize Kryo instaces creation in ...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1273#issuecomment-204110947
@satishd I need to revert the merge because of compilation issues.
---
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-1649 Optimize Kryo instaces creation in ...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1273#issuecomment-203507676
+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-1649 Optimize Kryo instaces creation in ...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the pull request:
https://github.com/apache/storm/pull/1273#issuecomment-203280025
@harshach @ptgoetz @arunmahadevan Can you please review this PR? These changes should be merged to 1.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: STORM-1649 Optimize Kryo instaces creation in ...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1273#issuecomment-203612897
+1
Why does this need to go into 1.x?
---
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-1649 Optimize Kryo instaces creation in ...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1273#issuecomment-204093716
@satishd @ptgoetz
Fine, but I think we need to stop putting stuff into 1.x real soon. This is not a bug fix, it's an optimization. Continuing to add stuff to 1.x is going to potentially push its release out farther than it already has been pushed.
---
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-1649 Optimize Kryo instaces creation in ...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1273#issuecomment-204126364
Reverted in master. @satishd can you take a look and re-open the pull request with a fix.
Unfortunately this likely won't make the cutoff for the 1.0 release, but could be included in a subsequent point release.
---
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-1649 Optimize Kryo instaces creation in ...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1273#discussion_r57955514
--- Diff: storm-core/src/jvm/org/apache/storm/trident/windowing/WindowKryoSerializer.java ---
@@ -0,0 +1,87 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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 org.apache.storm.trident.windowing;
+
+import com.esotericsoftware.kryo.Kryo;
+import com.esotericsoftware.kryo.io.Input;
+import com.esotericsoftware.kryo.io.Output;
+import org.apache.storm.serialization.SerializationFactory;
+
+import java.nio.ByteBuffer;
+import java.util.Map;
+
+/**
+ * Kryo serializer/deserializer for values that are stored as part of windowing. This can be used in {@link WindowsStore}.
+ * This class is not thread safe.
+ *
+ */
+public class WindowKryoSerializer {
--- End diff --
It would be nice to change the name of this class to something more descriptive of its function, since the class itself has nothing to do with windowing.
---
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-1649 Optimize Kryo instaces creation in ...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the pull request:
https://github.com/apache/storm/pull/1273#issuecomment-203764786
@knusbaum Trident windowing with HBaseWindowStore is merged to 1.x-branch. This PR includes changes of HBaseWindowsStore using WindowKryoSerializer which optimizes the Kryo instances creation 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-1649 Optimize Kryo instaces creation in ...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the pull request:
https://github.com/apache/storm/pull/1273#issuecomment-204207233
@ptgoetz TravisCI build is passed for this PR [here](https://travis-ci.org/apache/storm/builds/119160406). I ran locally and I did not face any issues.
---
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-1649 Optimize Kryo instaces creation in ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1273
---
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.
---