[GitHub] storm issue #2433: [STORM-2693] Heartbeats and assignments promotion for sto...

[danny0405]

Github user danny0405 commented on the issue:

https://github.com/apache/storm/pull/2433
  
@HeartSaVioR I can do this
But what is the motivation to let 2.0.0 daemons interact with Storm 1.x and 
0.10.x workers?
Another issue: is there already an option i can reuse or i need to fire a 
new one?


---

[GitHub] storm issue #2433: [STORM-2693] Heartbeats and assignments promotion for sto...

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2433
  
@danny0405 
Could you rebase?

And I'm sorry but I'd like to ask a favor of including option for 
compatibility mode for supporting Storm 1.x and 0.10.x. We have applied 
[STORM-2448](https://issues.apache.org/jira/browse/STORM-2448) which guarantees 
Storm 2.0.0 daemons can interact Storm 1.x and 0.10.x workers. I have 
initialized discussion to revisit that issue, but we may still want to have 
STORM-2448, so we need to have a compatibility mode for now.

Could we let Nimbus also reads the ZK heartbeat like before when the option 
is turned on?


---

[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2504
  
I left the questions because this PR looks like addressing only part of 
STORM-2156.

The summary of issue STORM-2156 is:

```
Add RocksDB instance in Nimbus and write heartbeat based metrics to it
```

and description is:

```
There should be a RocksDB instance in Nimbus where we write metrics from 
the heartbeats. This should allow us to replace storage for the statistics we 
see in the UI and expand the abilities of UIs to allow for time series charting.
Eventually this data will likely come via thrift to Nimbus as the overall 
metric system is overhauled.
```

(Even STORM-2156 should have follow up issue for representing RocksDB 
metrics to UI.)

In order to replace the metrics data in ZK heartbeat, it should be 
mandatory to address worker metrics (to supervisor) to nimbus. I don't mind it 
would be based on STORM-2693 which transfers worker metrics into Nimbus 
(addressing Metrics V1), or it would be following up patch for STORM-2153 
(Metrics V2) being implementation of metrics collector which communicates to 
Nimbus. (For latter we should migrate most of built-in metrics to Metrics V2 so 
that it can be available.)

Actually it only addresses metrics transfer from supervisor to nimbus which 
doesn't bring origin intention and benefits of issue. I guess you have follow 
up issues or even patches but they're opaque for me now so I don't see much 
benefit of this. Please also file follow up issues and group issues together 
(design doc explaining overall plan would be great) so that we can imagine what 
will be changed and how the changes improve Storm.


---

[GitHub] storm issue #2203: STORM-2153: New Metrics Reporting API

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2203
  
@revans2 I'm really sorry to bother you, but given that this is coupled 
with releasing Storm 1.2.0, I'd like to ask a favor to handle this issue prior 
to others.


---

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

[srishtyagrawal]

Github user srishtyagrawal commented on the issue:

https://github.com/apache/storm/pull/2505
  
@HeartSaVioR and @revans2 thanks for your comments. I had not noticed that 
the pagination values are saved per topology. It was good to understand the 
motive behind storing these values temporarily rather than in a cookie. I will 
make the changes in index.html to eliminate the additional call and update the 
PR. 



---

[GitHub] storm-site pull request #1: STORM-2629: Upgrade to latest github-pages to al...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm-site/pull/1


---

[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2504
  
Before looking into the patch in detail, I'd like to see which 
functionalities the patch would want to bring.

1. As far as I read from MetricStore, it looks like providing an aggregated 
point from time-range query. Do I understand correctly? Because what I've 
expected is like a time-series one, and it would replace metrics consumers and 
eventually provide time-series representation. I expect it can still replace 
metrics consumers for the store side btw.

2. I guess I know the answer (at least partially) and eventual following-up 
patch would be storing metrics into external storage (like HBase), but just to 
double check: how it will behave when leader Nimbus goes down and one of 
standby Nimbus promotes to leader? Will metrics stored previously be 
unavailable? If Nimbus gets leadership again, how Nimbus shows the gap while it 
didn't receive the metrics? (especially aggregated values) Do we want to apply 
interpolation, or just treat it as no metrics (hence 0 for sum and None for 
avg)?


---

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

[revans2]

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2505
  
@HeartSaVioR It is totally possible to do.  The problem we ran into was 
that I would change something in one tab (like searching for a specific 
keyword), and then refresh a different tab for a different topology and it 
would pick up the changes.

It was just not a good user experience because users would not know why 
something on one page/tab impacted things on another page/tab.  That is why we 
started to make the setting topology specific and then made them session based 
to avoid leaks.

I will see if I can come up with a simpler way to deal with this.


---

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

[HeartSaVioR]

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2505
  
@srishtyagrawal No, I wasn't aware of stateSave hence thought about 
implementing session. Please ignore about cookie.

@revans2 Thanks for the info. I see how it will work.

For me it would be ideal if we can extract paging size from state data and 
store/load it globally (in stateSaveCallback/stateLoadCallback), so table in 
topology B starts from length 50 if I changed the length from somewhere else. 
It will require understanding of state data in DataTables (and upgrading 
DataTables might break the functionality) and I'm not sure even it is possible, 
so please treat this as just a 2 cents.


---

[GitHub] storm pull request #2507: STORM-2885: Avoid conflicts with nimbusDaemon Loca...

[srdo]

Github user srdo commented on a diff in the pull request:

https://github.com/apache/storm/pull/2507#discussion_r160278186
  
--- Diff: 
storm-server/src/test/java/org/apache/storm/nimbus/LocalNimbusTest.java ---
@@ -32,42 +37,35 @@
 import org.junit.Assert;
 import org.junit.Test;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
-import java.util.UUID;
-
 /**
  * Tests local cluster with nimbus and a plugin for {@link 
Config#STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN}.
  */
 public class LocalNimbusTest {
 
 @Test
 public void testSubmitTopologyToLocalNimbus() throws Exception {
-
-HashMap localClusterConf = new HashMap<>();
-localClusterConf.put("nimbus-daemon", true);
-ILocalCluster localCluster = 
Testing.getLocalCluster(localClusterConf);
-
-Config topoConf = new Config();
-topoConf.putAll(Utils.readDefaultConfig());
-topoConf.setDebug(true);
-topoConf.put("storm.cluster.mode", "local"); // default is aways 
"distributed" but here local cluster is being used.
-topoConf.put(Config.STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN, 
InmemoryTopologySubmitterHook.class.getName());
-
-List topologyNames =new ArrayList<>();
-for (int i=0; i<4; i++) {
-final String topologyName = "word-count-"+ 
UUID.randomUUID().toString();
-final StormTopology stormTopology = createTestTopology();
-topologyNames.add(new TopologyDetails(topologyName, 
stormTopology));
-localCluster.submitTopology(topologyName, topoConf, 
stormTopology);
+int port = Utils.getAvailablePort();
--- End diff --

Thanks


---

[GitHub] storm pull request #2507: STORM-2885: Avoid conflicts with nimbusDaemon Loca...

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2507#discussion_r160277946
  
--- Diff: 
storm-server/src/test/java/org/apache/storm/nimbus/LocalNimbusTest.java ---
@@ -32,42 +37,35 @@
 import org.junit.Assert;
 import org.junit.Test;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
-import java.util.UUID;
-
 /**
  * Tests local cluster with nimbus and a plugin for {@link 
Config#STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN}.
  */
 public class LocalNimbusTest {
 
 @Test
 public void testSubmitTopologyToLocalNimbus() throws Exception {
-
-HashMap localClusterConf = new HashMap<>();
-localClusterConf.put("nimbus-daemon", true);
-ILocalCluster localCluster = 
Testing.getLocalCluster(localClusterConf);
-
-Config topoConf = new Config();
-topoConf.putAll(Utils.readDefaultConfig());
-topoConf.setDebug(true);
-topoConf.put("storm.cluster.mode", "local"); // default is aways 
"distributed" but here local cluster is being used.
-topoConf.put(Config.STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN, 
InmemoryTopologySubmitterHook.class.getName());
-
-List topologyNames =new ArrayList<>();
-for (int i=0; i<4; i++) {
-final String topologyName = "word-count-"+ 
UUID.randomUUID().toString();
-final StormTopology stormTopology = createTestTopology();
-topologyNames.add(new TopologyDetails(topologyName, 
stormTopology));
-localCluster.submitTopology(topologyName, topoConf, 
stormTopology);
+int port = Utils.getAvailablePort();
--- End diff --

I filed STORM-2886 to address the issue.


---

[GitHub] storm pull request #2507: STORM-2885: Avoid conflicts with nimbusDaemon Loca...

[srdo]

Github user srdo commented on a diff in the pull request:

https://github.com/apache/storm/pull/2507#discussion_r160271224
  
--- Diff: 
storm-server/src/test/java/org/apache/storm/nimbus/LocalNimbusTest.java ---
@@ -32,42 +37,35 @@
 import org.junit.Assert;
 import org.junit.Test;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
-import java.util.UUID;
-
 /**
  * Tests local cluster with nimbus and a plugin for {@link 
Config#STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN}.
  */
 public class LocalNimbusTest {
 
 @Test
 public void testSubmitTopologyToLocalNimbus() throws Exception {
-
-HashMap localClusterConf = new HashMap<>();
-localClusterConf.put("nimbus-daemon", true);
-ILocalCluster localCluster = 
Testing.getLocalCluster(localClusterConf);
-
-Config topoConf = new Config();
-topoConf.putAll(Utils.readDefaultConfig());
-topoConf.setDebug(true);
-topoConf.put("storm.cluster.mode", "local"); // default is aways 
"distributed" but here local cluster is being used.
-topoConf.put(Config.STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN, 
InmemoryTopologySubmitterHook.class.getName());
-
-List topologyNames =new ArrayList<>();
-for (int i=0; i<4; i++) {
-final String topologyName = "word-count-"+ 
UUID.randomUUID().toString();
-final StormTopology stormTopology = createTestTopology();
-topologyNames.add(new TopologyDetails(topologyName, 
stormTopology));
-localCluster.submitTopology(topologyName, topoConf, 
stormTopology);
+int port = Utils.getAvailablePort();
--- End diff --

I'd like us to at least file a JIRA to look into changing it, even if we 
don't do it now. Most of the other uses of Utils.getAvailablePort should have 
been removed in earlier PRs (e.g. https://github.com/apache/storm/pull/2140). I 
want to say assigning ports this way was causing test failures on Travis, but 
it's been a while so I may be misremembering. 


---

[GitHub] storm pull request #2507: STORM-2885: Avoid conflicts with nimbusDaemon Loca...

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2507#discussion_r160260170
  
--- Diff: 
storm-server/src/test/java/org/apache/storm/nimbus/LocalNimbusTest.java ---
@@ -32,42 +37,35 @@
 import org.junit.Assert;
 import org.junit.Test;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
-import java.util.UUID;
-
 /**
  * Tests local cluster with nimbus and a plugin for {@link 
Config#STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN}.
  */
 public class LocalNimbusTest {
 
 @Test
 public void testSubmitTopologyToLocalNimbus() throws Exception {
-
-HashMap localClusterConf = new HashMap<>();
-localClusterConf.put("nimbus-daemon", true);
-ILocalCluster localCluster = 
Testing.getLocalCluster(localClusterConf);
-
-Config topoConf = new Config();
-topoConf.putAll(Utils.readDefaultConfig());
-topoConf.setDebug(true);
-topoConf.put("storm.cluster.mode", "local"); // default is aways 
"distributed" but here local cluster is being used.
-topoConf.put(Config.STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN, 
InmemoryTopologySubmitterHook.class.getName());
-
-List topologyNames =new ArrayList<>();
-for (int i=0; i<4; i++) {
-final String topologyName = "word-count-"+ 
UUID.randomUUID().toString();
-final StormTopology stormTopology = createTestTopology();
-topologyNames.add(new TopologyDetails(topologyName, 
stormTopology));
-localCluster.submitTopology(topologyName, topoConf, 
stormTopology);
+int port = Utils.getAvailablePort();
--- End diff --

Oh we also explicitly check that the port is not 0 to avoid having users 
shoot themselves in the foot and bring up a nimbus that no one can talk to.


---

[GitHub] storm pull request #2507: STORM-2885: Avoid conflicts with nimbusDaemon Loca...

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2507#discussion_r160259977
  
--- Diff: 
storm-server/src/test/java/org/apache/storm/nimbus/LocalNimbusTest.java ---
@@ -32,42 +37,35 @@
 import org.junit.Assert;
 import org.junit.Test;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
-import java.util.UUID;
-
 /**
  * Tests local cluster with nimbus and a plugin for {@link 
Config#STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN}.
  */
 public class LocalNimbusTest {
 
 @Test
 public void testSubmitTopologyToLocalNimbus() throws Exception {
-
-HashMap localClusterConf = new HashMap<>();
-localClusterConf.put("nimbus-daemon", true);
-ILocalCluster localCluster = 
Testing.getLocalCluster(localClusterConf);
-
-Config topoConf = new Config();
-topoConf.putAll(Utils.readDefaultConfig());
-topoConf.setDebug(true);
-topoConf.put("storm.cluster.mode", "local"); // default is aways 
"distributed" but here local cluster is being used.
-topoConf.put(Config.STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN, 
InmemoryTopologySubmitterHook.class.getName());
-
-List topologyNames =new ArrayList<>();
-for (int i=0; i<4; i++) {
-final String topologyName = "word-count-"+ 
UUID.randomUUID().toString();
-final StormTopology stormTopology = createTestTopology();
-topologyNames.add(new TopologyDetails(topologyName, 
stormTopology));
-localCluster.submitTopology(topologyName, topoConf, 
stormTopology);
+int port = Utils.getAvailablePort();
--- End diff --

Yes it is racy.

We have done it this way for other ports without too many issues.  The 
problem is that it would require a feedback path for the port from Nimbus, and 
possibly a few updates in other places that try to read the port out of the 
config.  If you really want to see this changed I can file a follow on JIRA and 
look into it, as it would not likely be that much work.


---

[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/2485


---

[GitHub] storm-site issue #1: STORM-2629: Upgrade to latest github-pages to allow Win...

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm-site/pull/1
  
@HeartSaVioR Sorry to bother you with this, but I think this PR fell 
through the cracks. If you get a chance sometime could you try to build the 
site with the commands I mentioned in the previous comment?


---

[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB

[agresch]

Github user agresch commented on the issue:

https://github.com/apache/storm/pull/2504
  
Will look into these issues.  


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[agresch]

Github user agresch commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160250982
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/rocksdb/RocksDbMetricsWriter.java
 ---
@@ -0,0 +1,306 @@
+/**
+ * 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 org.apache.storm.metricstore.rocksdb;
+
+import com.codahale.metrics.Meter;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.ThreadLocalRandom;
+import org.apache.storm.metricstore.AggLevel;
+import org.apache.storm.metricstore.Metric;
+import org.apache.storm.metricstore.MetricException;
+import org.rocksdb.FlushOptions;
+import org.rocksdb.RocksDBException;
+import org.rocksdb.WriteBatch;
+import org.rocksdb.WriteOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class designed to perform all metrics inserts into RocksDB.  Metrics 
are processed from the a blocking queue.
+ * 
+ * A writable LRU StringMetadataCache is used to minimize looking up 
metadata string Ids.  As entries are added to the full cache, older
+ * entries are evicted from the cache and need to be written to the 
database.  This happens as the handleEvictedMetadata()
+ * method callback.
+ */
+public class RocksDbMetricsWriter implements Runnable, AutoCloseable {
+private static final Logger LOG = 
LoggerFactory.getLogger(RocksDbMetricsWriter.class);
+private RocksDbStore store;
+private BlockingQueue queue;
+private WritableStringMetadataCache stringMetadataCache;
+private Set unusedIds = new HashSet<>();
+private TreeMap insertBatch = new 
TreeMap<>(); // RocksDB should insert in sorted key order
+private WriteOptions writeOpts = new WriteOptions();
+private volatile boolean shutdown = false;
+private Meter failureMeter;
+private ArrayList aggBuckets = new ArrayList<>();
+
+/**
+ * Constructor for the RocksDbMetricsWriter.
+ *
+ * @param store   The RocksDB store
+ * @param queue   The queue to receive metrics for insertion
+ */
+RocksDbMetricsWriter(RocksDbStore store, BlockingQueue queue, Meter 
failureMeter)  {
+this.store = store;
+this.queue = queue;
+this.failureMeter = failureMeter;
+
+aggBuckets.add(AggLevel.AGG_LEVEL_1_MIN);
+aggBuckets.add(AggLevel.AGG_LEVEL_10_MIN);
+aggBuckets.add(AggLevel.AGG_LEVEL_60_MIN);
+}
+
+/**
+ * Init routine called once the Metadata cache has been created.
+ *
+ * @throws MetricException  on cache error
+ */
+void init() throws MetricException {
+this.stringMetadataCache = 
StringMetadataCache.getWritableStringMetadataCache();
+}
+
+/**
+ * Run routine to wait for metrics on a queue and insert into RocksDB.
+ */
+@Override
+public void run() {
+while (true) {
+if (shutdown) {
+return;
+}
+try {
+Metric m = (Metric) queue.take();
+processInsert(m);
+} catch (Exception e) {
+LOG.error("Failed to insert metric", e);
+if (this.failureMeter != null) {
+this.failureMeter.mark();
+}
+}
+}
+}
+
+/**
+ * Performs the actual metric insert, and aggregates over all bucket 
times.
+ *
+ * @param metric  Metric to store
+ * @throws MetricException  if database write fails
+ */
+private void processInsert(Metric metric) throws MetricException {
+
+// convert all strings to num

Re: [DISCUSS] Release Storm 1.0.5 / 1.1.2

[Stig Rohde Døssing]

+1 for starting 1.1.2 release process.

2018-01-08 20:27 GMT+01:00 P. Taylor Goetz :

> +1
>
> If there are no remaining issues to be included, we can start the release
> process.
>
> -Taylor
>
> > On Jan 7, 2018, at 7:07 PM, Jungtaek Lim  wrote:
> >
> > Bump, does someone have issues which are necessary to be included in
> Storm
> > 1.1.2? If not I think we should start release phase for 1.1.2 soon.
> >
> > -Jungtaek Lim (HeartSaVioR)
> >
> > 2017년 12월 28일 (목) 오후 3:16, Jungtaek Lim 님이 작성:
> >
> >> I have been really busy so couldn't care about releases, and now I got
> >> some time period to track again.
> >>
> >> We have been delaying new release, since we have been focusing on 1.2.0
> >> and issues relevant in storm-kafka-client have been raised continuously.
> >> (though things looks like going to be less critical)
> >>
> >> But other than storm-kafka-client issues, I think Storm 1.1.2 is ready
> to
> >> be released, and we should release Storm 1.1.2 regardless of Storm 1.2.0
> >> because we have fixed another critical issues in core (STORM-2231[1],
> >> STORM-2682[2]) which are published to 1.0.5 but no release in 1.1.x
> version
> >> line yet.
> >> (Noting that some bugfixes on storm-kafka-client are not ported back to
> >> 1.1.x version line because of heavy divergence.)
> >>
> >> It may not be good time to discuss since it is year-end now, but would
> >> like to remind this so that we could start the process at least earlier
> in
> >> next year.
> >>
> >> Thanks,
> >> Jungtaek Lim (HeartSaVioR)
> >>
> >> 1. https://issues.apache.org/jira/browse/STORM-2231
> >> 2. https://issues.apache.org/jira/browse/STORM-2682
> >>
> >> 2017년 10월 19일 (목) 오전 1:19, Stig Rohde Døssing  >님이
> >> 작성:
> >>
> >>> Looks like Hugo is working on it
> >>> https://issues.apache.org/jira/browse/STORM-2781
> >>>
> >>> 2017-10-18 4:22 GMT+02:00 Jungtaek Lim :
> >>>
> > I'm hoping the delay for 1.2.0 will be very short. The changes we
>  discussed
> > were minor, and had to do with renaming some of the new methods and
> > constants. It would be good to do before 1.2.0 because the renames
> are
> > breaking changes.
> 
>  Suppose the changes will be minor, then why not go ahead making the
> >>> change?
>  I guess it doesn't need much efforts to do.
>  (Will Hugo submit the patch?)
> 
>  I'm +1 to get it before 1.2.0, and also hope that the delay will be
> very
>  short.
> 
>  2017년 10월 14일 (토) 오후 6:05, Alexandre Vermeerbergen <
>  avermeerber...@gmail.com>님이
>  작성:
> 
> > +1 for a short delay until 1.2.0 is avaible :)
> >
> > 2017-10-14 8:48 GMT+02:00 Stig Rohde Døssing   :
> >
> >> I'm hoping the delay for 1.2.0 will be very short. The changes we
> > discussed
> >> were minor, and had to do with renaming some of the new methods and
> >> constants. It would be good to do before 1.2.0 because the renames
> >>> are
> >> breaking changes.
> >>
> >> 2017-10-14 5:33 GMT+02:00 Arun Mahadevan :
> >>
> >>> I was hoping we will get 1.2.0 out along with 1.1.2. The pending
>  issues
> >> in
> >>> the epic https://issues.apache.org/jira/browse/STORM-2710 seems
> >>> to
> > have
> >>> been addressed. Can you add the new issue to the epic?
> >>>
> >>> If its not something critical we can do it in a minor release post
> > 1.2.0.
> >>>
> >>> Thanks,
> >>> Arun
> >>>
> >>>
> >>> On 10/14/17, 3:50 AM, "Hugo Da Cruz Louro" <
> >>> hlo...@hortonworks.com>
> >> wrote:
> >>>
>  I am +1 to releasing 1.1.2 right away. I am in the middle of one
> > review
> >>> but I will finish it in the next day, such that we can get this
>  merged
> >> soon.
> 
>  However, we need to hold onto releasing 1.2.0 until some of the
> > changes
> >>> for ProcessingGuarantee that got in this patch<
> >>> https://github.com/
> >>> apache/storm/commit/48f6969027e7b02a5b9220577189d3911aa2226d> are
> > fixed.
> >>> I briefly discussed [1] this issue with @Stig on Gitter, I will
>  submit
> > a
> >>> patch with the change.
> 
>  Thanks,
>  Hugo
>  [1] - We did not have a technical discussion. I just asked a
> >>> couple
>  of
> >>> clarifying questions and then the idea surged that we should
> >>> improve
> > some
> >>> of the changes in this  patch  apache/storm/commit/
> >>> 48f6969027e7b02a5b9220577189d3911aa2226d>. I will create a JIRA,
> >>> and
> > all
> >>> the discussion go through either JIRA or dev email list.
> 
>  On Oct 10, 2017, at 12:48 PM, Stig Rohde Døssing <
> >> stigdoess...@gmail.com<
> >>> mailto:stigdoess...@gmail.com>> wrote:
> 
>  Thanks Jungtaek, that sounds like a good plan. Here's the new PR
> >>> for
> >> 2607
>  https://github.com/apache/storm/pull/2367.
> >

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[agresch]

Github user agresch commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160250660
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/rocksdb/RocksDbMetricsWriter.java
 ---
@@ -0,0 +1,306 @@
+/**
+ * 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 org.apache.storm.metricstore.rocksdb;
+
+import com.codahale.metrics.Meter;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.ThreadLocalRandom;
+import org.apache.storm.metricstore.AggLevel;
+import org.apache.storm.metricstore.Metric;
+import org.apache.storm.metricstore.MetricException;
+import org.rocksdb.FlushOptions;
+import org.rocksdb.RocksDBException;
+import org.rocksdb.WriteBatch;
+import org.rocksdb.WriteOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class designed to perform all metrics inserts into RocksDB.  Metrics 
are processed from the a blocking queue.
+ * 
+ * A writable LRU StringMetadataCache is used to minimize looking up 
metadata string Ids.  As entries are added to the full cache, older
+ * entries are evicted from the cache and need to be written to the 
database.  This happens as the handleEvictedMetadata()
+ * method callback.
+ */
+public class RocksDbMetricsWriter implements Runnable, AutoCloseable {
+private static final Logger LOG = 
LoggerFactory.getLogger(RocksDbMetricsWriter.class);
+private RocksDbStore store;
+private BlockingQueue queue;
+private WritableStringMetadataCache stringMetadataCache;
+private Set unusedIds = new HashSet<>();
+private TreeMap insertBatch = new 
TreeMap<>(); // RocksDB should insert in sorted key order
--- End diff --

The MetricsWriter is a single thread implementation.  RocksDB documentation 
recommended a single writer for speed, and this simplified the caching of 
metadata.  If we need to switch to multiple threads, I completely agree the 
design is not thread safe.


---

[GitHub] storm pull request #2507: STORM-2885: Avoid conflicts with nimbusDaemon Loca...

[srdo]

Github user srdo commented on a diff in the pull request:

https://github.com/apache/storm/pull/2507#discussion_r160248987
  
--- Diff: 
storm-server/src/test/java/org/apache/storm/nimbus/LocalNimbusTest.java ---
@@ -32,42 +37,35 @@
 import org.junit.Assert;
 import org.junit.Test;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
-import java.util.UUID;
-
 /**
  * Tests local cluster with nimbus and a plugin for {@link 
Config#STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN}.
  */
 public class LocalNimbusTest {
 
 @Test
 public void testSubmitTopologyToLocalNimbus() throws Exception {
-
-HashMap localClusterConf = new HashMap<>();
-localClusterConf.put("nimbus-daemon", true);
-ILocalCluster localCluster = 
Testing.getLocalCluster(localClusterConf);
-
-Config topoConf = new Config();
-topoConf.putAll(Utils.readDefaultConfig());
-topoConf.setDebug(true);
-topoConf.put("storm.cluster.mode", "local"); // default is aways 
"distributed" but here local cluster is being used.
-topoConf.put(Config.STORM_TOPOLOGY_SUBMISSION_NOTIFIER_PLUGIN, 
InmemoryTopologySubmitterHook.class.getName());
-
-List topologyNames =new ArrayList<>();
-for (int i=0; i<4; i++) {
-final String topologyName = "word-count-"+ 
UUID.randomUUID().toString();
-final StormTopology stormTopology = createTestTopology();
-topologyNames.add(new TopologyDetails(topologyName, 
stormTopology));
-localCluster.submitTopology(topologyName, topoConf, 
stormTopology);
+int port = Utils.getAvailablePort();
--- End diff --

Since this is the only test that uses a Nimbus daemon it's probably not 
going to cause issues yet, but this way of getting a port is racy. Is there a 
way to instead get the Nimbus port from the running LocalCluster, and configure 
the Nimbus daemon to start on port 0?


---

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

[srishtyagrawal]

Github user srishtyagrawal commented on the issue:

https://github.com/apache/storm/pull/2505
  
@HeartSaVioR, I agree that saving pagination for a user makes more sense. 
Currently, this information is saved per session for a user. [This is being 
done using stateSave 
functionality](https://datatables.net/examples/basic_init/state_save.html). 
Do we still want a cookie to store the pagination value?


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160241948
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -1081,6 +1088,14 @@ public Nimbus(Map conf, INimbus 
inimbus, IStormClusterState stor
 BlobStore blobStore, TopoCache topoCache, ILeaderElector 
leaderElector, IGroupMappingServiceProvider groupMapper)
 throws Exception {
 this.conf = conf;
+
+this.metricsStore = null;
--- End diff --

Can we verify that if the metricsStore is null that we don't get any 
NullPointerExceptions in really bad places.


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160242680
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -4166,4 +4184,23 @@ public void shutdown() {
 public boolean isWaiting() {
 return timer.isTimerWaiting();
 }
+
+@Override
+public void processWorkerMetrics(WorkerMetrics metrics) throws 
org.apache.thrift.TException {
+if (this.metricsStore == null) {
--- End diff --

It would also be nice to have a metric for the number of times that this 
method was called, like with the other RPC metrics we have.


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160243540
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/MetricStore.java ---
@@ -0,0 +1,76 @@
+/**
+ * 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 org.apache.storm.metricstore;
+
+import com.codahale.metrics.Meter;
+
+import java.util.Map;
+
+public interface MetricStore {
--- End diff --

Can we make this extend AutoCloseable?


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160241598
  
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -1023,6 +1023,43 @@
 public static String STORM_SUPERVISOR_MEDIUM_MEMORY_GRACE_PERIOD_MS =
 "storm.supervisor.medium.memory.grace.period.ms";
 
+/**
+ * Class implementing MetricStore.
+ */
+@NotNull
+@isString
+public static final String STORM_METRIC_STORE_CLASS = 
"storm.metricstore.class";
+
+/**
+ * RocksDB file location.
+ */
+@isString
+public static final String STORM_ROCKSDB_LOCATION = 
"storm.metricstore.rocksdb.location";
+
+/**
+ * RocksDB create if missing flag.
+ */
+@isBoolean
+public static final String STORM_ROCKSDB_CREATE_IF_MISSING = 
"storm.metricstore.rocksdb.create_if_missing";
+
+/**
+ * RocksDB metadata cache capacity.
+ */
+@isInteger
+public static final String 
STORM_ROCKSDB_METADATA_STRING_CACHE_CAPACITY = 
"storm.metricstore.rocksdb.metadata_string_cache_capacity";
+
+/**
+ * RocksDB setting for length of metric retention.
+ */
+@isInteger
+public static final String STORM_ROCKSDB_METRIC_RETENTION_HOURS = 
"storm.metricstore.rocksdb.retention_hours";
+
+/**
+ * RocksDB setting for period of metric deletion thread.
+ */
+@isInteger
+public static final String STORM_ROCKSDB_METRIC_RETENTION_PERIOD_HOURS 
= "storm.metricstore.rocksdb.retention_period_hours";
--- End diff --

Could we add in deletion to the name of the config somewhere?  I am not 
sure that it is that clear what the config does from just the name of it.


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160245890
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/rocksdb/RocksDbStore.java
 ---
@@ -0,0 +1,636 @@
+/**
+ * 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 org.apache.storm.metricstore.rocksdb;
+
+import com.codahale.metrics.Meter;
+import java.io.File;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.storm.DaemonConfig;
+import org.apache.storm.metric.StormMetricsRegistry;
+import org.apache.storm.metricstore.AggLevel;
+import org.apache.storm.metricstore.FilterOptions;
+import org.apache.storm.metricstore.Metric;
+import org.apache.storm.metricstore.MetricException;
+import org.apache.storm.metricstore.MetricStore;
+import org.apache.storm.utils.ObjectReader;
+import org.rocksdb.BlockBasedTableConfig;
+import org.rocksdb.IndexType;
+import org.rocksdb.Options;
+import org.rocksdb.ReadOptions;
+import org.rocksdb.RocksDB;
+import org.rocksdb.RocksDBException;
+import org.rocksdb.RocksIterator;
+import org.rocksdb.WriteBatch;
+import org.rocksdb.WriteOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class RocksDbStore implements MetricStore, AutoCloseable {
+private static final Logger LOG = 
LoggerFactory.getLogger(RocksDbStore.class);
+private static final int MAX_QUEUE_CAPACITY = 4000;
+static final int INVALID_METADATA_STRING_ID = 0;
+RocksDB db;
+private ReadOnlyStringMetadataCache readOnlyStringMetadataCache = null;
+private BlockingQueue queue = new 
LinkedBlockingQueue(MAX_QUEUE_CAPACITY);
+private RocksDbMetricsWriter metricsWriter = null;
+private MetricsCleaner metricsCleaner = null;
+private Meter failureMeter = null;
+
+interface RocksDbScanCallback {
+boolean cb(RocksDbKey key, RocksDbValue val);  // return false to 
stop scan
+}
+
+/**
+ * Create metric store instance using the configurations provided via 
the config map.
+ *
+ * @param config Storm config map
+ * @throws MetricException on preparation error
+ */
+public void prepare(Map config) throws MetricException {
+validateConfig(config);
+
+this.failureMeter = 
StormMetricsRegistry.registerMeter("RocksDB:metric-failures");
+
+RocksDB.loadLibrary();
+boolean createIfMissing = 
ObjectReader.getBoolean(config.get(DaemonConfig.STORM_ROCKSDB_CREATE_IF_MISSING),
 false);
+
+try (Options options = new 
Options().setCreateIfMissing(createIfMissing)) {
+// use the hash index for prefix searches
+BlockBasedTableConfig tfc = new BlockBasedTableConfig();
+tfc.setIndexType(IndexType.kHashSearch);
+options.setTableFormatConfig(tfc);
+options.useCappedPrefixExtractor(RocksDbKey.KEY_SIZE);
+
+String path = getRocksDbAbsoluteDir(config);
+LOG.info("Opening RocksDB from " + path);
+db = RocksDB.open(options, path);
+} catch (RocksDBException e) {
+String message = "Error opening RockDB database";
+LOG.error(message, e);
+throw new MetricException(message, e);
+}
+
+// create thread to delete old metrics and metadata
+Integer retentionHours = 
Integer.parseInt(config.get(DaemonConfig.STORM_ROCKSDB_METRIC_RETENTION_HOURS).toString());
+Integer retentionPeriod = 0;
+if 
(config.containsKey(DaemonConfig.STORM_ROCKSDB_METRIC_RETENTION_PERIOD_HOURS)) {
+retentionPeriod = 
Integer.parseInt(config.get(DaemonConfig.STORM_ROCKSDB_METRIC_RETENTION_PERIOD_HOUR

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160244314
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/rocksdb/RocksDbMetricsWriter.java
 ---
@@ -0,0 +1,306 @@
+/**
+ * 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 org.apache.storm.metricstore.rocksdb;
+
+import com.codahale.metrics.Meter;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.ThreadLocalRandom;
+import org.apache.storm.metricstore.AggLevel;
+import org.apache.storm.metricstore.Metric;
+import org.apache.storm.metricstore.MetricException;
+import org.rocksdb.FlushOptions;
+import org.rocksdb.RocksDBException;
+import org.rocksdb.WriteBatch;
+import org.rocksdb.WriteOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class designed to perform all metrics inserts into RocksDB.  Metrics 
are processed from the a blocking queue.
+ * 
+ * A writable LRU StringMetadataCache is used to minimize looking up 
metadata string Ids.  As entries are added to the full cache, older
+ * entries are evicted from the cache and need to be written to the 
database.  This happens as the handleEvictedMetadata()
+ * method callback.
+ */
+public class RocksDbMetricsWriter implements Runnable, AutoCloseable {
+private static final Logger LOG = 
LoggerFactory.getLogger(RocksDbMetricsWriter.class);
+private RocksDbStore store;
+private BlockingQueue queue;
+private WritableStringMetadataCache stringMetadataCache;
+private Set unusedIds = new HashSet<>();
+private TreeMap insertBatch = new 
TreeMap<>(); // RocksDB should insert in sorted key order
--- End diff --

I don't think this is being used in a thread safe way.
  


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160245178
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/rocksdb/RocksDbMetricsWriter.java
 ---
@@ -0,0 +1,306 @@
+/**
+ * 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 org.apache.storm.metricstore.rocksdb;
+
+import com.codahale.metrics.Meter;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.ThreadLocalRandom;
+import org.apache.storm.metricstore.AggLevel;
+import org.apache.storm.metricstore.Metric;
+import org.apache.storm.metricstore.MetricException;
+import org.rocksdb.FlushOptions;
+import org.rocksdb.RocksDBException;
+import org.rocksdb.WriteBatch;
+import org.rocksdb.WriteOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class designed to perform all metrics inserts into RocksDB.  Metrics 
are processed from the a blocking queue.
+ * 
+ * A writable LRU StringMetadataCache is used to minimize looking up 
metadata string Ids.  As entries are added to the full cache, older
+ * entries are evicted from the cache and need to be written to the 
database.  This happens as the handleEvictedMetadata()
+ * method callback.
+ */
+public class RocksDbMetricsWriter implements Runnable, AutoCloseable {
+private static final Logger LOG = 
LoggerFactory.getLogger(RocksDbMetricsWriter.class);
+private RocksDbStore store;
+private BlockingQueue queue;
+private WritableStringMetadataCache stringMetadataCache;
+private Set unusedIds = new HashSet<>();
+private TreeMap insertBatch = new 
TreeMap<>(); // RocksDB should insert in sorted key order
+private WriteOptions writeOpts = new WriteOptions();
+private volatile boolean shutdown = false;
+private Meter failureMeter;
+private ArrayList aggBuckets = new ArrayList<>();
+
+/**
+ * Constructor for the RocksDbMetricsWriter.
+ *
+ * @param store   The RocksDB store
+ * @param queue   The queue to receive metrics for insertion
+ */
+RocksDbMetricsWriter(RocksDbStore store, BlockingQueue queue, Meter 
failureMeter)  {
+this.store = store;
+this.queue = queue;
+this.failureMeter = failureMeter;
+
+aggBuckets.add(AggLevel.AGG_LEVEL_1_MIN);
+aggBuckets.add(AggLevel.AGG_LEVEL_10_MIN);
+aggBuckets.add(AggLevel.AGG_LEVEL_60_MIN);
+}
+
+/**
+ * Init routine called once the Metadata cache has been created.
+ *
+ * @throws MetricException  on cache error
+ */
+void init() throws MetricException {
+this.stringMetadataCache = 
StringMetadataCache.getWritableStringMetadataCache();
+}
+
+/**
+ * Run routine to wait for metrics on a queue and insert into RocksDB.
+ */
+@Override
+public void run() {
+while (true) {
+if (shutdown) {
+return;
+}
+try {
+Metric m = (Metric) queue.take();
+processInsert(m);
+} catch (Exception e) {
+LOG.error("Failed to insert metric", e);
+if (this.failureMeter != null) {
+this.failureMeter.mark();
+}
+}
+}
+}
+
+/**
+ * Performs the actual metric insert, and aggregates over all bucket 
times.
+ *
+ * @param metric  Metric to store
+ * @throws MetricException  if database write fails
+ */
+private void processInsert(Metric metric) throws MetricException {
+
+// convert all strings to num

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160240315
  
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -291,12 +291,22 @@ public static long bitXor(Long a, Long b) {
  * runtime to avoid any zombie process in case cleanup function hangs.
  */
 public static void addShutdownHookWithForceKillIn1Sec (Runnable func) {
+addShutdownHookWithDelayedForceKill(func, 1);
+}
+
+/**
+ * Adds the user supplied function as a shutdown hook for cleanup.
+ * Also adds a function that sleeps for numSecs and then halts the
+ * runtime to avoid any zombie process in case cleanup function hangs.
+ */
+public static void addShutdownHookWithDelayedForceKill (Runnable func, 
int numSecs) {
 Runnable sleepKill = new Runnable() {
 @Override
 public void run() {
 try {
-Time.sleepSecs(1);
-LOG.warn("Forceing Halt...");
+LOG.info("Halting after " + numSecs + " seconds");
--- End diff --

Nit could we use the slf4j logging format like

```
LOG.info("Halting after {} second", numSeconds);
```


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160241080
  
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -1023,6 +1023,43 @@
 public static String STORM_SUPERVISOR_MEDIUM_MEMORY_GRACE_PERIOD_MS =
 "storm.supervisor.medium.memory.grace.period.ms";
 
+/**
+ * Class implementing MetricStore.
+ */
+@NotNull
+@isString
+public static final String STORM_METRIC_STORE_CLASS = 
"storm.metricstore.class";
+
+/**
+ * RocksDB file location.
--- End diff --

Could we expand the javadocs to explain that this is only for the 
`org.apache.storm.metricstore.rocksdb.RocksDbStore`, and that would be good for 
all of the newly added javadocs that are specific to the RocksDBMetaStore.
  


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160246323
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metricstore/rocksdb/RocksDbStore.java
 ---
@@ -0,0 +1,636 @@
+/**
+ * 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 org.apache.storm.metricstore.rocksdb;
+
+import com.codahale.metrics.Meter;
+import java.io.File;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.storm.DaemonConfig;
+import org.apache.storm.metric.StormMetricsRegistry;
+import org.apache.storm.metricstore.AggLevel;
+import org.apache.storm.metricstore.FilterOptions;
+import org.apache.storm.metricstore.Metric;
+import org.apache.storm.metricstore.MetricException;
+import org.apache.storm.metricstore.MetricStore;
+import org.apache.storm.utils.ObjectReader;
+import org.rocksdb.BlockBasedTableConfig;
+import org.rocksdb.IndexType;
+import org.rocksdb.Options;
+import org.rocksdb.ReadOptions;
+import org.rocksdb.RocksDB;
+import org.rocksdb.RocksDBException;
+import org.rocksdb.RocksIterator;
+import org.rocksdb.WriteBatch;
+import org.rocksdb.WriteOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class RocksDbStore implements MetricStore, AutoCloseable {
+private static final Logger LOG = 
LoggerFactory.getLogger(RocksDbStore.class);
+private static final int MAX_QUEUE_CAPACITY = 4000;
+static final int INVALID_METADATA_STRING_ID = 0;
+RocksDB db;
+private ReadOnlyStringMetadataCache readOnlyStringMetadataCache = null;
+private BlockingQueue queue = new 
LinkedBlockingQueue(MAX_QUEUE_CAPACITY);
+private RocksDbMetricsWriter metricsWriter = null;
+private MetricsCleaner metricsCleaner = null;
+private Meter failureMeter = null;
+
+interface RocksDbScanCallback {
+boolean cb(RocksDbKey key, RocksDbValue val);  // return false to 
stop scan
+}
+
+/**
+ * Create metric store instance using the configurations provided via 
the config map.
+ *
+ * @param config Storm config map
+ * @throws MetricException on preparation error
+ */
+public void prepare(Map config) throws MetricException {
+validateConfig(config);
+
+this.failureMeter = 
StormMetricsRegistry.registerMeter("RocksDB:metric-failures");
+
+RocksDB.loadLibrary();
+boolean createIfMissing = 
ObjectReader.getBoolean(config.get(DaemonConfig.STORM_ROCKSDB_CREATE_IF_MISSING),
 false);
+
+try (Options options = new 
Options().setCreateIfMissing(createIfMissing)) {
+// use the hash index for prefix searches
+BlockBasedTableConfig tfc = new BlockBasedTableConfig();
+tfc.setIndexType(IndexType.kHashSearch);
+options.setTableFormatConfig(tfc);
+options.useCappedPrefixExtractor(RocksDbKey.KEY_SIZE);
+
+String path = getRocksDbAbsoluteDir(config);
+LOG.info("Opening RocksDB from " + path);
+db = RocksDB.open(options, path);
+} catch (RocksDBException e) {
+String message = "Error opening RockDB database";
+LOG.error(message, e);
+throw new MetricException(message, e);
+}
+
+// create thread to delete old metrics and metadata
+Integer retentionHours = 
Integer.parseInt(config.get(DaemonConfig.STORM_ROCKSDB_METRIC_RETENTION_HOURS).toString());
+Integer retentionPeriod = 0;
+if 
(config.containsKey(DaemonConfig.STORM_ROCKSDB_METRIC_RETENTION_PERIOD_HOURS)) {
+retentionPeriod = 
Integer.parseInt(config.get(DaemonConfig.STORM_ROCKSDB_METRIC_RETENTION_PERIOD_HOUR

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160242577
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -4166,4 +4184,23 @@ public void shutdown() {
 public boolean isWaiting() {
 return timer.isTimerWaiting();
 }
+
+@Override
+public void processWorkerMetrics(WorkerMetrics metrics) throws 
org.apache.thrift.TException {
+if (this.metricsStore == null) {
--- End diff --

Can we add in some authorization calls before we do anything with executing 
the command?

```
checkAuthorization(null, null, "processWorkerMetrics");
```

Then we would need to update 


https://github.com/apache/storm/blob/7ecb3d73e8e909c01d39e03a7a7ed45a2fb81859/storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java#L52

to have processWorkerMetrics in the list.



---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160240938
  
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -1023,6 +1023,43 @@
 public static String STORM_SUPERVISOR_MEDIUM_MEMORY_GRACE_PERIOD_MS =
 "storm.supervisor.medium.memory.grace.period.ms";
 
+/**
+ * Class implementing MetricStore.
+ */
+@NotNull
+@isString
--- End diff --

We have an `@isImplementationOfClass` annotation that I think would be more 
appropriate for this config.


---

[GitHub] storm pull request #2504: STORM-2156: store metrics into RocksDB

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2504#discussion_r160241831
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -1081,6 +1088,14 @@ public Nimbus(Map conf, INimbus 
inimbus, IStormClusterState stor
 BlobStore blobStore, TopoCache topoCache, ILeaderElector 
leaderElector, IGroupMappingServiceProvider groupMapper)
 throws Exception {
 this.conf = conf;
+
+this.metricsStore = null;
+try {
+this.metricsStore = MetricStoreConfig.configure(conf);
+} catch (Exception e) {
+LOG.error("Failed to initialize metric store", e);
--- End diff --

Could we add in a comment that the metrics store is not critical to the 
operation of the cluster so if it does not come up we will not stop the cluster 
from coming up.


---

[GitHub] storm pull request #2505: STORM-2877: Add an option to configure pagination ...

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2505#discussion_r160239743
  
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -307,6 +307,12 @@
 @isString
 public static final String UI_CENTRAL_LOGGING_URL = 
"ui.central.logging.url";
 
+/**
+ * Storm UI drop-down pagination value.
--- End diff --

Could you also add in some example values that can be set in the javadocs.  
Just so people know what to do.


---

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

[revans2]

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2505
  
@srishtyagrawal yes my concern was mostly with the number of times that the 
REST calls were being hit.  If you can reduce it so no new rest calls come in, 
that would be great.


---

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

[revans2]

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2505
  
@HeartSaVioR That should be very simple to do.  Right now we are storing 
the data in session storage.


https://github.com/apache/storm/blob/7ecb3d73e8e909c01d39e03a7a7ed45a2fb81859/storm-core/src/ui/public/component.html#L147-L153

We could switch it over to something more permanent, that would survive 
reboots.  The reason we didn't do that before is because we store the data 
per-topology for some pages.


https://github.com/apache/storm/blob/7ecb3d73e8e909c01d39e03a7a7ed45a2fb81859/storm-core/src/ui/public/component.html#L140

And keeping that data permanently would be bad for users as it would be a 
memory leak for them.

We could switch it to permanent for non-topology specific pages (a few 
lines of change).  Also if you really want to we could add a new page to switch 
the defaults for this browser on all topology pages, but that feels a bit more 
difficult to get right, and harder to maintain if the page layout changes.


---

Re: [DISCUSS] Release Storm 1.0.5 / 1.1.2

[P. Taylor Goetz]

+1

If there are no remaining issues to be included, we can start the release 
process.

-Taylor

> On Jan 7, 2018, at 7:07 PM, Jungtaek Lim  wrote:
> 
> Bump, does someone have issues which are necessary to be included in Storm
> 1.1.2? If not I think we should start release phase for 1.1.2 soon.
> 
> -Jungtaek Lim (HeartSaVioR)
> 
> 2017년 12월 28일 (목) 오후 3:16, Jungtaek Lim 님이 작성:
> 
>> I have been really busy so couldn't care about releases, and now I got
>> some time period to track again.
>> 
>> We have been delaying new release, since we have been focusing on 1.2.0
>> and issues relevant in storm-kafka-client have been raised continuously.
>> (though things looks like going to be less critical)
>> 
>> But other than storm-kafka-client issues, I think Storm 1.1.2 is ready to
>> be released, and we should release Storm 1.1.2 regardless of Storm 1.2.0
>> because we have fixed another critical issues in core (STORM-2231[1],
>> STORM-2682[2]) which are published to 1.0.5 but no release in 1.1.x version
>> line yet.
>> (Noting that some bugfixes on storm-kafka-client are not ported back to
>> 1.1.x version line because of heavy divergence.)
>> 
>> It may not be good time to discuss since it is year-end now, but would
>> like to remind this so that we could start the process at least earlier in
>> next year.
>> 
>> Thanks,
>> Jungtaek Lim (HeartSaVioR)
>> 
>> 1. https://issues.apache.org/jira/browse/STORM-2231
>> 2. https://issues.apache.org/jira/browse/STORM-2682
>> 
>> 2017년 10월 19일 (목) 오전 1:19, Stig Rohde Døssing 님이
>> 작성:
>> 
>>> Looks like Hugo is working on it
>>> https://issues.apache.org/jira/browse/STORM-2781
>>> 
>>> 2017-10-18 4:22 GMT+02:00 Jungtaek Lim :
>>> 
> I'm hoping the delay for 1.2.0 will be very short. The changes we
 discussed
> were minor, and had to do with renaming some of the new methods and
> constants. It would be good to do before 1.2.0 because the renames are
> breaking changes.
 
 Suppose the changes will be minor, then why not go ahead making the
>>> change?
 I guess it doesn't need much efforts to do.
 (Will Hugo submit the patch?)
 
 I'm +1 to get it before 1.2.0, and also hope that the delay will be very
 short.
 
 2017년 10월 14일 (토) 오후 6:05, Alexandre Vermeerbergen <
 avermeerber...@gmail.com>님이
 작성:
 
> +1 for a short delay until 1.2.0 is avaible :)
> 
> 2017-10-14 8:48 GMT+02:00 Stig Rohde Døssing >>> :
> 
>> I'm hoping the delay for 1.2.0 will be very short. The changes we
> discussed
>> were minor, and had to do with renaming some of the new methods and
>> constants. It would be good to do before 1.2.0 because the renames
>>> are
>> breaking changes.
>> 
>> 2017-10-14 5:33 GMT+02:00 Arun Mahadevan :
>> 
>>> I was hoping we will get 1.2.0 out along with 1.1.2. The pending
 issues
>> in
>>> the epic https://issues.apache.org/jira/browse/STORM-2710 seems
>>> to
> have
>>> been addressed. Can you add the new issue to the epic?
>>> 
>>> If its not something critical we can do it in a minor release post
> 1.2.0.
>>> 
>>> Thanks,
>>> Arun
>>> 
>>> 
>>> On 10/14/17, 3:50 AM, "Hugo Da Cruz Louro" <
>>> hlo...@hortonworks.com>
>> wrote:
>>> 
 I am +1 to releasing 1.1.2 right away. I am in the middle of one
> review
>>> but I will finish it in the next day, such that we can get this
 merged
>> soon.
 
 However, we need to hold onto releasing 1.2.0 until some of the
> changes
>>> for ProcessingGuarantee that got in this patch<
>>> https://github.com/
>>> apache/storm/commit/48f6969027e7b02a5b9220577189d3911aa2226d> are
> fixed.
>>> I briefly discussed [1] this issue with @Stig on Gitter, I will
 submit
> a
>>> patch with the change.
 
 Thanks,
 Hugo
 [1] - We did not have a technical discussion. I just asked a
>>> couple
 of
>>> clarifying questions and then the idea surged that we should
>>> improve
> some
>>> of the changes in this  patch>> 48f6969027e7b02a5b9220577189d3911aa2226d>. I will create a JIRA,
>>> and
> all
>>> the discussion go through either JIRA or dev email list.
 
 On Oct 10, 2017, at 12:48 PM, Stig Rohde Døssing <
>> stigdoess...@gmail.com<
>>> mailto:stigdoess...@gmail.com>> wrote:
 
 Thanks Jungtaek, that sounds like a good plan. Here's the new PR
>>> for
>> 2607
 https://github.com/apache/storm/pull/2367.
 
 Beginning release next week sounds good to me.
 
 2017-10-10 17:42 GMT+02:00 Arun Mahadevan >> > arunm
>>> @apache.org>>:
 
 +1 for addressing the pending reviews and getting 1.2.0 out soon.
 
 
 
 
 On 10/10/17, 6:14 AM, "Jungtaek Lim" >> kabh
>>> w

[GitHub] storm pull request #2507: STORM-2885: Avoid conflicts with nimbusDaemon Loca...

[revans2]

GitHub user revans2 opened a pull request:

https://github.com/apache/storm/pull/2507

STORM-2885: Avoid conflicts with nimbusDaemon LocalCluster Tests



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/revans2/incubator-storm STORM-2885

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/2507.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 #2507


commit 8f70777bee43d8add02d659d689076c15540976c
Author: Robert (Bobby) Evans 
Date:   2018-01-08T19:21:44Z

STORM-2885: Avoid conflicts with nimbusDaemon LocalCluster Tests




---

Re: [Discuss] Release Storm 1.2.0 (cont.)

[Arun Mahadevan]

+1 to start the release process once STORM-2153 is merged. 

If STORM-2860 can be merged soon we can include that as well.

Thanks,
Arun




On 1/7/18, 4:14 PM, "Jungtaek Lim"  wrote:

>Now we merged STORM-2867 and STORM-2869.
>
>Remaining issues are STORM-2153 and STORM-2860, and STORM-2860 doesn't seem
>to bring benefit on 1.x version line hence unless I'm missing here, we just
>need to make sure STORM-2153 is resolved so that we could start release
>phase for Storm 1.2.0.
>
>- Jungtaek Lim (HeartSaVioR)
>
>2017년 12월 29일 (금) 오전 4:51, Arun Mahadevan 님이 작성:
>
>> >STORM-2869: KafkaSpout discards all pending record when adjusting the
>> >consumer position after a commit [1]
>>
>> Hope we could get it merged this week or early next week.
>>
>>
>> >>New Feature
>> >STORM-2153: New Metrics Reporting API [2]
>>
>> I think this is waiting for a final +1 from revans2.
>>
>>
>> >STORM-2867: Add consumer lag metrics to KafkaSpout [3]
>>
>>
>> If required we can call out the kafka dependency since its a minor version
>> change. It may not be an issue if we use the reflection workaround proposed
>> in the PR ?
>>
>> IMO, it will be ideal to start the release process for 1.2.0 in the first
>> week of Jan after the above three are addressed.
>>
>> Thanks,
>> Arun
>>
>>
>>
>> On 12/27/17, 11:46 PM, "Jungtaek Lim"  wrote:
>>
>> >Looks like we got lost the chance to make release phase be started in
>> 2017,
>> >but I think we are really close to be sure we could start the process in
>> >early Jan. 2018.
>> >
>> >We haven't had "feature freeze" before releasing, so typically we still
>> >have a chance to get more features in Storm 1.2.0.
>> >
>> >So far, what we have remaining issues for Storm 1.2.0:
>> >
>> >> Bug
>> >STORM-2869: KafkaSpout discards all pending record when adjusting the
>> >consumer position after a commit [1]
>> >
>> >The PR for master got +1, so once we have PR for 1.x-branch, we could go
>> on
>> >merging. I expect this will be done in several days, unless Stig is going
>> >for long vacation.
>> >
>> >> New Feature
>> >STORM-2153: New Metrics Reporting API [2]
>> >
>> >This is likely waiting for final review, so I expect this patch to be
>> >finished within couple of weeks (early Jan. 2018), and if not I'd like to
>> >propose moving out of 1.2.0.
>> >
>> >STORM-2867: Add consumer lag metrics to KafkaSpout [3]
>> >
>> >The patch looks good, but it requires Kafka dependency to be updated from
>> >0.10.0.0 to 0.10.1.0 which might make Kafka 0.10.0.x user unable to use
>> >storm-kafka-client in Storm 1.2.0. Do we want to have a poll, or would it
>> >be not a big deal?
>> >
>> >STORM-2860: Add Kerberos support to Solr bolt [4]
>> >
>> >This patch breaks backward compatibility and we are discussing about
>> >alternative way to not break backward compatibility for 1.2.0. If we can
>> >get alternative in time, we could bring it to Storm 1.2.0, but if not, it
>> >should not block the release.
>> >
>> >Please participate reviewing, or provide any missing issues for Storm
>> >1.2.0, or give opinions on Storm 1.2.0.
>> >
>> >Thanks,
>> >Jungtaek Lim (HeartSaVioR)
>> >
>> >1. https://issues.apache.org/jira/browse/STORM-2869
>> >2. https://issues.apache.org/jira/browse/STORM-2153
>> >3. https://issues.apache.org/jira/browse/STORM-2867
>> >4. https://issues.apache.org/jira/browse/STORM-2860
>> >
>> >2017년 12월 18일 (월) 오전 3:50, Stig Rohde Døssing 님이
>> 작성:
>> >
>> >> Alexandre,
>> >>
>> >> There are a couple more issues pending, see
>> >> https://issues.apache.org/jira/browse/STORM-2710. It might be easier if
>> >> you
>> >> build the code yourself. There's a guide at
>> >>
>> >>
>> https://github.com/apache/storm/blob/master/DEVELOPER.md#create-a-storm-distribution-packaging
>> >> .
>> >> The only change you should need to make is to run "mvn package
>> -Dgpg.skip"
>> >> instead of "mvn package" in the storm-dist directory to skip the GPG
>> >> signature part.
>> >>
>> >> 2017-12-17 15:09 GMT+01:00 Alexandre Vermeerbergen <
>> >> avermeerber...@gmail.com
>> >> >:
>> >>
>> >> > Hello Storm developers,
>> >> >
>> >> > Now that I see that everything planned for Storm 1.2.0 release is done
>> >> (as
>> >> > I see at
>> https://issues.apache.org/jira/projects/STORM/versions/12341047
>> >> ),
>> >> > would it be possible to have new binaries for us to assess this new
>> >> release
>> >> > non-regression?
>> >> >
>> >> > In particular, I would like to check whether or not the bizarre
>> >> capacities
>> >> > metrics I get with the 1+ month-old Storm 1.2.0 preview are still
>> there.
>> >> >
>> >> > Best regards,
>> >> > Alexandre
>> >> >
>> >> >
>> >> > 2017-12-09 10:38 GMT+01:00 Alexandre Vermeerbergen <
>> >> > avermeerber...@gmail.com
>> >> > >:
>> >> >
>> >> > > Hello Storm developers,
>> >> > >
>> >> > > It's been about 2 weeks that I running Storm 1.2.0 preview with 15
>> >> > > topologies, 6 Supervisors, 1 Nimbus, 3 Zookeepers, and Kafka 0.10
>> libs
>> >> > all
>> >> > > with Storm Kafka client spout instead of our ow

[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB

[agresch]

Github user agresch commented on the issue:

https://github.com/apache/storm/pull/2504
  
Further metrics can be added to the database as desired.  The intent of 
this patch was to get the database functional.  Given the patch size and 
ongoing metrics work, I would suggest adding more metrics as a follow on JIRA.


---

Re: [DISCUSS] Regarding support old Storm workers in Storm 2.0.0

[Alexandre Vermeerbergen]

Hello Jungtaek,

+1 for a distributed snapshot support for Storm !

Regarding breaking workers compatibility, on my side that wouln't be a big
deal, as we do not yet do "rolling upgrades" of our Storm clusters.

Even do we where doing rolling upgrades for normal upgrades, getting such a
great improvement such as distributed snapshot would be a good reason to
make a "cold upgrade" of our clusters.

Thanks,
Alexandre Vermeerbergen


2018-01-08 0:53 GMT+01:00 Jungtaek Lim :

> Hi devs,
>
> We have added a feature regarding support old Storm workers in Storm 2.0.0
> via STORM-2448 [1] which was OK to me before addressing metrics issue, but
> for now I think it worths to discuss.
>
> STORM-2448 assumes we have backward compatible interaction between daemons
> (Nimbus/Supervisor/etc.) and worker in Storm 2.0.0. It is not only for
> interaction via thrift, but also for interaction via any ways including
> Zookeeper.
>
> STORM-2693[2] came in as nice improvement, which changes the mechanism of
> heartbeat (replace ZK with thrift RPC for interprocess heartbeat transfer)
> and it is not compatible with old Storm workers. (We are still be able to
> make it as backward compatible via letting Nimbus also support old style
> heartbeat - reading ZK periodically, but it clearly reduces the performance
> gain.)
>
> Now I can see a patch for STORM-2156[3], which stores metrics into RocksDB,
> but worker metrics are not addressed yet. I guess it will depend on Metrics
> V2 (STORM-2153)[4] and regardless of dependent, if STORM-2156 would want to
> change the approach of publishing metric from workers (via thrift RPC), it
> will be also backward incompatible (same reason as STORM-2693).
>
> We should break backward compatibility eventually to enjoy full benefits on
> this (and others if we have similar improvements), and I'm not sure why it
> can't be at Storm 2.0.0 (major release, nearly 2 years after 1.0.0). Some
> users might be upset with backward incompatibility, but I don't think they
> would not be upset we postpone the breaking changes and finally bring them
> to Storm 3.0.0.
>
> I would like to hear everyone's opinions regarding how to handle this
> situation. We might have some workarounds which makes us bring both
> features but with reducing effects.
>
> Thanks,
> Jungtaek Lim (HeartSaVioR)
>
> 1. https://issues.apache.org/jira/browse/STORM-2448
> 2. https://issues.apache.org/jira/browse/STORM-2693
> 3. https://issues.apache.org/jira/browse/STORM-2156
> 4. https://issues.apache.org/jira/browse/STORM-2153
>
> ps. I imagine that how our consensus goes for this situation: if we could
> bring much improvements but only breaking backward compatible way. One
> possible change would be dropping Acker mechanism and adopting distributed
> snapshot: I have been thinking this as worth to do, and JStorm already made
> a change to bring performance gain and also get advantage while windowing.
>