[GitHub] storm pull request #2487: [STORM-2873] Do not delete backpressure ephemeral ...

[kishorvpatil]

Github user kishorvpatil closed the pull request at:

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


---

[GitHub] storm pull request #2487: [STORM-2873] Do not delete backpressure ephemeral ...

[srdo]

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

https://github.com/apache/storm/pull/2487#discussion_r159097108
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/cluster/StormClusterStateImpl.java ---
@@ -437,18 +436,14 @@ public void supervisorHeartbeat(String supervisorId, 
SupervisorInfo info) {
 public void workerBackpressure(String stormId, String node, Long port, 
long timestamp) {
 String path = ClusterUtils.backpressurePath(stormId, node, port);
 boolean existed = stateStorage.node_exists(path, false);
+if (timestamp <= 0) {
--- End diff --

This check could be moved to the top of the function, if the timestamp is 
negative there's no reason to check if the znode exists.


---

[GitHub] storm pull request #2487: [STORM-2873] Do not delete backpressure ephemeral ...

[srdo]

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

https://github.com/apache/storm/pull/2487#discussion_r159093595
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/cluster/StormClusterStateImpl.java ---
@@ -438,9 +438,7 @@ public void workerBackpressure(String stormId, String 
node, Long port, long time
 String path = ClusterUtils.backpressurePath(stormId, node, port);
 boolean existed = stateStorage.node_exists(path, false);
 if (existed) {
-if (timestamp <= 0) {
-stateStorage.delete_node(path);
-} else {
+if (timestamp > 0) {
--- End diff --

The change seems reasonable to me. The javadoc for this method is slightly 
out of date now, consider updating the first line of it.

The two outermost branches here do mostly the same thing. Consider 
rewriting to something like 
```
if (timestamp <= 0) {
  return
}
String path = ...
boolean existed = ...
byte[] data = ...
if (existed) {
  set_data
} else {
  set_ephemeral_node
}
```


---

[GitHub] storm pull request #2487: [STORM-2873] Do not delete backpressure ephemeral ...

[kishorvpatil]

GitHub user kishorvpatil opened a pull request:

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

[STORM-2873] Do not delete backpressure ephemeral node frequently

If ephemeral znode is created once, then we can leave it as is - as other 
workers would look at timestamp to ensure it is not stale. This avoid 
deletion/creation of same ephemeral znode path at very high frequency.

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

$ git pull https://github.com/kishorvpatil/incubator-storm STORM-2873

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

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


commit 7f891fd6eaed1be5a9e238ff4d246ca195d37b21
Author: Kishor Patil 
Date:   2017-12-29T18:20:32Z

Do not delete backpressure ephemeral node frequently




---