[GitHub] storm pull request #2090: STORM-2489: Overlap and data loss on WindowedBolt ...

[satishd]

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

https://github.com/apache/storm/pull/2090#discussion_r113369572
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/windowing/TimeTriggerPolicy.java ---
@@ -109,13 +112,22 @@ private Runnable newTriggerTask() {
 return new Runnable() {
 @Override
 public void run() {
+// compute the current trigger timestamp based on 
prevTriggerTimestamp,
+// since the calculation based on System.currentTimeMillis 
might have a slight drift
+long now = System.currentTimeMillis();
+long triggerTs = prevTriggerTimestamp == 0 ? now : 
prevTriggerTimestamp + duration;
+prevTriggerTimestamp = triggerTs;
+if (Math.abs(triggerTs - now) > 1000) {
--- End diff --

OK, Throwing warning incase of large drifts looks fine for now.

Multiple concurrent executes will not happen as there is only one thread 
which executes the bolt and the subsequent bolt executions wait till the 
current execution is done.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2090: STORM-2489: Overlap and data loss on WindowedBolt ...

[satishd]

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

https://github.com/apache/storm/pull/2090#discussion_r113369191
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/windowing/TimeTriggerPolicy.java ---
@@ -62,7 +63,9 @@ public void reset() {
 
 @Override
 public void start() {
-executorFuture = executor.scheduleAtFixedRate(newTriggerTask(), 
duration, duration, TimeUnit.MILLISECONDS);
+// initial delay is slightly less than the duration so that the 
initial tuples wont't expire due to time drift
+long initialDelay = duration - Math.min((long) (duration * .1), 
100);
+executorFuture = executor.scheduleAtFixedRate(newTriggerTask(), 
initialDelay, duration, TimeUnit.MILLISECONDS);
--- End diff --

subsequentTrigger(x) = initialDelay + x*(delay)
Right, but the subsequent triggers may happen earlier as the initialDelay 
is slightly reduced.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2090: STORM-2489: Overlap and data loss on WindowedBolt ...

[arunmahadevan]

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

https://github.com/apache/storm/pull/2090#discussion_r113368411
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/windowing/TimeTriggerPolicy.java ---
@@ -62,7 +63,9 @@ public void reset() {
 
 @Override
 public void start() {
-executorFuture = executor.scheduleAtFixedRate(newTriggerTask(), 
duration, duration, TimeUnit.MILLISECONDS);
+// initial delay is slightly less than the duration so that the 
initial tuples wont't expire due to time drift
+long initialDelay = duration - Math.min((long) (duration * .1), 
100);
+executorFuture = executor.scheduleAtFixedRate(newTriggerTask(), 
initialDelay, duration, TimeUnit.MILLISECONDS);
--- End diff --

Subsequent scheduling will happen after "duration" which would be the 
window sliding interval.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2090: STORM-2489: Overlap and data loss on WindowedBolt ...

[arunmahadevan]

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

https://github.com/apache/storm/pull/2090#discussion_r113368417
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/windowing/TimeTriggerPolicy.java ---
@@ -109,13 +112,22 @@ private Runnable newTriggerTask() {
 return new Runnable() {
 @Override
 public void run() {
+// compute the current trigger timestamp based on 
prevTriggerTimestamp,
+// since the calculation based on System.currentTimeMillis 
might have a slight drift
+long now = System.currentTimeMillis();
+long triggerTs = prevTriggerTimestamp == 0 ? now : 
prevTriggerTimestamp + duration;
+prevTriggerTimestamp = triggerTs;
+if (Math.abs(triggerTs - now) > 1000) {
--- End diff --

The timing issues are due to the scheduled executor (which uses nanoTime) 
which does not exactly trigger after the duration when measured with 
System.currentTimeMillis. Theres a mismatch of a few ms, which is addressed 
here. In general user code is expected to return before the next trigger. If 
user code takes more time than the trigger interval still we set the next 
trigger timestamp correctly and execute as soon as they return from execute, 
but this should be very rare and users are not expected to do this, so we log 
the warning. I don't think we want multiple concurrent executes (i.e trigger 
execute when they haven't returned from the first execute)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2089: STORM-2490: Lambda support

[vesense]

Github user vesense commented on the issue:

https://github.com/apache/storm/pull/2089
  
@srdo Addressed your comments. Please take a look again.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2090: STORM-2489: Overlap and data loss on WindowedBolt ...

[satishd]

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

https://github.com/apache/storm/pull/2090#discussion_r113366037
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/windowing/TimeTriggerPolicy.java ---
@@ -109,13 +112,22 @@ private Runnable newTriggerTask() {
 return new Runnable() {
 @Override
 public void run() {
+// compute the current trigger timestamp based on 
prevTriggerTimestamp,
+// since the calculation based on System.currentTimeMillis 
might have a slight drift
+long now = System.currentTimeMillis();
+long triggerTs = prevTriggerTimestamp == 0 ? now : 
prevTriggerTimestamp + duration;
+prevTriggerTimestamp = triggerTs;
+if (Math.abs(triggerTs - now) > 1000) {
--- End diff --

Did we think about an option to execute a bolt in a different thread to 
avoid timing issues in trigger because of user code?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2090: STORM-2489: Overlap and data loss on WindowedBolt ...

[satishd]

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

https://github.com/apache/storm/pull/2090#discussion_r113363343
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/windowing/TimeTriggerPolicy.java ---
@@ -62,7 +63,9 @@ public void reset() {
 
 @Override
 public void start() {
-executorFuture = executor.scheduleAtFixedRate(newTriggerTask(), 
duration, duration, TimeUnit.MILLISECONDS);
+// initial delay is slightly less than the duration so that the 
initial tuples wont't expire due to time drift
+long initialDelay = duration - Math.min((long) (duration * .1), 
100);
+executorFuture = executor.scheduleAtFixedRate(newTriggerTask(), 
initialDelay, duration, TimeUnit.MILLISECONDS);
--- End diff --

If the initial delay is slightly less than the given duration then its 
subsequent scheduling will happen after the given period of initialDelay which 
can be earlier than expected. Is not it?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2089: STORM-2490: Lambda support

[vesense]

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

https://github.com/apache/storm/pull/2089#discussion_r113364466
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/lambda/LambdaBiConsumerBolt.java ---
@@ -0,0 +1,36 @@
+/**
+ * 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.lambda;
+
+import org.apache.storm.topology.BasicOutputCollector;
+import org.apache.storm.tuple.Tuple;
+
+public class LambdaBiConsumerBolt extends AbstractLambdaBolt {
+
+private SerializableBiConsumer biConsumer;
+
+public 
LambdaBiConsumerBolt(SerializableBiConsumer 
biConsumer) {
--- End diff --

I prefer to keep the current names. Both Consumer and BiConsumer are 
functional interfaces accepting parameters and returning nothing, these two 
bolts are corresponding processor. And users don't access them directly when 
writing spout/bolt by lambda.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2089: STORM-2490: Lambda support

[vesense]

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

https://github.com/apache/storm/pull/2089#discussion_r113364472
  
--- Diff: 
examples/storm-starter/src/jvm/org/apache/storm/starter/LambdaTopology.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 org.apache.storm.starter;
+
+import org.apache.storm.Config;
+import org.apache.storm.topology.ConfigurableTopology;
+import org.apache.storm.topology.TopologyBuilder;
+import org.apache.storm.tuple.Values;
+
+import java.util.UUID;
+
+public class LambdaTopology extends ConfigurableTopology {
+public static void main(String[] args) {
+ConfigurableTopology.start(new LambdaTopology(), args);
+}
+
+@Override
+protected int run(String[] args) throws Exception {
+TopologyBuilder builder = new TopologyBuilder();
+
+// example. spout1: generate random strings
+// bolt1: get the first part of a string
+// bolt2: output the tuple
+builder.setSpout("spout1", () -> UUID.randomUUID().toString());
--- End diff --

Variable used in lambda expression should be final or effectively final (or 
it will cause compilation error), and variable type should implement the 
Serializable interface if it isn't primitive type (or it will cause not 
serializable exception).

@srdo Thanks for your reminding. I will add some NOTE to remind users.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2089: STORM-2490: Lambda support

[vesense]

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

https://github.com/apache/storm/pull/2089#discussion_r113364469
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/topology/TopologyBuilder.java ---
@@ -316,6 +322,68 @@ public BoltDeclarer setBolt(String id, IWindowedBolt 
bolt, Number parallelism_hi
 }
 
 /**
+ * Define a new bolt in this topology. This defines a lambda basic 
bolt, which is a
+ * simpler to use but more restricted kind of bolt. Basic bolts are 
intended
+ * for non-aggregation processing and automate the anchoring/acking 
process to
+ * achieve proper reliability in the topology.
+ *
+ * @param id the id of this component. This id is referenced by other 
components that want to consume this bolt's outputs.
+ * @param biConsumer lambda expression which is the instance of 
functional interface BiConsumer
--- End diff --

Good catch. Will update.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2083: STORM-2421: support lists of childopts in DaemonConfig.

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2083
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2082: expose Tuple for node.js

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2082
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2084: STORM-2488: The UI user Must be HTTP

[asfgit]

Github user asfgit closed the pull request at:

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


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2086: STORM-2491: Adding extra Cassandra configuration paramete...

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2086
  
overall LGTM. +1 . 
@tandrup  would like to see these configs documented here 
https://github.com/apache/storm/blob/master/external/storm-cassandra/README.md 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2086: STORM-2491: Adding extra Cassandra configuration paramete...

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2086
  
@tandrup for small docs changes etc. we don't file JIRAs but this one had 
quite few changes good to have that in JIRA and subsequently in CHANGELOG. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2089: STORM-2490: Lambda support

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2089
  
@vesense this looks good. +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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2066: [STORM-2472] kafkaspout should work normally in kerberos ...

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2066
  
@liu-zhaokun following upon my previous comment , do not use 
stom-kafka-client from 1.0.x-branch as there are lot of bug-fixes went into 
storm-kafka-client in Storm 1.1 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2066: [STORM-2472] kafkaspout should work normally in kerberos ...

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2066
  
@liu-zhaokun I am still not sure why are we adding this code. One shouldn't 
be using 0.9 kafka clients and this storm-kafka-client will not work with 0.9 
as the interface changed in 0.10 and we made changes to storm-kafka-client to 
start working only from 0.10 kafka clients only. As I said before 0.9 kafka 
clients shouldn't be used as they are alpha quality.
If you look the kafka client docs https://kafka.apache.org/documentation/ , 
 you'll see "**sasl.jaas.config"** this is consumer config one can pass and the 
storm-kafka-client will allow you to pass this config. So there is necessity to 
add this code.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2058: [STORM-2466] The example of jaas.conf in jaas_kerberos.co...

[liu-zhaokun]

Github user liu-zhaokun commented on the issue:

https://github.com/apache/storm/pull/2058
  
@vesense 
Hi, I am sorry to bother you,could you help me to review it?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2066: [STORM-2472] kafkaspout should work normally in kerberos ...

[liu-zhaokun]

Github user liu-zhaokun commented on the issue:

https://github.com/apache/storm/pull/2066
  
@vesense 
Hello,could you help me to review this PR?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: [DISCUSS] Code Style

[P. Taylor Goetz]

Code convention discussions can get pretty heated/religious in nature, which we 
should seek to avoid. I'm largely in alignment with Hugo on this: Start with a 
good base like the Google or Sun/Oracle style guides, and establish consensus 
on where we feel a desire to differ.

Rather than haggle over certain individual conventions, I propose we choose an 
existing style guide to base ours on and tweak it as we see fit. I have 
definite opinions about java code style, but I'll hold off on sharing them for 
now.

Let's pick a starting point first. I believe the google and sun/oracle guides 
are both good starting points. Let's also try not to drag this out too much. 
I've seen similar conversations absorb more time and effort than it would take 
to update the associated codebase.

-Taylor

> On Apr 25, 2017, at 7:13 PM, Hugo Da Cruz Louro  
> wrote:
> 
> Hi,
> 
> We should just follow the Google Java Style 
> Guide. There are many 
> opinions on this subject, but I conjecture that this guide, which is very 
> similar to the original Sun Java Code 
> Conventions
>   has a lot of research and reasoning behind it. It’s widely adopted, and I 
> think that we should just follow it. The only exception I would really make 
> is the column limit. I would change 100 to 120 or 130, but not longer than 
> that. Very long lines makes the code really hard to read.
> 
> There are already existing check style, IntelliJ, and even Git hooks 
> enforcing/validating this style, so adopting it would make it easier to 
> incorporate it in the dev tools.
> 
> Regardless of the variants to the aforementioned style, I do not think that 
> we should use names starting with underscore _ in any circumstance. Not only 
> it is not a Java convention, as it does not add anything to the code. The 
> reason behind using _ was before the widespread uses of IDE’s, to mark class 
> fields. IDEs do that with smart coloring.
> 
> Thanks,
> Hugo
> 
> On Apr 25, 2017, at 3:50 PM, Jungtaek Lim 
> > wrote:
> 
> I would like to review the style guide of other projects, like HBase, and
> so.
> 
> Btw, IMHO, I don't like using underscore as prefix for fields. We're using
> Java and the expression "class member variable" is from C++, and also
> underscore style came from C++. We need to avoid mixing other languages'
> style as well.
> 
> Thanks,
> Jungtaek Lim (HeartSaVioR)
> 
> 2017년 4월 26일 (수) 오전 7:10, Kyle Nusbaum 
> >님이
> 작성:
> 
> Now that most of our code is in Java, I think it might be time to revisit
> the issue of having some official and enforced code style.
> I don't have very strong feelings about most of it, but here is what I was
> thinking as a start, since I've seen this style quite a bit, and I've found
> it makes code pretty easy to read:
> 1. Indentation is 4 spaces per level (no tabs)
> 2. All class member variables begin with underscore3. No wildcard
> imports4. if / else / for / etc. always get curly braces.
> There are obviously tons of other things, and we can get as picky as we
> want, but I think enforcing at least these rules would go a long way to
> making the code more consistent.
> 
> Thoughts?
> 
> Thanks,
> -- Kyle
> 

Re: [DISCUSS] Code Style

[Hugo Da Cruz Louro]

Hi,

We should just follow the Google Java Style 
Guide. There are many 
opinions on this subject, but I conjecture that this guide, which is very 
similar to the original Sun Java Code 
Conventions  
has a lot of research and reasoning behind it. It’s widely adopted, and I think 
that we should just follow it. The only exception I would really make is the 
column limit. I would change 100 to 120 or 130, but not longer than that. Very 
long lines makes the code really hard to read.

There are already existing check style, IntelliJ, and even Git hooks 
enforcing/validating this style, so adopting it would make it easier to 
incorporate it in the dev tools.

Regardless of the variants to the aforementioned style, I do not think that we 
should use names starting with underscore _ in any circumstance. Not only it is 
not a Java convention, as it does not add anything to the code. The reason 
behind using _ was before the widespread uses of IDE’s, to mark class fields. 
IDEs do that with smart coloring.

Thanks,
Hugo

On Apr 25, 2017, at 3:50 PM, Jungtaek Lim 
> wrote:

I would like to review the style guide of other projects, like HBase, and
so.

Btw, IMHO, I don't like using underscore as prefix for fields. We're using
Java and the expression "class member variable" is from C++, and also
underscore style came from C++. We need to avoid mixing other languages'
style as well.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 4월 26일 (수) 오전 7:10, Kyle Nusbaum 
>님이
작성:

Now that most of our code is in Java, I think it might be time to revisit
the issue of having some official and enforced code style.
I don't have very strong feelings about most of it, but here is what I was
thinking as a start, since I've seen this style quite a bit, and I've found
it makes code pretty easy to read:
1. Indentation is 4 spaces per level (no tabs)
2. All class member variables begin with underscore3. No wildcard
imports4. if / else / for / etc. always get curly braces.
There are obviously tons of other things, and we can get as picky as we
want, but I think enforcing at least these rules would go a long way to
making the code more consistent.

Thoughts?

Thanks,
-- Kyle

Re: [DISCUSS] Code Style

[Roshan Naik]

- Personally not a big fan of _ for members either from the readability 
standpoint. Also much of the existing code doesn’t follow this.
- Personally I find it is easier to write/read one-liner if/else without the 
curlys. Always wondered about the rationale behind that. 

Would suggest any such a coding convention be very minimalist as a conding 
convention’s usefulness is inversely proportional to its length (

-roshan

Would be good to keep any such coding convention as minimalistic and 

On 4/25/17, 3:50 PM, "Jungtaek Lim"  wrote:

I would like to review the style guide of other projects, like HBase, and
so.

Btw, IMHO, I don't like using underscore as prefix for fields. We're using
Java and the expression "class member variable" is from C++, and also
underscore style came from C++. We need to avoid mixing other languages'
style as well.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 4월 26일 (수) 오전 7:10, Kyle Nusbaum 님이
작성:

> Now that most of our code is in Java, I think it might be time to revisit
> the issue of having some official and enforced code style.
> I don't have very strong feelings about most of it, but here is what I was
> thinking as a start, since I've seen this style quite a bit, and I've 
found
> it makes code pretty easy to read:
> 1. Indentation is 4 spaces per level (no tabs)
> 2. All class member variables begin with underscore3. No wildcard
> imports4. if / else / for / etc. always get curly braces.
> There are obviously tons of other things, and we can get as picky as we
> want, but I think enforcing at least these rules would go a long way to
> making the code more consistent.
>
> Thoughts?
>
> Thanks,
> -- Kyle

Re: [DISCUSS] Code Style

[Jungtaek Lim]

I would like to review the style guide of other projects, like HBase, and
so.

Btw, IMHO, I don't like using underscore as prefix for fields. We're using
Java and the expression "class member variable" is from C++, and also
underscore style came from C++. We need to avoid mixing other languages'
style as well.

Thanks,
Jungtaek Lim (HeartSaVioR)

2017년 4월 26일 (수) 오전 7:10, Kyle Nusbaum 님이
작성:

> Now that most of our code is in Java, I think it might be time to revisit
> the issue of having some official and enforced code style.
> I don't have very strong feelings about most of it, but here is what I was
> thinking as a start, since I've seen this style quite a bit, and I've found
> it makes code pretty easy to read:
> 1. Indentation is 4 spaces per level (no tabs)
> 2. All class member variables begin with underscore3. No wildcard
> imports4. if / else / for / etc. always get curly braces.
> There are obviously tons of other things, and we can get as picky as we
> want, but I think enforcing at least these rules would go a long way to
> making the code more consistent.
>
> Thoughts?
>
> Thanks,
> -- Kyle

[DISCUSS] Code Style

[Kyle Nusbaum]

Now that most of our code is in Java, I think it might be time to revisit the 
issue of having some official and enforced code style.
I don't have very strong feelings about most of it, but here is what I was 
thinking as a start, since I've seen this style quite a bit, and I've found it 
makes code pretty easy to read:
1. Indentation is 4 spaces per level (no tabs)
2. All class member variables begin with underscore3. No wildcard imports4. if 
/ else / for / etc. always get curly braces.
There are obviously tons of other things, and we can get as picky as we want, 
but I think enforcing at least these rules would go a long way to making the 
code more consistent.

Thoughts?

Thanks,
-- Kyle

[GitHub] storm issue #2088: [STORM-2486] Prevent cd from printing target directory to...

[ptgoetz]

Github user ptgoetz commented on the issue:

https://github.com/apache/storm/pull/2088
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2088: [STORM-2486] Prevent cd from printing target directory to...

[hmcl]

Github user hmcl commented on the issue:

https://github.com/apache/storm/pull/2088
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2089: STORM-2490: Lambda support

[srdo]

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

https://github.com/apache/storm/pull/2089#discussion_r113218838
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/lambda/LambdaBiConsumerBolt.java ---
@@ -0,0 +1,36 @@
+/**
+ * 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.lambda;
+
+import org.apache.storm.topology.BasicOutputCollector;
+import org.apache.storm.tuple.Tuple;
+
+public class LambdaBiConsumerBolt extends AbstractLambdaBolt {
+
+private SerializableBiConsumer biConsumer;
+
+public 
LambdaBiConsumerBolt(SerializableBiConsumer 
biConsumer) {
--- End diff --

Nitpick: It might be clearer to name this and the LambdaConsumerBolt 
something like Lambda(Non)TerminalBolt?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2089: STORM-2490: Lambda support

[srdo]

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

https://github.com/apache/storm/pull/2089#discussion_r113237257
  
--- Diff: 
examples/storm-starter/src/jvm/org/apache/storm/starter/LambdaTopology.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 org.apache.storm.starter;
+
+import org.apache.storm.Config;
+import org.apache.storm.topology.ConfigurableTopology;
+import org.apache.storm.topology.TopologyBuilder;
+import org.apache.storm.tuple.Values;
+
+import java.util.UUID;
+
+public class LambdaTopology extends ConfigurableTopology {
+public static void main(String[] args) {
+ConfigurableTopology.start(new LambdaTopology(), args);
+}
+
+@Override
+protected int run(String[] args) throws Exception {
+TopologyBuilder builder = new TopologyBuilder();
+
+// example. spout1: generate random strings
+// bolt1: get the first part of a string
+// bolt2: output the tuple
+builder.setSpout("spout1", () -> UUID.randomUUID().toString());
--- End diff --

I'm wondering if it makes sense to add a note here to help people avoid the 
case where they refer to a field on LambdaTopology from the lambda and hit a 
NotSerializableException?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2089: STORM-2490: Lambda support

[srdo]

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

https://github.com/apache/storm/pull/2089#discussion_r113220738
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/topology/TopologyBuilder.java ---
@@ -316,6 +322,68 @@ public BoltDeclarer setBolt(String id, IWindowedBolt 
bolt, Number parallelism_hi
 }
 
 /**
+ * Define a new bolt in this topology. This defines a lambda basic 
bolt, which is a
+ * simpler to use but more restricted kind of bolt. Basic bolts are 
intended
+ * for non-aggregation processing and automate the anchoring/acking 
process to
+ * achieve proper reliability in the topology.
+ *
+ * @param id the id of this component. This id is referenced by other 
components that want to consume this bolt's outputs.
+ * @param biConsumer lambda expression which is the instance of 
functional interface BiConsumer
--- End diff --

Nitpick: This comment doesn't really say much about what the consumer 
is/does. Maybe something like "lambda expression that implements tuple 
processing for this bolt"?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2074: Storm 1290:port backtype.storm.local-state-test to java

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2074
  
+1. once the commits gets squashed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2074: Storm 1290:port backtype.storm.local-state-test to java

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2074
  
@kamleshbhatt can you please squash commits into single commit.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2088: [STORM-2486] Prevent cd from printing target directory to...

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2088
  
+1. Thanks @erikdw 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2090: STORM-2489: Overlap and data loss on WindowedBolt based o...

[harshach]

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2090
  
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #2085: STORM-2492: Adding Cassandra Object Mapper statement buil...

[tandrup]

Github user tandrup commented on the issue:

https://github.com/apache/storm/pull/2085
  
Thanks for the tip @vesense 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request #2090: STORM-2489: Overlap and data loss on WindowedBolt ...

[arunmahadevan]

GitHub user arunmahadevan opened a pull request:

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

STORM-2489: Overlap and data loss on WindowedBolt based on Duration

- Fixed time eviction to not evict events beyond current timestamp.
- Modified time trigger to compute the reference tim based on window 
duration.

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

$ git pull https://github.com/arunmahadevan/storm STORM-2489

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

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


commit 09cf77bb84ddcf5ab05757ac4f72ab1d20cf1e82
Author: Arun Mahadevan 
Date:   2017-04-25T07:07:43Z

STORM-2489: Overlap and data loss on WindowedBolt based on Duration

- Fixed time eviction to not evict events beyond current timestamp.
- Modified time trigger to compute the reference tim based on window 
duration.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---