[GitHub] storm issue #2712: Update Multilang-protocol.md

2018-06-14 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2712
  
@MaurGi OK never mind. I'll handle it during merging. Thanks for the 
contribution!


---


[GitHub] storm issue #2712: Update Multilang-protocol.md

2018-06-14 Thread MaurGi
Github user MaurGi commented on the issue:

https://github.com/apache/storm/pull/2712
  
@HeartSaVioR how do I squash an existing PR in GitHub? I don't see any 
option on the UI, do I make a new branch? thanks -


---


[GitHub] storm issue #2714: STORM-3101: Fix unexpected metrics registration in StormM...

2018-06-14 Thread zd-project
Github user zd-project commented on the issue:

https://github.com/apache/storm/pull/2714
  
Another implementation would be to require all metrics to be registered 
when at least one instance of a class has been instantiated. This will 
disqualify the final property of all metrics variables but we can encapsulate 
all the assignment to a static method and invoked at appropriate time.

For example,

```
class Scratch {
private static Integer x = null;

public Scratch() {
if (Scratch.x == null) {
x = 7;
}
}
}
```


---


[GitHub] storm issue #2714: STORM-3101: Fix unexpected metrics registration in StormM...

2018-06-14 Thread Ethanlm
Github user Ethanlm commented on the issue:

https://github.com/apache/storm/pull/2714
  
Talked with @zd-project offline, I now have a better understand of the 
problem he is trying to solve as explained in 
https://issues.apache.org/jira/browse/STORM-3101. It's a really good catch. 
But we can discuss more about how to address it.


---


[GitHub] storm pull request #2719: STORM-3105 Update hive.version to stable hive rele...

2018-06-14 Thread aandis
GitHub user aandis opened a pull request:

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

STORM-3105 Update hive.version to stable hive release.

[STORM-3105](https://issues.apache.org/jira/browse/STORM-3105)

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

$ git pull https://github.com/aandis/storm STORM-3105

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

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


commit 16d3910101ae4be0db66cb6972c614b98572d098
Author: Abhishek 
Date:   2018-06-14T16:35:27Z

Update hive.version to last stable hive release.




---


[GitHub] storm issue #2714: STORM-3101: Add filter to StormMetricsRegistry.

2018-06-14 Thread Ethanlm
Github user Ethanlm commented on the issue:

https://github.com/apache/storm/pull/2714
  
Thanks for the contribution. It's good to exploit more on metrics 
functionality. 

I don't see the real need for adding filters since it's all storm internal 
code. But if you think it's better to have,  I would suggest to use a filter 
layer to make this more flexible. 

For example, 

Create a filter interface with a filter function. e.g.
```
public interface IStormMetricsRegistryFilter {
   public default boolean filter(String metricName) {
return false;
}
}
```
then you can have a function (e.g. addFilter) in StormMetricsRegistryFilter 
to add real implementation of the filter interface before StormMetricsRegistry 
starts to registerMeters().

The above is a very simple interface and might not be able to do much 
except filtering based on the `String` parameter. You can think about more on 
this.

I think the current implementation in this PR won't work because you are 
calling `setsource()` to late


---


[GitHub] storm issue #2660: STORM-3056: Add a test for quickly rebinding to a port

2018-06-14 Thread kishorvpatil
Github user kishorvpatil commented on the issue:

https://github.com/apache/storm/pull/2660
  
@raghavgautam Can you please address the merge conflicts?


---


[GitHub] storm issue #2718: STORM-3103 allow nimbus to shutdown properly

2018-06-14 Thread kishorvpatil
Github user kishorvpatil commented on the issue:

https://github.com/apache/storm/pull/2718
  
Good catch @agresch 


---


[GitHub] storm pull request #2714: STORM-3101: Add filter to StormMetricsRegistry.

2018-06-14 Thread zd-project
Github user zd-project commented on a diff in the pull request:

https://github.com/apache/storm/pull/2714#discussion_r195467563
  
--- Diff: 
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -73,6 +130,12 @@ private static void 
startMetricsReporter(PreparableReporter reporter, Map T register(String name, T metric) {
+if (source != null && !name.startsWith(source)) {
--- End diff --

Thanks for the clarification. Anyone else also has suggestions in the 
implementation?


---


[GitHub] storm issue #2660: STORM-3056: Add a test for quickly rebinding to a port

2018-06-14 Thread ghajos
Github user ghajos commented on the issue:

https://github.com/apache/storm/pull/2660
  
@raghavgautam I completely missed something with my test. Originally 
started a tcp server and a tcp client in separate processes and sent KILLTERM 
to the client side. The server port stayed in TIME_WAIT then started a new 
client to check if it can connect to the server side.  

When #STORM-3039 is landed on our lines and the original exception was 
still present, rechecked the test and realized that even without the change the 
new client could connect to the server at least on ubuntu16.

Sorry for the mistake!


---


[GitHub] storm issue #2714: STORM-3101: Add filter to StormMetricsRegistry.

2018-06-14 Thread zd-project
Github user zd-project commented on the issue:

https://github.com/apache/storm/pull/2714
  
Additional questions:
1. Is worker considered a separate daemon from supervisor? How should we 
differentiate worker's metrics from supervisor's metrics? For example, the 
worker's kill/restart operations are handled by supervisors. Does this make 
them supervisor's metrics?
2. Are DRPC and LogViewer considered server Daemon? Can I add them to the 
DaemonType enum?


---


[GitHub] storm issue #2718: STORM-3103 allow nimbus to shutdown properly

2018-06-14 Thread agresch
Github user agresch commented on the issue:

https://github.com/apache/storm/pull/2718
  
It seems not to, as Nimbus was able to continue the shutdown process 
cleanly.


---


[GitHub] storm issue #2718: STORM-3103 allow nimbus to shutdown properly

2018-06-14 Thread Ethanlm
Github user Ethanlm commented on the issue:

https://github.com/apache/storm/pull/2718
  
The code change makes sense to me. Just wonder if the whole JVM is 
terminated immediately after exitProcess() is being called. 


---