[GitHub] storm pull request #2366: [STORM-2686] Add locality awareness to LoadAwareSh...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] storm pull request #2366: [STORM-2686] Add locality awareness to LoadAwareSh...

[Ethanlm]

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

https://github.com/apache/storm/pull/2366#discussion_r143865842
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java ---
@@ -50,10 +69,28 @@
 volatile int[] choices;
 private volatile int[] prepareChoices;
 private AtomicInteger current;
+private Scope currentScope;
+private NodeInfo sourceNodeInfo;
+private List targetTasks;
+private AtomicReference> taskToNodePort;
+private Map conf;
+private DNSToSwitchMapping dnsToSwitchMapping;
+private Map localityGroup;
+private double HIGHER_BOUND;
--- End diff --

Thanks. Fixed it


---

[GitHub] storm pull request #2366: [STORM-2686] Add locality awareness to LoadAwareSh...

[HeartSaVioR]

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

https://github.com/apache/storm/pull/2366#discussion_r143862027
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java ---
@@ -50,10 +69,28 @@
 volatile int[] choices;
 private volatile int[] prepareChoices;
 private AtomicInteger current;
+private Scope currentScope;
+private NodeInfo sourceNodeInfo;
+private List targetTasks;
+private AtomicReference> taskToNodePort;
+private Map conf;
+private DNSToSwitchMapping dnsToSwitchMapping;
+private Map localityGroup;
+private double HIGHER_BOUND;
--- End diff --

This and below line are against style guide and check style will catch it. 
`higherBound` and `lowerBound`.


---

[GitHub] storm pull request #2366: [STORM-2686] Add locality awareness to LoadAwareSh...

[Ethanlm]

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

https://github.com/apache/storm/pull/2366#discussion_r143837500
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java ---
@@ -50,10 +61,28 @@
 volatile int[] choices;
 private volatile int[] prepareChoices;
 private AtomicInteger current;
+private AtomicReference currentScopeRef;
--- End diff --

I think you are right


---

[GitHub] storm pull request #2366: [STORM-2686] Add locality awareness to LoadAwareSh...

[revans2]

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

https://github.com/apache/storm/pull/2366#discussion_r143829089
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java ---
@@ -50,10 +61,28 @@
 volatile int[] choices;
 private volatile int[] prepareChoices;
 private AtomicInteger current;
+private AtomicReference currentScopeRef;
--- End diff --

I don't think this needs to be atomic.  Only `chooseTasks` needs to be 
thread safe.  All the others are called from a single thread, and we can put a 
lock around if we really want to.


---

[GitHub] storm pull request #2366: [STORM-2686] Add locality awareness to LoadAwareSh...

[revans2]

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

https://github.com/apache/storm/pull/2366#discussion_r143828158
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/task/WorkerTopologyContext.java ---
@@ -34,6 +36,8 @@
 private String _pidDir;
 Map _userResources;
 Map _defaultResources;
+private AtomicReference> _taskToNodePort;
+private String _assignmentId;
--- End diff --

Could we avoid adding in new member variables with _ as the first 
character.  This is against the new guidelines.


---

[GitHub] storm pull request #2366: [STORM-2686] Add locality awareness to LoadAwareSh...

[revans2]

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

https://github.com/apache/storm/pull/2366#discussion_r143826667
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/grouping/LoadAwareShuffleGrouping.java ---
@@ -20,14 +20,20 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import java.io.Serializable;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
+import java.util.*;
--- End diff --

We avoid .* imports, it is against the checkstyle guidelines we have.


---

[GitHub] storm pull request #2366: [STORM-2686] Add locality awareness to LoadAwareSh...

[Ethanlm]

GitHub user Ethanlm opened a pull request:

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

[STORM-2686] Add locality awareness to LoadAwareShuffleGrouping

A redesign of https://github.com/apache/storm/pull/2270 . This adds the 
locality awareness to LoadAwareShuffleGrouping. It applies the concept of 
Bang–bang control (credit to @revans2 ).

I didn't change the critical function like chooseTasks(). So the 
performance should be good. 

I am doing some performance testing. Hope to get some feedback first. 
Thanks.

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

$ git pull https://github.com/Ethanlm/storm STORM-2686-redesign

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

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


commit 379ea4cf872160ad29744c5729476e860d40fc14
Author: Ethan Li 
Date:   2017-10-09T23:41:39Z

[STORM-2686] Add locality awareness to LoadAwareShuffleGrouping




---