[GitHub] [helix] mcvsubbu opened a new issue #377: Multiple problems with 0.9.0
mcvsubbu opened a new issue #377: Multiple problems with 0.9.0 URL: https://github.com/apache/helix/issues/377 We see the following issues in Pinot deployment: 1. Controller callback does not happen on a PARTICIPANT helix manager when leadership changes. 2. Other participants that host regular partitions do not get a state transition after the situation in previous step happens. The only solution is to restart the leader controller. 3. Task queues are stuck with multiple tasks in IN_PROGRESS state. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] i3wangyi opened a new pull request #376: Add #batchGetInstancesStoppableChecks to solve performance issue
i3wangyi opened a new pull request #376: Add #batchGetInstancesStoppableChecks to solve performance issue URL: https://github.com/apache/helix/pull/376 TODO, will update everything later This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] narendly opened a new issue #375: Drop all tasks whose requested state is DROPPED
narendly opened a new issue #375: Drop all tasks whose requested state is DROPPED URL: https://github.com/apache/helix/issues/375 Upon a Participant disconnect, the Participant would carry over from the last session. This would copy all previous task states to the current session and set their requested states as DROPPED (for INIT and RUNNING states). It came to our attention that sometimes these Participants experience connection issues and the tasks happen to be in TASK_ERROR or COMPLETED states. These tasks would get stuck on the Participant and never be dropped. This issue proposes to add the logic that would get all tasks whose requested states are DROPPED to be dropped immediately. See: JobDispatcher.java line 441. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] kaisun2000 commented on issue #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
kaisun2000 commented on issue #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug URL: https://github.com/apache/helix/pull/365#issuecomment-518453741 Resolved all the review request. Also passing the test. Next step is to merge into trunk This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310841129
##
File path:
helix-core/src/test/java/org/apache/helix/common/caches/TestCurrentStateSnapshot.java
##
@@ -0,0 +1,106 @@
+package org.apache.helix.common.caches;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.MockAccessor;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.LiveInstance;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+/**
+ * Unit test for {@link CurrentStateSnapshot}
+ */
+public class TestCurrentStateSnapshot {
Review comment:
added.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310838708
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
+logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
clusterName,
(System.currentTimeMillis() - startTime));
-notifyRoutingTableChange();
+
+// TODO: move the callback user code logic to separate thread upon routing
table statePropagation latency
+// integration test result. If the latency is more than 2 secs, we need to
change this part.
+notifyRoutingTableChange(clusterName);
// Update timestamp for last refresh
if (_isPeriodicRefreshEnabled) {
_lastRefreshTimestamp = System.currentTimeMillis();
}
}
- private void notifyRoutingTableChange() {
-for (Map.Entry entry :
_routingTableChangeListenerMap
-.entrySet()) {
- entry.getKey().onRoutingTableChange(new
RoutingTableSnapshot(_routingTableRef.get()),
- entry.getValue().getContext());
+ private void notifyRoutingTableChange(String clusterName) {
+// This call back is called in the main event queue of
RoutingTableProvider. We add log to record time spent
+// here. Potentially, we should call this callback in a separate thread if
this is a bottleneck.
+long startTime = System.currentTimeMillis();
+for (Map.Entry entry :
_routingTableChangeListenerMap.entrySet()) {
+ entry.getKey()
+ .onRoutingTableChange(new
RoutingTableSnapshot(_routingTableRef.get()), entry.getValue().getContext());
}
+logger.info("RoutingTableProvider callback [aka not helix time] for
cluster {}, take {} ms.", clusterName,
Review comment:
Helix Time == time spent in Helix side. Will change to
"RoutingTableProvider user callback time"
No. they will not overwhelm the log. One RoutingTableProvider will be
created by one cluster. The routingTableProvider change will not be that
frequent, definitely should be a lot less than all the pipeline running log. So
we should be safe here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310838708
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
+logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
clusterName,
(System.currentTimeMillis() - startTime));
-notifyRoutingTableChange();
+
+// TODO: move the callback user code logic to separate thread upon routing
table statePropagation latency
+// integration test result. If the latency is more than 2 secs, we need to
change this part.
+notifyRoutingTableChange(clusterName);
// Update timestamp for last refresh
if (_isPeriodicRefreshEnabled) {
_lastRefreshTimestamp = System.currentTimeMillis();
}
}
- private void notifyRoutingTableChange() {
-for (Map.Entry entry :
_routingTableChangeListenerMap
-.entrySet()) {
- entry.getKey().onRoutingTableChange(new
RoutingTableSnapshot(_routingTableRef.get()),
- entry.getValue().getContext());
+ private void notifyRoutingTableChange(String clusterName) {
+// This call back is called in the main event queue of
RoutingTableProvider. We add log to record time spent
+// here. Potentially, we should call this callback in a separate thread if
this is a bottleneck.
+long startTime = System.currentTimeMillis();
+for (Map.Entry entry :
_routingTableChangeListenerMap.entrySet()) {
+ entry.getKey()
+ .onRoutingTableChange(new
RoutingTableSnapshot(_routingTableRef.get()), entry.getValue().getContext());
}
+logger.info("RoutingTableProvider callback [aka not helix time] for
cluster {}, take {} ms.", clusterName,
Review comment:
Helix Time == time spent in Helix side. Will change to this term.
No. they will not overwhelm the log. One RoutingTableProvider will be
created by one cluster. The routingTableProvider change will not be that
frequent, definitely should be a lot less than all the pipeline running log. So
we should be safe here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on issue #374: Stablize the REST tests in helix-rest
dasahcc commented on issue #374: Stablize the REST tests in helix-rest URL: https://github.com/apache/helix/pull/374#issuecomment-518443325 > A lot of duplicate prints, can you check testng's BeforeMethod and AfterMethod annotations > https://dzone.com/articles/testng-aftermethod-annotation No. I tried that. It is not working. It only shows the "beforeMethod" or "afterMethod" name, since you dont have a way to memorize which method it is running. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] dasahcc commented on issue #374: Stablize the REST tests in helix-rest
dasahcc commented on issue #374: Stablize the REST tests in helix-rest URL: https://github.com/apache/helix/pull/374#issuecomment-518442369 > A lot of duplicate prints, can you check testng's BeforeMethod and AfterMethod annotations > https://dzone.com/articles/testng-aftermethod-annotation That's a good point. Let me change it. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310832882
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
+logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
clusterName,
(System.currentTimeMillis() - startTime));
-notifyRoutingTableChange();
+
+// TODO: move the callback user code logic to separate thread upon routing
table statePropagation latency
+// integration test result. If the latency is more than 2 secs, we need to
change this part.
Review comment:
Yes, storage team is doing integration test and they are blocked by this
one. 2 sec is perceived number from meeting with them
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310832625
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
+logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
clusterName,
Review comment:
changed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310832401
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
Review comment:
The code is always like this, maybe for the reason that some traditional
deployment code can indeed have helixManager to be null and people don't want
them to fail. I guess the most immediate previous change in the piece of code
was from you and it was still kept this way. So let us just keep it this way.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
kaisun2000 commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310831810
##
File path:
helix-core/src/main/java/org/apache/helix/common/caches/AbstractDataCache.java
##
@@ -48,15 +50,17 @@ public AbstractDataCache(ControlContextProvider
controlContextProvider) {
* Selectively fetch Helix Properties from ZK by comparing the version of
local cached one with the one on ZK.
* If version on ZK is newer, fetch it from zk and update local cache.
* @param accessor the HelixDataAccessor
- * @param reloadKeys keys needs to be reload
+ * @param reloadKeysIn keys needs to be reload
Review comment:
ReloadKeysIn means this is the input parameter, it is not changed
Note, later, there is a reloadedKeys, that is the output parameter with all
the keys that are reloaded.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc opened a new pull request #374: Stablize the REST tests in helix-rest
dasahcc opened a new pull request #374: Stablize the REST tests in helix-rest URL: https://github.com/apache/helix/pull/374 ### Issues For test stabilizing, not create an issue. ### Description - [ ] Here are some details about my PR, including screenshots of any UI changes: Stablize the REST tests by following changes: 1. Remove temporary cluster which impact the ClusterAccessor test 2. Add all start/end message for test debug purpose. 3. Disable unstable monitoring test for default MBeans. Sometimes we can query it sometimes not. It is not critical test path. Let's make it stable later. ### Tests mvn test [INFO] Tests run: 83, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 35.442 s - in TestSuite [INFO] [INFO] Results: [INFO] [INFO] Tests run: 83, Failures: 0, Errors: 0, Skipped: 0 [INFO] [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 42.434 s [INFO] Finished at: 2019-08-05T16:23:01-07:00 [INFO] Final Memory: 42M/1215M [INFO] This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] i3wangyi commented on issue #367: Add transient cache for CustomRestClient implementation
i3wangyi commented on issue #367: Add transient cache for CustomRestClient implementation URL: https://github.com/apache/helix/pull/367#issuecomment-518406925 > > > The parallel request still need to be considered. Say you are implementing the locking by keys. We need 100 participants partition level status called from single instance health check. Then you will read one instance partition level status after the other. It is still sequential read. > > > > > > The sequential read comes from how the upper class uses the CustomRestClientImpl. Currently, it is sequential but it's the scope of next PR. In this PR, my purpose is to add the cache implementation so we can change the implementation of the upper class (ServiceImpl) and make it parallel. > > We need to think about it as a whole piece. I can image that you parallel the requests by using thread pool for each key. But in that case, you may end up with have multiple idling threads hanging there because of locking on the same key. From the decouple point of view, the PR focuses on CustomRestClient's change and its responsibility IMO is to ensure the same request won't get called twice. It's the upper class' job to make the right parallel requests and I'm planning to address it later in the next PR piece by piece. Is your idea to dedupe the # of threads based on requests, in that case, the dedupe logic needs to be handled in the upper layer. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] dasahcc commented on issue #367: Add transient cache for CustomRestClient implementation
dasahcc commented on issue #367: Add transient cache for CustomRestClient implementation URL: https://github.com/apache/helix/pull/367#issuecomment-518402635 > > The parallel request still need to be considered. Say you are implementing the locking by keys. We need 100 participants partition level status called from single instance health check. Then you will read one instance partition level status after the other. It is still sequential read. > > The sequential read comes from how the upper class uses the CustomRestClientImpl. Currently, it is sequential but it's the scope of next PR. In this PR, my purpose is to add the cache implementation so we can change the implementation of the upper class (ServiceImpl) and make it parallel. We need to think about it as a whole piece. I can image that you parallel the requests by using thread pool for each key. But in that case, you may end up with have multiple idling threads hanging there because of locking on same key. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] i3wangyi commented on issue #367: Add transient cache for CustomRestClient implementation
i3wangyi commented on issue #367: Add transient cache for CustomRestClient implementation URL: https://github.com/apache/helix/pull/367#issuecomment-518397174 > The parallel request still need to be considered. Say you are implementing the locking by keys. We need 100 participants partition level status called from single instance health check. Then you will read one instance partition level status after the other. It is still sequential read. The sequential read comes from how the upper class uses the CustomRestClientImpl. Currently, it is sequential but it's the scope of next PR. In this PR, my purpose is to add the cache implementation so we can change the implementation of the upper class (ServiceImpl) and make it parallel. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] i3wangyi commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
i3wangyi commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r310786737
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -97,7 +106,43 @@ public CustomRestClientImpl(HttpClient httpClient) {
kv -> result.put(kv.getKey(),
kv.getValue().get(IS_HEALTHY_FIELD).asBoolean()));
return result;
};
-return handleResponse(post(url, payLoads), jsonConverter);
+String requestId = baseUrl + customPayloads.toString() +
partitions.toString();
+try {
+ lock(requestId);
+ if (_cachedResults.containsKey(requestId)) {
+return _cachedResults.get(requestId);
+ }
+ Map result = handleResponse(post(url, payLoads),
jsonConverter);
+ _cachedResults.put(requestId, result);
+ return result;
+} catch (InterruptedException e) {
+ e.printStackTrace();
+} finally {
+ unlock(requestId);
+}
+return Collections.emptyMap();
+ }
+
+ @VisibleForTesting
+ Map> getCachedResults() {
+return _cachedResults;
+ }
+
+ private void lock(String key) throws InterruptedException {
+synchronized (lockedKeys) {
Review comment:
It is locked on key level, my unit test just verifies it. See the below
debug message, other thread is able to acquire a different key before others
release the key
```
Thread: 7 acquired key http://localhost:1000{}[Thread: 7]
Thread: 4 acquired key http://localhost:1000{}[Thread: 4]
Thread: 6 acquired key http://localhost:1000{}[Thread: 6]
Thread: 0 acquired key http://localhost:1000{}[Thread: 0]
Thread: 9 acquired key http://localhost:1000{}[Thread: 9]
Thread: 5 acquired key http://localhost:1000{}[Thread: 5]
Thread: 2 acquired key http://localhost:1000{}[Thread: 2]
Thread: 8 acquired key http://localhost:1000{}[Thread: 8]
Thread: 3 acquired key http://localhost:1000{}[Thread: 3]
Thread: 1 acquired key http://localhost:1000{}[Thread: 1]
Thread: 4 release lock for http://localhost:1000{}[Thread: 4]
Thread: 9 release lock for http://localhost:1000{}[Thread: 9]
Thread: 2 release lock for http://localhost:1000{}[Thread: 2]
Thread: 1 release lock for http://localhost:1000{}[Thread: 1]
Thread: 6 release lock for http://localhost:1000{}[Thread: 6]
Thread: 0 release lock for http://localhost:1000{}[Thread: 0]
Thread: 3 release lock for http://localhost:1000{}[Thread: 3]
Thread: 8 release lock for http://localhost:1000{}[Thread: 8]
Thread: 7 release lock for http://localhost:1000{}[Thread: 7]
Thread: 5 release lock for http://localhost:1000{}[Thread: 5]
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on issue #367: Add transient cache for CustomRestClient implementation
dasahcc commented on issue #367: Add transient cache for CustomRestClient implementation URL: https://github.com/apache/helix/pull/367#issuecomment-518390345 The parallel request still need to be considered. Say you are implementing the locking by keys. We need 100 participants partition level status called from single instance health check. Then you will read one instance partition level status after the other. It is still sequential read. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] dasahcc merged pull request #359: Dynamically change the processor thread name when consuming event
dasahcc merged pull request #359: Dynamically change the processor thread name when consuming event URL: https://github.com/apache/helix/pull/359 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] i3wangyi commented on issue #359: Dynamically change the processor thread name when consuming event
i3wangyi commented on issue #359: Dynamically change the processor thread name when consuming event URL: https://github.com/apache/helix/pull/359#issuecomment-518348999 This PR is ready to be merged, approved by @lei-xia This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r310212588
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -97,7 +106,43 @@ public CustomRestClientImpl(HttpClient httpClient) {
kv -> result.put(kv.getKey(),
kv.getValue().get(IS_HEALTHY_FIELD).asBoolean()));
return result;
};
-return handleResponse(post(url, payLoads), jsonConverter);
+String requestId = baseUrl + customPayloads.toString() +
partitions.toString();
+try {
+ lock(requestId);
+ if (_cachedResults.containsKey(requestId)) {
+return _cachedResults.get(requestId);
+ }
+ Map result = handleResponse(post(url, payLoads),
jsonConverter);
+ _cachedResults.put(requestId, result);
+ return result;
+} catch (InterruptedException e) {
+ e.printStackTrace();
Review comment:
Why not use log but print out?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] dasahcc commented on a change in pull request #367: Add transient cache for CustomRestClient implementation
dasahcc commented on a change in pull request #367: Add transient cache for
CustomRestClient implementation
URL: https://github.com/apache/helix/pull/367#discussion_r310212407
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##
@@ -97,7 +106,43 @@ public CustomRestClientImpl(HttpClient httpClient) {
kv -> result.put(kv.getKey(),
kv.getValue().get(IS_HEALTHY_FIELD).asBoolean()));
return result;
};
-return handleResponse(post(url, payLoads), jsonConverter);
+String requestId = baseUrl + customPayloads.toString() +
partitions.toString();
+try {
+ lock(requestId);
+ if (_cachedResults.containsKey(requestId)) {
+return _cachedResults.get(requestId);
+ }
+ Map result = handleResponse(post(url, payLoads),
jsonConverter);
+ _cachedResults.put(requestId, result);
+ return result;
+} catch (InterruptedException e) {
+ e.printStackTrace();
+} finally {
+ unlock(requestId);
+}
+return Collections.emptyMap();
+ }
+
+ @VisibleForTesting
+ Map> getCachedResults() {
+return _cachedResults;
+ }
+
+ private void lock(String key) throws InterruptedException {
+synchronized (lockedKeys) {
Review comment:
This is not locking on key level but set object level.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly closed issue #358: removing DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD from clusterconfig
narendly closed issue #358: removing DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD from clusterconfig URL: https://github.com/apache/helix/issues/358 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] narendly merged pull request #373: Remove DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD from ClusterConfig #358
narendly merged pull request #373: Remove DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD from ClusterConfig #358 URL: https://github.com/apache/helix/pull/373 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] narendly commented on issue #373: Remove DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD from ClusterConfig #358
narendly commented on issue #373: Remove DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD from ClusterConfig #358 URL: https://github.com/apache/helix/pull/373#issuecomment-518325951 This PR is ready to be merged, approved by @alirezazamani This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] narendly opened a new pull request #373: Remove DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD from ClusterConfig #358
narendly opened a new pull request #373: Remove
DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD from ClusterConfig #358
URL: https://github.com/apache/helix/pull/373
### Issues
- [x] My PR addresses the following Helix issues and references them in the
PR title:
#358
### Description
- [x] Here are some details about my PR, including screenshots of any UI
changes:
DEFAULT_VIEW_CLUSTER_REFRESH_PERIOD is a constant field for View Aggregator.
Since this component was removed from the master branch, this constant field
should be removed as well.
### Tests
No tests were written/run because this is just removal of an unused constant.
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in
their subject lines, and I have squashed multiple commits if they address the
same issue. In addition, my commits follow the guidelines from "[How to write a
good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- [x] My diff has been formatted using helix-style.xml
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
narendly commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310622237
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
+logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
clusterName,
Review comment:
Refresh -> refreshed, takes -> took. Generally shouldn't change the logs but
we're changing it here anyways..
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
narendly commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310619356
##
File path:
helix-core/src/main/java/org/apache/helix/common/caches/AbstractDataCache.java
##
@@ -48,15 +50,17 @@ public AbstractDataCache(ControlContextProvider
controlContextProvider) {
* Selectively fetch Helix Properties from ZK by comparing the version of
local cached one with the one on ZK.
* If version on ZK is newer, fetch it from zk and update local cache.
* @param accessor the HelixDataAccessor
- * @param reloadKeys keys needs to be reload
+ * @param reloadKeysIn keys needs to be reload
Review comment:
reload -> reloaded
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
narendly commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310624803
##
File path:
helix-core/src/test/java/org/apache/helix/common/caches/TestCurrentStateSnapshot.java
##
@@ -0,0 +1,106 @@
+package org.apache.helix.common.caches;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.MockAccessor;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.LiveInstance;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+/**
+ * Unit test for {@link CurrentStateSnapshot}
+ */
+public class TestCurrentStateSnapshot {
Review comment:
Please add some more description on why this test is needed and what it's
testing :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
narendly commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310621901
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
Review comment:
Should we print out a different log entirely if helixManager is null and
return? Since it signals a bigger problem?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
narendly commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310623149
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
+logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
clusterName,
(System.currentTimeMillis() - startTime));
-notifyRoutingTableChange();
+
+// TODO: move the callback user code logic to separate thread upon routing
table statePropagation latency
+// integration test result. If the latency is more than 2 secs, we need to
change this part.
Review comment:
Question: Which test are you specifically referring to? Do we have an
integration test that fails when the latency exceeds 2 secs? Is 2 secs an
arbitrary number?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #365: Fix RoutingTableProvider statePropagationLatency metric reporting bug
narendly commented on a change in pull request #365: Fix RoutingTableProvider
statePropagationLatency metric reporting bug
URL: https://github.com/apache/helix/pull/365#discussion_r310623904
##
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##
@@ -558,23 +558,30 @@ protected void refresh(Map>> curre
private void resetRoutingTableAndNotify(long startTime, RoutingTable
newRoutingTable) {
_routingTableRef.set(newRoutingTable);
-logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
-(_helixManager != null ? _helixManager.getClusterName() : null),
+String clusterName = _helixManager != null ?
_helixManager.getClusterName() : null;
+logger.info("Refresh the RoutingTable for cluster {}, takes {} ms.",
clusterName,
(System.currentTimeMillis() - startTime));
-notifyRoutingTableChange();
+
+// TODO: move the callback user code logic to separate thread upon routing
table statePropagation latency
+// integration test result. If the latency is more than 2 secs, we need to
change this part.
+notifyRoutingTableChange(clusterName);
// Update timestamp for last refresh
if (_isPeriodicRefreshEnabled) {
_lastRefreshTimestamp = System.currentTimeMillis();
}
}
- private void notifyRoutingTableChange() {
-for (Map.Entry entry :
_routingTableChangeListenerMap
-.entrySet()) {
- entry.getKey().onRoutingTableChange(new
RoutingTableSnapshot(_routingTableRef.get()),
- entry.getValue().getContext());
+ private void notifyRoutingTableChange(String clusterName) {
+// This call back is called in the main event queue of
RoutingTableProvider. We add log to record time spent
+// here. Potentially, we should call this callback in a separate thread if
this is a bottleneck.
+long startTime = System.currentTimeMillis();
+for (Map.Entry entry :
_routingTableChangeListenerMap.entrySet()) {
+ entry.getKey()
+ .onRoutingTableChange(new
RoutingTableSnapshot(_routingTableRef.get()), entry.getValue().getContext());
}
+logger.info("RoutingTableProvider callback [aka not helix time] for
cluster {}, take {} ms.", clusterName,
Review comment:
Would this possibly overwhelm the log? And what do you mean by `[aka not
helix time]`? Let's clarify the terminology here?
take -> took
This log message in general isn't very intuitive.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on issue #369: Add the workaround fix for assigning partitions when instance weight …
jiajunwang commented on issue #369: Add the workaround fix for assigning partitions when instance weight … URL: https://github.com/apache/helix/pull/369#issuecomment-518101703 I just notice the title, this is not a workaround. Please remove this to avoid confusion. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on issue #342: Kickoff for the Weight Aware Globally Even Distribute Rebalancer
jiajunwang commented on issue #342: Kickoff for the Weight Aware Globally Even Distribute Rebalancer URL: https://github.com/apache/helix/issues/342#issuecomment-517893119 The foundation of the new rebalancer has been built after the interface/cluster model checked in. I'm closing this issue since we will have separate issues to track the following components development. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] jiajunwang closed issue #342: Kickoff for the Weight Aware Globally Even Distribute Rebalancer
jiajunwang closed issue #342: Kickoff for the Weight Aware Globally Even Distribute Rebalancer URL: https://github.com/apache/helix/issues/342 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] jiajunwang merged pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang merged pull request #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on issue #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on issue #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#issuecomment-517892944 This PR is ready to be merged, approved by @narendly. I will do the merge. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310338335
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,119 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
Review comment:
The rebalance logic will take care of the resource, partition, and also
state priority.
The priority we recorded in the replica is mainly for the top state. Since
that is really something special.
In fact, I tried to use a boolean isTopState. But then I find the number of
priority already exists and it is the most direct information. The model class
shall not do extra calculation.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336821
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,121 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
+ /**
+ * @param resourceConfig The resource config for the resource which contains
the replication.
+ * @param partitionName The replication's partition name.
+ * @param replicaState The state of the replication.
+ * @param statePriority The priority of the replication's state.
+ */
+ AssignableReplica(ResourceConfig resourceConfig, String partitionName,
String replicaState,
+ int statePriority) {
+_partitionName = partitionName;
+_replicaState = replicaState;
+_statePriority = statePriority;
+_resourceName = resourceConfig.getResourceName();
+_capacityUsage = fetchCapacityUsage(partitionName, resourceConfig);
Review comment:
I would only advocate for the Builder pattern if there were more arguments
(or if we expect to add more arguments to the constructor). To me, this is OK.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336903
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
+_disabledPartitionsMap.put("TestResource", disabledPartitions);
+_testInstanceTags = new ArrayList<>();
+_testInstanceTags.add("TestTag");
+_testFaultZoneId = "testZone";
+ }
+
+ protected ResourceControllerDataProvider setupClusterDataCache() throws
IOException {
Review comment:
I like the descriptive comments. Thanks :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336872
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
+ * Note that the replicas in this list shall
not be included while initializing the context and assignable nodes.
+ * @param assignableNodesThe active instances.
+ * @param baselineAssignment The recorded baseline assignment.
+ * @param bestPossibleAssignment The current best possible assignment.
+ */
+ ClusterModel(ClusterContext clusterContext, Set
assignableReplicas,
+ Set assignableNodes, Map
baselineAssignment,
+ Map bestPossibleAssignment) {
+_clusterContext = clusterContext;
+
+// Save all the to be assigned replication
+_assignableReplicas = assignableReplicas.stream()
+.collect(Collectors.groupingBy(AssignableReplica::getResourceName,
Collectors.toSet()));
+
+// Index all the replicas to be assigned. Dedup the replica if two
instances have the same resource/partition/state
+_assignableReplicaIndex = assignableReplicas.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName, Collectors
+.toMap(AssignableReplica::toString, replica -> replica,
+(oldValue, newValue) -> oldValue)));
+
+_assignableNodeMap = assignableNodes.stream()
+.collect(Collectors.toMap(AssignableNode::getInstanceName, node ->
node));
+
+_baselineAssignment = baselineAssignment;
+_bestPossibleAssignment = bestPossibleAssignment;
+ }
+
+ public ClusterContext getContext() {
+return _clusterContext;
+ }
+
+ public Map getAssignableNodes() {
+return _assignableNodeMap;
+ }
+
+ public Map> getAssignableReplicas() {
+return _assignableReplicas;
+ }
+
+ public Map getBaseline() {
+return _baselineAssignment;
+ }
+
+ public Map getBestPossibleAssignment() {
+return _bestPossibleAssignment;
+ }
+
+ /**
+ * Propose the assignment to the cluster model.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param instanceName
+ */
+ public void assign(String resourceName, String partitionName, String state,
String instanceName) {
+AssignableNode node = locateAssignableNode(instanceName);
+AssignableReplica replica = locateAssignableReplica(resourceName,
partitionName, state);
+
+node.assign(replica);
+_clusterContext.addPartitionToFaultZone(node.getFaultZone(), resourceName,
partitionName);
+ }
+
+ /**
+ * Revert the proposed assignment from the cluster model.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param instanceName
+ */
+ public void release(String resourceName, String partitionName, String state,
+ String instanceName) {
+AssignableNode node = locateAssignableNode(instanceName);
+AssignableReplica replica = locateAssignableReplica(resourceName,
partitionName, state);
+
+node.release(replica);
+_clusterContext.removePartitionFromFaultZone(node.getFaultZone(),
resourceName, partitionName);
+ }
+
+ private AssignableNode locateAssignableNode(String instanceName) {
+AssignableNode node = _assignableNodeMap.get(instanceName);
+if (node == null) {
+ throw new HelixException("Cannot find the instance: " + instanceName);
Review comment:
Sorry I don't think I understand.
This is a private method so I don't think anyone is going to call it but us,
but what would it mean when an AssignableNode is not found for the given
instanceName? Would that be an indication that we messed up somewhere?
This is an automated message from the Apache Git
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336640
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,119 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
Review comment:
I think I was thinking about priority among different partitions. I don't
know how the assignment logic works (is it part of this PR or is it TBD in
another PR?), but I agree with you if we assign replicas by partition.
I was just bringing up the fact that we may need to account partition-level
or resource-level priority at assign time. OK to resolve this issue since we
could always revisit later.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336694
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -19,10 +19,287 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static java.lang.Math.max;
+
/**
- * A placeholder before we have the implementation.
- *
- * This class represents a potential allocation of the replication.
- * Note that AssignableNode is not thread safe.
+ * This class represents a possible allocation of the replication.
+ * Note that any usage updates to the AssignableNode are not thread safe.
*/
-public class AssignableNode { }
+public class AssignableNode {
+ private static final Logger LOG =
LoggerFactory.getLogger(AssignableNode.class.getName());
+
+ // basic node information
+ private final String _instanceName;
+ private Set _instanceTags;
+ private String _faultZone;
+ private Map> _disabledPartitionsMap;
+ private Map _maxCapacity;
+ private int _maxPartition; // maximum number of the partitions that can be
assigned to the node.
+
+ // proposed assignment tracking
+ //
+ private Map> _currentAssignments;
+ //
+ private Map> _currentTopStateAssignments;
+ //
+ private Map _currentCapacity;
+ // The maximum capacity utilization (0.0 - 1.0) across all the capacity
categories.
+ private float _highestCapacityUtilization;
+
+ AssignableNode(ClusterConfig clusterConfig, InstanceConfig instanceConfig,
String instanceName,
+ Collection existingAssignment) {
+_instanceName = instanceName;
+refresh(clusterConfig, instanceConfig, existingAssignment);
+ }
+
+ private void reset() {
+_currentAssignments = new HashMap<>();
+_currentTopStateAssignments = new HashMap<>();
+_currentCapacity = new HashMap<>();
+_highestCapacityUtilization = 0;
+ }
+
+ /**
+ * Update the node with a ClusterDataCache. This resets the current
assignment and recalculates currentCapacity.
+ * NOTE: While this is required to be used in the constructor, this can also
be used when the clusterCache needs to be
+ * refreshed. This is under the assumption that the capacity mappings of
InstanceConfig and ResourceConfig could
+ * subject to change. If the assumption is no longer true, this function
should become private.
+ *
+ * @param clusterConfig - the Cluster Config of the cluster where the node
is located
+ * @param instanceConfig - the Instance Config of the node
+ * @param existingAssignment - all the existing replicas that are current
assigned to the node
+ */
+ private void refresh(ClusterConfig clusterConfig, InstanceConfig
instanceConfig,
+ Collection existingAssignment) {
+reset();
+
+_currentCapacity.putAll(instanceConfig.getInstanceCapacityMap());
+_faultZone = computeFaultZone(clusterConfig, instanceConfig);
+_instanceTags = new HashSet<>(instanceConfig.getTags());
+_disabledPartitionsMap = instanceConfig.getDisabledPartitionsMap();
+_maxCapacity = instanceConfig.getInstanceCapacityMap();
+_maxPartition = clusterConfig.getMaxPartitionsPerInstance();
+
+assignNewBatch(existingAssignment);
+ }
+
+ /**
+ * Assign a replica to the node.
+ *
+ * @param assignableReplica - the replica to be assigned
+ */
+ void assign(AssignableReplica assignableReplica) {
+if (!addToAssignmentRecord(assignableReplica, _currentAssignments)) {
+ throw new HelixException(String
+ .format("Resource %s already has a replica from partition %s on node
%s",
+ assignableReplica.getResourceName(),
assignableReplica.getPartitionName(),
+ getInstanceName()));
+} else {
+ if (assignableReplica.isReplicaTopState()) {
+addToAssignmentRecord(assignableReplica, _currentTopStateAssignments);
+ }
+ assignableReplica.getCapacity().entrySet().stream().forEach(
+ capacity -> updateCapacityAndUtilization(capacity.getKey(),
capacity.getValue()));
+}
+ }
+
+ /**
+ * Release a replica from the node.
+ * If the replication is not on this node, the assignable node is not
updated.
+ *
+ * @param assignableReplica - the replica to be released
+ */
+ void release(AssignableReplica assignableReplica) throws
IllegalArgumentException {
+String resourceName = assignableReplica.getResourceName();
+String partitionName = assignableReplica.getPartitionName();
+
+// Check if the release is necessary
+if
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336640
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,119 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
Review comment:
I think I was thinking about priority among different partitions. I don't
know how the assignment logic works (is it part of this PR or is it TBD in
another PR?), but I agree with you if we assign replicas by partition.
I was just bringing up the fact that we may need to account partition-level
or resource-level priority at assign time.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336585
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -19,10 +19,293 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static java.lang.Math.max;
+
/**
- * A placeholder before we have the implementation.
- *
- * This class represents a potential allocation of the replication.
- * Note that AssignableNode is not thread safe.
+ * This class represents a possible allocation of the replication.
+ * Note that any usage updates to the AssignableNode are not thread safe.
*/
-public class AssignableNode { }
+public class AssignableNode {
+ private static final Logger _logger =
LoggerFactory.getLogger(AssignableNode.class.getName());
+
+ // basic node information
+ private final String _instanceName;
+ private Set _instanceTags;
+ private String _faultZone;
+ private Map> _disabledPartitionsMap;
+ private Map _maxCapacity;
+ private int _maxPartition;
+
+ // proposed assignment tracking
+ //
+ private Map> _currentAssignments;
+ //
+ private Map> _currentTopStateAssignments;
+ //
+ private Map _currentCapacity;
+ // runtime usage tracking
+ private int _totalReplicaAssignmentCount;
+ private float _highestCapacityUtilization;
+
+ AssignableNode(ResourceControllerDataProvider clusterCache, String
instanceName,
+ Collection existingAssignment) {
+_instanceName = instanceName;
+refresh(clusterCache, existingAssignment);
+ }
+
+ private void reset() {
+_currentAssignments = new HashMap<>();
+_currentTopStateAssignments = new HashMap<>();
+_currentCapacity = new HashMap<>();
+_totalReplicaAssignmentCount = 0;
+_highestCapacityUtilization = 0;
+ }
+
+ /**
+ * Update the node with a ClusterDataCache. This resets the current
assignment and recalculate currentCapacity.
+ * NOTE: While this is required to be used in the constructor, this can also
be used when the clusterCache needs to be
+ * refreshed. This is under the assumption that the capacity mappings of
InstanceConfig and ResourceConfig could
+ * subject to changes. If the assumption is no longer true, this function
should become private.
+ *
+ * @param clusterCache - the current cluster cache to initial the
AssignableNode.
+ */
+ private void refresh(ResourceControllerDataProvider clusterCache,
+ Collection existingAssignment) {
+reset();
+
+InstanceConfig instanceConfig =
clusterCache.getInstanceConfigMap().get(_instanceName);
+ClusterConfig clusterConfig = clusterCache.getClusterConfig();
+
+_currentCapacity.putAll(instanceConfig.getInstanceCapacityMap());
+_faultZone = computeFaultZone(clusterConfig, instanceConfig);
+_instanceTags = new HashSet<>(instanceConfig.getTags());
+_disabledPartitionsMap = instanceConfig.getDisabledPartitionsMap();
+_maxCapacity = instanceConfig.getInstanceCapacityMap();
+_maxPartition = clusterConfig.getMaxPartitionsPerInstance();
+
+assignNewBatch(existingAssignment);
+ }
+
+ /**
+ * Assign a replica to the node.
+ *
+ * @param assignableReplica - the replica to be assigned
+ */
+ void assign(AssignableReplica assignableReplica) {
+if (!addToAssignmentRecord(assignableReplica, _currentAssignments)) {
+ throw new HelixException(String
+ .format("Resource %s already has a replica from partition %s on this
node",
+ assignableReplica.getResourceName(),
assignableReplica.getPartitionName()));
+} else {
+ if (assignableReplica.isReplicaTopState()) {
+addToAssignmentRecord(assignableReplica, _currentTopStateAssignments);
+ }
+ _totalReplicaAssignmentCount += 1;
+ assignableReplica.getCapacity().entrySet().stream()
+ .forEach(entry -> updateCapacityAndUtilization(entry.getKey(),
entry.getValue()));
+}
+ }
+
+ /**
+ * Release a replica from the node.
+ * If the replication is not on this node, the assignable node is not
updated.
+ *
+ * @param assignableReplica - the replica to be released
+ */
+ void release(AssignableReplica assignableReplica) throws
IllegalArgumentException {
+String resourceName = assignableReplica.getResourceName();
+String partitionName = assignableReplica.getPartitionName();
+
+// Check if the release is necessary
+if
[GitHub] [helix] jiajunwang opened a new issue #372: Implement the Cluster Model data provider for the WAGED rebalancer
jiajunwang opened a new issue #372: Implement the Cluster Model data provider for the WAGED rebalancer URL: https://github.com/apache/helix/issues/372 Please find the design doc here: https://github.com/apache/helix/wiki/Design-Proposal---Weight-Aware-Globally-Even-Distribute-Rebalancer This data provider will help to convert Helix cluster status data into the rebalancer readable Cluster Model object. The result will be used to support rebalance calculation. AC: - finish the implementation based on the design. - add unit tests for the new data provider. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on issue #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on issue #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#issuecomment-517854427 Thanks for your hard work of reviewing. I know it is a huge PR. Mainly because it is new classes that are built from scratch, even one file's change has been large enough. And as the initial interface checking, I wish to keep then in one PR. The following functional components will still be a large file, but they will be loosely coupled. So we will be able to make relatively smaller PR. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310284240
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -19,10 +19,290 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static java.lang.Math.max;
+
/**
- * A placeholder before we have the implementation.
- *
- * This class represents a potential allocation of the replication.
- * Note that AssignableNode is not thread safe.
+ * This class represents a possible allocation of the replication.
+ * Note that any usage updates to the AssignableNode are not thread safe.
*/
-public class AssignableNode { }
+public class AssignableNode {
+ private static final Logger _logger =
LoggerFactory.getLogger(AssignableNode.class.getName());
+
+ // proposed assignment tracking
+ //
+ private Map> _currentAssignments;
+ //
+ private Map> _currentTopStateAssignments;
+ //
+ private Map _currentCapacity;
+ // runtime usage tracking
+ private int _totalReplicaAssignmentCount;
+ private float _highestCapacityUtilization;
+
+ // basic node information
+ private final String _instanceName;
+ private Set _instanceTags;
+ private String _faultZone;
+ private Map> _disabledPartitionsMap;
+ private Map _maxCapacity;
+ private int _maxPartition;
+
+ AssignableNode(ResourceControllerDataProvider clusterCache, String
instanceName,
+ Collection existingAssignment) {
+_instanceName = instanceName;
+refresh(clusterCache, existingAssignment);
+ }
+
+ private void reset() {
+_currentAssignments = new HashMap<>();
+_currentTopStateAssignments = new HashMap<>();
+_currentCapacity = new HashMap<>();
+_totalReplicaAssignmentCount = 0;
+_highestCapacityUtilization = 0;
+ }
+
+ /**
+ * Update the node with a ClusterDataCache. This resets the current
assignment and recalculate currentCapacity.
+ * NOTE: While this is required to be used in the constructor, this can also
be used when the clusterCache needs to be
+ * refreshed. This is under the assumption that the capacity mappings of
InstanceConfig and ResourceConfig could
+ * subject to changes. If the assumption is no longer true, this function
should become private.
+ *
+ * @param clusterCache - the current cluster cache to initial the
AssignableNode.
+ */
+ private void refresh(ResourceControllerDataProvider clusterCache,
Review comment:
Some background here. We want to have a cluster data cache snapshot based on
DataProvider. And that should have been used here. The snapshot is immutable.
Unfortunately, we don't have this class implemented yet.
This provider the closest thing we can use for now. We will need to refactor
the usage of the data provider everywhere once the snapshot is done. This is
planed in the scope of controller improvement.
About the second point, the data provider does not know anything about data
model. This is the current situation. I guess you are saying that data model
shall not rely on the data provider's methods, right?
This is a valid call, let me try to change the refresh method parameters.
But still, a builder is too complicated for now.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310279995
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
+Assert.assertEquals(context.getEstimatedMaxPartitionCount(), 3);
+Assert.assertEquals(context.getEstimatedMaxTopStateCount(), 2);
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
Collections.emptyMap());
+for (String resourceName : _resourceNames) {
+
Assert.assertEquals(context.getEstimatedMaxPartitionByResource(resourceName),
2);
+ Assert.assertEquals(
+ context.getPartitionsForResourceAndFaultZone(_testFaultZoneId,
resourceName),
+ Collections.emptySet());
+}
+
+// Assign
+Map>> expectedFaultZoneMap = Collections
+.singletonMap(_testFaultZoneId,
assignmentSet.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName,
+Collectors.mapping(AssignableReplica::getPartitionName,
toSet();
+
+assignmentSet.stream().forEach(r -> context
+.addPartitionToFaultZone(_testFaultZoneId, r.getResourceName(),
r.getPartitionName()));
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+
+// release
+
Assert.assertTrue(expectedFaultZoneMap.get(_testFaultZoneId).get(_resourceNames.get(0))
Review comment:
This is a typo. Good catch! Should check the next line.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310280122
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
+Assert.assertEquals(context.getEstimatedMaxPartitionCount(), 3);
+Assert.assertEquals(context.getEstimatedMaxTopStateCount(), 2);
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
Collections.emptyMap());
+for (String resourceName : _resourceNames) {
+
Assert.assertEquals(context.getEstimatedMaxPartitionByResource(resourceName),
2);
+ Assert.assertEquals(
+ context.getPartitionsForResourceAndFaultZone(_testFaultZoneId,
resourceName),
+ Collections.emptySet());
+}
+
+// Assign
+Map>> expectedFaultZoneMap = Collections
+.singletonMap(_testFaultZoneId,
assignmentSet.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName,
+Collectors.mapping(AssignableReplica::getPartitionName,
toSet();
+
+assignmentSet.stream().forEach(r -> context
+.addPartitionToFaultZone(_testFaultZoneId, r.getResourceName(),
r.getPartitionName()));
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+
+// release
+
Assert.assertTrue(expectedFaultZoneMap.get(_testFaultZoneId).get(_resourceNames.get(0))
+.remove(_partitionNames.get(0)));
+context.removePartitionFromFaultZone(_testFaultZoneId,
_resourceNames.get(0),
+_partitionNames.get(0));
+
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+ }
+
+ @Test(expectedExceptions = HelixException.class,
expectedExceptionsMessageRegExp = "Resource Resource1 already has a replica
from partition Partition1 in fault zone testZone")
+ public void testAssignAlreadyExist() throws IOException {
Review comment:
testDuplicateAssign
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310278490
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java
##
@@ -0,0 +1,100 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.model.ResourceConfig;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY;
+
+public class TestAssignableReplica {
+ String resourceName = "Resource";
+ String partitionNamePrefix = "partition";
+ String masterState = "Master";
+ int masterPriority = TOP_STATE_PRIORITY;
+ String slaveState = "Slave";
+ int slavePriority = 2;
+
+ @Test
+ public void testConstructRepliaWithResourceConfig() throws IOException {
+// Init assignable replication with a basic config object
+Map capacityDataMapResource1 = new HashMap<>();
+capacityDataMapResource1.put("item1", 3);
+capacityDataMapResource1.put("item2", 6);
+ResourceConfig testResourceConfigResource = new
ResourceConfig(resourceName);
+testResourceConfigResource.setPartitionCapacityMap(
+Collections.singletonMap(ResourceConfig.DEFAULT_PARTITION_KEY,
capacityDataMapResource1));
+
+String partitionName = partitionNamePrefix + 1;
+AssignableReplica replica =
+new AssignableReplica(testResourceConfigResource, partitionName,
masterState,
+masterPriority);
+Assert.assertEquals(replica.getResourceName(), resourceName);
Review comment:
In that case, we will still need to list all the different parameters.
Moreover, if anything new to check in different places, we need to split the
method or check redundantly. So I prefer to keep the Asserts listed in the test
case.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#discussion_r310277680 ## File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java ## @@ -0,0 +1,100 @@ +package org.apache.helix.controller.rebalancer.waged.model; + +/* + * 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. + */ + +import org.apache.helix.model.ResourceConfig; +import org.testng.Assert; +import org.testng.annotations.Test; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY; Review comment: This is automatically done... I'm fine with both. Will change it. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310277478
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310277080
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310276844
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310249111
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
+_disabledPartitionsMap.put("TestResource", disabledPartitions);
+_testInstanceTags = new ArrayList<>();
+_testInstanceTags.add("TestTag");
+_testFaultZoneId = "testZone";
+ }
+
+ protected ResourceControllerDataProvider setupClusterDataCache() throws
IOException {
Review comment:
Sure, make sense.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310248906
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
+ * Note that the replicas in this list shall
not be included while initializing the context and assignable nodes.
+ * @param assignableNodesThe active instances.
+ * @param baselineAssignment The recorded baseline assignment.
+ * @param bestPossibleAssignment The current best possible assignment.
+ */
+ ClusterModel(ClusterContext clusterContext, Set
assignableReplicas,
+ Set assignableNodes, Map
baselineAssignment,
+ Map bestPossibleAssignment) {
+_clusterContext = clusterContext;
+
+// Save all the to be assigned replication
+_assignableReplicas = assignableReplicas.stream()
+.collect(Collectors.groupingBy(AssignableReplica::getResourceName,
Collectors.toSet()));
+
+// Index all the replicas to be assigned. Dedup the replica if two
instances have the same resource/partition/state
+_assignableReplicaIndex = assignableReplicas.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName, Collectors
+.toMap(AssignableReplica::toString, replica -> replica,
+(oldValue, newValue) -> oldValue)));
+
+_assignableNodeMap = assignableNodes.stream()
+.collect(Collectors.toMap(AssignableNode::getInstanceName, node ->
node));
+
+_baselineAssignment = baselineAssignment;
+_bestPossibleAssignment = bestPossibleAssignment;
+ }
+
+ public ClusterContext getContext() {
+return _clusterContext;
+ }
+
+ public Map getAssignableNodes() {
+return _assignableNodeMap;
+ }
+
+ public Map> getAssignableReplicas() {
+return _assignableReplicas;
+ }
+
+ public Map getBaseline() {
+return _baselineAssignment;
+ }
+
+ public Map getBestPossibleAssignment() {
+return _bestPossibleAssignment;
+ }
+
+ /**
+ * Propose the assignment to the cluster model.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param instanceName
+ */
+ public void assign(String resourceName, String partitionName, String state,
String instanceName) {
+AssignableNode node = locateAssignableNode(instanceName);
+AssignableReplica replica = locateAssignableReplica(resourceName,
partitionName, state);
+
+node.assign(replica);
+_clusterContext.addPartitionToFaultZone(node.getFaultZone(), resourceName,
partitionName);
+ }
+
+ /**
+ * Revert the proposed assignment from the cluster model.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param instanceName
+ */
+ public void release(String resourceName, String partitionName, String state,
+ String instanceName) {
+AssignableNode node = locateAssignableNode(instanceName);
+AssignableReplica replica = locateAssignableReplica(resourceName,
partitionName, state);
+
+node.release(replica);
+_clusterContext.removePartitionFromFaultZone(node.getFaultZone(),
resourceName, partitionName);
+ }
+
+ private AssignableNode locateAssignableNode(String instanceName) {
+AssignableNode node = _assignableNodeMap.get(instanceName);
+if (node == null) {
+ throw new HelixException("Cannot find the instance: " + instanceName);
Review comment:
This is a public interface. If anyone calls it in a wrong way, we'd better
have a HelixException instead of NPE.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service,
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310246676
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
Review comment:
Good catch
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310245795
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,121 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
+ /**
+ * @param resourceConfig The resource config for the resource which contains
the replication.
+ * @param partitionName The replication's partition name.
+ * @param replicaState The state of the replication.
+ * @param statePriority The priority of the replication's state.
+ */
+ AssignableReplica(ResourceConfig resourceConfig, String partitionName,
String replicaState,
+ int statePriority) {
+_partitionName = partitionName;
+_replicaState = replicaState;
+_statePriority = statePriority;
+_resourceName = resourceConfig.getResourceName();
+_capacityUsage = fetchCapacityUsage(partitionName, resourceConfig);
Review comment:
Even the constructor of Assignable Replica is not public. There is no need
we make the construction methods public. For now, I think it is overdesigned to
use builder pattern here.
We can revisit here if we are going to construct a replica object somewhere
else.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310244594
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
Review comment:
As comments above, I don't want to make this list immutable.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310244369
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
Review comment:
This is a test class. The abstract class might be used by the child in any
way. So if they want to add more resources to the list, it is fine.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310237287
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
Review comment:
Partation -> Partition
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310238349
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
Review comment:
Nit: also could do Assert.assertEquals(condition, error msg). One liner. I'm
fine with the way it is.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239674
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310236475
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310237911
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
Review comment:
This 3 is a magic number, so let's make it `NUM_FAULT_ZONES` and make it a
constant.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239447
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
Review comment:
Magic number -> constant
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239990
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239874
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239254
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
Review comment:
Magic number 9! Let's make this a constant: `NUM_INSTANCES`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239855
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310236475
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310234688
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Node.java
##
@@ -36,6 +36,11 @@
public Node() { }
+ public Node(String name, long id) {
Review comment:
I think this is a minor point. Alternatively, you could just use the empty
constructor and call set methods but either way I think would work?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310234688
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Node.java
##
@@ -36,6 +36,11 @@
public Node() { }
+ public Node(String name, long id) {
Review comment:
I think this is a minor point. Alternatively, you could just use the empty
constructor and call set methods but either way I think would work?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310233448
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
Review comment:
I like how you consistently add the description. Here, though, could you add
a few lines on what properties are tested?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310233448
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
Review comment:
I like how you consistently add the description. Here, though, could you add
a few lines on what properties are tested?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config URL: https://github.com/apache/helix/pull/333#discussion_r310232277 ## File path: helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java ## @@ -99,6 +99,21 @@ public StateTransitionThrottleController(Set resources, ClusterConfig cl } } + // The getter methods are used for debugging and testing purpose Review comment: Yes, you would need to make the relevant fields `protected` (that was implied). `protected` should be good enough as long as we don't make it `public`. The point here is to avoid adding methods that aren't strictly part of the class logic :) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config URL: https://github.com/apache/helix/pull/333#discussion_r310232277 ## File path: helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java ## @@ -99,6 +99,21 @@ public StateTransitionThrottleController(Set resources, ClusterConfig cl } } + // The getter methods are used for debugging and testing purpose Review comment: Yes, you would need to make the relevant fields `protected`. That was implied. `protected` should be good enough as long as we don't make it `public`. The point here is to avoid adding methods that aren't strictly part of the class logic :) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only
sets ANY at cluster level throttle config
URL: https://github.com/apache/helix/pull/333#discussion_r310230832
##
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java
##
@@ -208,13 +210,20 @@ protected void
chargeResource(StateTransitionThrottleConfig.RebalanceType rebala
*/
protected void chargeInstance(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
String instance) {
-if (_pendingTransitionAllowedPerInstance.containsKey(instance)
-&&
_pendingTransitionAllowedPerInstance.get(instance).containsKey(rebalanceType)) {
- chargeANYType(_pendingTransitionAllowedPerInstance.get(instance));
- Long instanceThrottle =
_pendingTransitionAllowedPerInstance.get(instance).get(rebalanceType);
- if (instanceThrottle > 0) {
-_pendingTransitionAllowedPerInstance.get(instance).put(rebalanceType,
instanceThrottle - 1);
- }
+charge(rebalanceType,
_pendingTransitionAllowedPerInstance.getOrDefault(instance, new HashMap<>()));
+ }
+
+ private void charge(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
+ Map quota) {
+if
(StateTransitionThrottleConfig.RebalanceType.NONE.equals(rebalanceType)) {
+ logger.error("Wrong rebalance type NONE as parameter");
+ return;
+}
+// if ANY type is present, decrement one else do nothing
+quota.computeIfPresent(StateTransitionThrottleConfig.RebalanceType.ANY,
(key, val) -> Math.max(0, val - 1));
Review comment:
I see what you were saying earlier. You could do `(type, quotaCount)` or
something similar. Point is to make the code a little more readable :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] i3wangyi commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
i3wangyi commented on a change in pull request #333: Fix issue when client only
sets ANY at cluster level throttle config
URL: https://github.com/apache/helix/pull/333#discussion_r310226396
##
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java
##
@@ -208,13 +210,20 @@ protected void
chargeResource(StateTransitionThrottleConfig.RebalanceType rebala
*/
protected void chargeInstance(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
String instance) {
-if (_pendingTransitionAllowedPerInstance.containsKey(instance)
-&&
_pendingTransitionAllowedPerInstance.get(instance).containsKey(rebalanceType)) {
- chargeANYType(_pendingTransitionAllowedPerInstance.get(instance));
- Long instanceThrottle =
_pendingTransitionAllowedPerInstance.get(instance).get(rebalanceType);
- if (instanceThrottle > 0) {
-_pendingTransitionAllowedPerInstance.get(instance).put(rebalanceType,
instanceThrottle - 1);
- }
+charge(rebalanceType,
_pendingTransitionAllowedPerInstance.getOrDefault(instance, new HashMap<>()));
+ }
+
+ private void charge(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
+ Map quota) {
+if
(StateTransitionThrottleConfig.RebalanceType.NONE.equals(rebalanceType)) {
+ logger.error("Wrong rebalance type NONE as parameter");
+ return;
+}
+// if ANY type is present, decrement one else do nothing
+quota.computeIfPresent(StateTransitionThrottleConfig.RebalanceType.ANY,
(key, val) -> Math.max(0, val - 1));
Review comment:
Look at the method parameter, these names are already taken
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r309939372
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
Review comment:
deduped
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310213495
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
Review comment:
Explain please? What does it mean to reduce assignment?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310218704
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
+Assert.assertEquals(context.getEstimatedMaxPartitionCount(), 3);
+Assert.assertEquals(context.getEstimatedMaxTopStateCount(), 2);
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
Collections.emptyMap());
+for (String resourceName : _resourceNames) {
+
Assert.assertEquals(context.getEstimatedMaxPartitionByResource(resourceName),
2);
+ Assert.assertEquals(
+ context.getPartitionsForResourceAndFaultZone(_testFaultZoneId,
resourceName),
+ Collections.emptySet());
+}
+
+// Assign
+Map>> expectedFaultZoneMap = Collections
+.singletonMap(_testFaultZoneId,
assignmentSet.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName,
+Collectors.mapping(AssignableReplica::getPartitionName,
toSet();
+
+assignmentSet.stream().forEach(r -> context
+.addPartitionToFaultZone(_testFaultZoneId, r.getResourceName(),
r.getPartitionName()));
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+
+// release
+
Assert.assertTrue(expectedFaultZoneMap.get(_testFaultZoneId).get(_resourceNames.get(0))
+.remove(_partitionNames.get(0)));
+context.removePartitionFromFaultZone(_testFaultZoneId,
_resourceNames.get(0),
+_partitionNames.get(0));
+
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+ }
+
+ @Test(expectedExceptions = HelixException.class,
expectedExceptionsMessageRegExp = "Resource Resource1 already has a replica
from partition Partition1 in fault zone testZone")
+ public void testAssignAlreadyExist() throws IOException {
Review comment:
AssignExistingReplica?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310212807
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
+_disabledPartitionsMap.put("TestResource", disabledPartitions);
+_testInstanceTags = new ArrayList<>();
+_testInstanceTags.add("TestTag");
+_testFaultZoneId = "testZone";
+ }
+
+ protected ResourceControllerDataProvider setupClusterDataCache() throws
IOException {
+ResourceControllerDataProvider testCache =
Mockito.mock(ResourceControllerDataProvider.class);
+
+InstanceConfig testInstanceConfig = new InstanceConfig("testInstanceId");
+testInstanceConfig.setInstanceCapacityMap(_capacityDataMap);
+testInstanceConfig.addTag(_testInstanceTags.get(0));
+testInstanceConfig.setInstanceEnabledForPartition("TestResource",
"TestPartition", false);
+testInstanceConfig.setInstanceEnabled(true);
+testInstanceConfig.setZoneId(_testFaultZoneId);
+Map instanceConfigMap = new HashMap<>();
+instanceConfigMap.put(_testInstanceId, testInstanceConfig);
+when(testCache.getInstanceConfigMap()).thenReturn(instanceConfigMap);
+
+ClusterConfig testClusterConfig = new ClusterConfig("testClusterConfigId");
+testClusterConfig.setMaxPartitionsPerInstance(5);
+testClusterConfig.setDisabledInstances(Collections.emptyMap());
+testClusterConfig.setTopologyAwareEnabled(false);
+when(testCache.getClusterConfig()).thenReturn(testClusterConfig);
+
+LiveInstance testLiveInstance = new LiveInstance(_testInstanceId);
+testLiveInstance.setSessionId("testSessionId");
+Map liveInstanceMap = new HashMap<>();
+liveInstanceMap.put(_testInstanceId, testLiveInstance);
+when(testCache.getLiveInstances()).thenReturn(liveInstanceMap);
+
+CurrentState testCurrentStateResource1 = Mockito.mock(CurrentState.class);
+Map partitionStateMap1 = new HashMap<>();
+partitionStateMap1.put(_partitionNames.get(0), "MASTER");
+partitionStateMap1.put(_partitionNames.get(1), "SLAVE");
+
when(testCurrentStateResource1.getResourceName()).thenReturn(_resourceNames.get(0));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310170701
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
+ * Note that the replicas in this list shall
not be included while initializing the context and assignable nodes.
+ * @param assignableNodesThe active instances.
+ * @param baselineAssignment The recorded baseline assignment.
+ * @param bestPossibleAssignment The current best possible assignment.
+ */
+ ClusterModel(ClusterContext clusterContext, Set
assignableReplicas,
+ Set assignableNodes, Map
baselineAssignment,
+ Map bestPossibleAssignment) {
+_clusterContext = clusterContext;
+
+// Save all the to be assigned replication
+_assignableReplicas = assignableReplicas.stream()
+.collect(Collectors.groupingBy(AssignableReplica::getResourceName,
Collectors.toSet()));
+
+// Index all the replicas to be assigned. Dedup the replica if two
instances have the same resource/partition/state
+_assignableReplicaIndex = assignableReplicas.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName, Collectors
+.toMap(AssignableReplica::toString, replica -> replica,
+(oldValue, newValue) -> oldValue)));
+
+_assignableNodeMap = assignableNodes.stream()
+.collect(Collectors.toMap(AssignableNode::getInstanceName, node ->
node));
+
+_baselineAssignment = baselineAssignment;
+_bestPossibleAssignment = bestPossibleAssignment;
+ }
+
+ public ClusterContext getContext() {
+return _clusterContext;
+ }
+
+ public Map getAssignableNodes() {
+return _assignableNodeMap;
+ }
+
+ public Map> getAssignableReplicas() {
+return _assignableReplicas;
+ }
+
+ public Map getBaseline() {
+return _baselineAssignment;
+ }
+
+ public Map getBestPossibleAssignment() {
+return _bestPossibleAssignment;
+ }
+
+ /**
+ * Propose the assignment to the cluster model.
Review comment:
Rephrase? This description doesn't make sense. Say "Try to make an
assignment for a given replica on the given instance. Note that this may not
show up in the final assignment - this assignment may be released and
re-assigned elsewhere."
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310223885
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterModel.java
##
@@ -0,0 +1,108 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+public class TestClusterModel extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ Set generateNodes(ResourceControllerDataProvider testCache) {
+Set nodeSet = new HashSet<>();
+testCache.getInstanceConfigMap().values().stream().forEach(config ->
nodeSet
+.add(new AssignableNode(testCache, config.getInstanceName(),
Collections.emptyList(;
+return nodeSet;
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignableReplicas = generateReplicas(testCache);
+Set assignableNodes = generateNodes(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignableReplicas, 2);
+ClusterModel clusterModel =
+new ClusterModel(context, assignableReplicas, assignableNodes,
Collections.emptyMap(),
+Collections.emptyMap());
+
+
Assert.assertTrue(clusterModel.getContext().getAssignmentForFaultZoneMap().values().stream()
+.allMatch(v -> v.values().isEmpty()));
+Assert.assertFalse(clusterModel.getAssignableNodes().values().stream()
+.anyMatch(n -> n.getCurrentAssignmentCount() != 0));
+
+// The initialization of the context, node and replication has been tested
separately. So for
+// cluster model, focus on testing the assignment and release.
+
+// Assign
+AssignableReplica replica = assignableReplicas.iterator().next();
+AssignableNode node = assignableNodes.iterator().next();
+clusterModel
+.assign(replica.getResourceName(), replica.getPartitionName(),
replica.getReplicaState(),
+node.getInstanceName());
+
+Assert.assertTrue(
+
clusterModel.getContext().getAssignmentForFaultZoneMap().get(node.getFaultZone())
+
.get(replica.getResourceName()).contains(replica.getPartitionName()));
+
Assert.assertTrue(node.getCurrentAssignmentsMap().get(replica.getResourceName())
+.contains(replica.getPartitionName()));
+
+// Assign a non-exist replication
+try {
+ clusterModel.assign("NOT-EXIST", replica.getPartitionName(),
replica.getReplicaState(),
+ node.getInstanceName());
+ Assert.fail("Assigning a non existing resource partition shall fail.");
+} catch (HelixException ex) {
+ // expected
+}
+
+// Assign a non-exist replication
+try {
+ clusterModel
+ .assign(replica.getResourceName(), replica.getPartitionName(),
replica.getReplicaState(),
+ "NON-EXIST");
+ Assert.fail("Assigning a resource partition to a non existing instance
shall fail.");
+} catch (HelixException ex) {
+ // expected
+}
+
+// Release
+clusterModel
+.release(replica.getResourceName(), replica.getPartitionName(),
replica.getReplicaState(),
+node.getInstanceName());
+
+
Assert.assertTrue(clusterModel.getContext().getAssignmentForFaultZoneMap().values().stream()
+.allMatch(v -> v.values().stream().allMatch(s -> s.isEmpty(;
Review comment:
Spell out one letter variables. They don't have to be long, but this is now
affecting readability. Especially it becomes confusing when nested maps are
involved.
Lambda expressions exist partly to reduce verbosity of Java but we shouldn't
sacrifice readability.
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r309921368
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterContext.java
##
@@ -19,9 +19,100 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
- * This class tracks the global rebalance-related status of a Helix managed
cluster.
+ * This class tracks the rebalance-related global cluster status.
*/
-public class ClusterContext { }
+public class ClusterContext {
+ private final static float ERROR_MARGIN_FOR_ESTIMATED_MAX_COUNT = 1.1f;
+
+ // This estimation helps to ensure global partition count evenness
+ private final int _estimatedMaxPartitionCount;
+ // This estimation helps to ensure global top state replica count evenness
+ private final int _estimatedMaxTopStateCount;
+ // This estimation helps to ensure per-resource partition count evenness
+ private final Map _estimatedMaxPartitionByResource = new
HashMap<>();
+
+ // map{zoneName : map{resourceName : set(partitionNames)}}
+ private Map>> _assignmentForFaultZoneMap =
new HashMap<>();
+
+ /**
+ * Construct the cluster context based on the current instance status.
+ *
+ * @param replicaSetAll the partition replicas that are managed by the
rebalancer
+ * @param instanceCount The count of all the active instances that can be
used to host partitions.
+ */
+ ClusterContext(Set replicaSet, int instanceCount) {
+int totalReplicas = 0;
+int totalTopStateReplicas = 0;
+
+for (Map.Entry> entry : replicaSet.stream()
+
.collect(Collectors.groupingBy(AssignableReplica::getResourceName)).entrySet())
{
+ int replicas = entry.getValue().size();
+ totalReplicas += replicas;
+
+ int replicaCnt = Math.max(1, estimateAvgReplicaCount(replicas,
instanceCount));
+ _estimatedMaxPartitionByResource.put(entry.getKey(), replicaCnt);
+
+ totalTopStateReplicas +=
+
entry.getValue().stream().filter(AssignableReplica::isReplicaTopState).count();
+}
+
+_estimatedMaxPartitionCount = estimateAvgReplicaCount(totalReplicas,
instanceCount);
+_estimatedMaxTopStateCount =
estimateAvgReplicaCount(totalTopStateReplicas, instanceCount);
+ }
+
+ public Map>> getAssignmentForFaultZoneMap() {
+return _assignmentForFaultZoneMap;
+ }
+
+ public int getEstimatedMaxPartitionCount() {
+return _estimatedMaxPartitionCount;
+ }
+
+ public int getEstimatedMaxPartitionByResource(String resourceName) {
+return _estimatedMaxPartitionByResource.get(resourceName);
+ }
+
+ public int getEstimatedMaxTopStateCount() {
+return _estimatedMaxTopStateCount;
+ }
+
+ public Set getPartitionsForResourceAndFaultZone(String faultZoneId,
String resourceName) {
Review comment:
Minor: Keep the order consistent in both the name and arguments - resource
and fault zone
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310213728
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310214870
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r309939522
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
Review comment:
Revise this replica key to resource_partition_state? Or we could link the
method?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#discussion_r310217077 ## File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java ## @@ -0,0 +1,93 @@ +package org.apache.helix.controller.rebalancer.waged.model; + +/* + * 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. + */ + +import org.apache.helix.HelixException; +import org.apache.helix.controller.dataproviders.ResourceControllerDataProvider; +import org.testng.Assert; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import java.io.IOException; +import java.util.Collections; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import static java.util.stream.Collectors.toSet; Review comment: Nit: use Collectors.toSet inline? I don't really care either way but we usually don't import static methods? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310216570
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
Review comment:
leaved -> left
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310216339
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java
##
@@ -0,0 +1,100 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.model.ResourceConfig;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY;
+
+public class TestAssignableReplica {
+ String resourceName = "Resource";
+ String partitionNamePrefix = "partition";
+ String masterState = "Master";
+ int masterPriority = TOP_STATE_PRIORITY;
+ String slaveState = "Slave";
+ int slavePriority = 2;
+
+ @Test
+ public void testConstructRepliaWithResourceConfig() throws IOException {
+// Init assignable replication with a basic config object
+Map capacityDataMapResource1 = new HashMap<>();
+capacityDataMapResource1.put("item1", 3);
+capacityDataMapResource1.put("item2", 6);
+ResourceConfig testResourceConfigResource = new
ResourceConfig(resourceName);
+testResourceConfigResource.setPartitionCapacityMap(
+Collections.singletonMap(ResourceConfig.DEFAULT_PARTITION_KEY,
capacityDataMapResource1));
+
+String partitionName = partitionNamePrefix + 1;
+AssignableReplica replica =
+new AssignableReplica(testResourceConfigResource, partitionName,
masterState,
+masterPriority);
+Assert.assertEquals(replica.getResourceName(), resourceName);
Review comment:
Can this series of asserts could be put into a private helper method called
verifyReplica or something? Too much repeated code?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310214018
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310162365
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
+ * Note that the replicas in this list shall
not be included while initializing the context and assignable nodes.
+ * @param assignableNodesThe active instances.
+ * @param baselineAssignment The recorded baseline assignment.
+ * @param bestPossibleAssignment The current best possible assignment.
+ */
+ ClusterModel(ClusterContext clusterContext, Set
assignableReplicas,
+ Set assignableNodes, Map
baselineAssignment,
+ Map bestPossibleAssignment) {
+_clusterContext = clusterContext;
+
+// Save all the to be assigned replication
+_assignableReplicas = assignableReplicas.stream()
Review comment:
Nit: just to avoid confusion, what do you think about renaming
assignableReplicas to assignableReplicaMap? That's what you seem to be doing
assignableNodeMap/assignableNodes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310215326
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java
##
@@ -0,0 +1,100 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.model.ResourceConfig;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY;
+
+public class TestAssignableReplica {
+ String resourceName = "Resource";
+ String partitionNamePrefix = "partition";
+ String masterState = "Master";
+ int masterPriority = TOP_STATE_PRIORITY;
+ String slaveState = "Slave";
+ int slavePriority = 2;
+
+ @Test
+ public void testConstructRepliaWithResourceConfig() throws IOException {
+// Init assignable replication with a basic config object
Review comment:
replication -> replica?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r309939616
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
Review comment:
What does the string represent?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on issue #362: The WAGED rebalancer cluster model implementation
narendly commented on issue #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#issuecomment-517781121 This is a big PR! I think it would be a good idea to keep the size of PRs smaller if possible. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310221038
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterModel.java
##
@@ -0,0 +1,108 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+public class TestClusterModel extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ Set generateNodes(ResourceControllerDataProvider testCache) {
+Set nodeSet = new HashSet<>();
+testCache.getInstanceConfigMap().values().stream().forEach(config ->
nodeSet
+.add(new AssignableNode(testCache, config.getInstanceName(),
Collections.emptyList(;
+return nodeSet;
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignableReplicas = generateReplicas(testCache);
+Set assignableNodes = generateNodes(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignableReplicas, 2);
+ClusterModel clusterModel =
+new ClusterModel(context, assignableReplicas, assignableNodes,
Collections.emptyMap(),
+Collections.emptyMap());
+
+
Assert.assertTrue(clusterModel.getContext().getAssignmentForFaultZoneMap().values().stream()
+.allMatch(v -> v.values().isEmpty()));
+Assert.assertFalse(clusterModel.getAssignableNodes().values().stream()
+.anyMatch(n -> n.getCurrentAssignmentCount() != 0));
+
+// The initialization of the context, node and replication has been tested
separately. So for
+// cluster model, focus on testing the assignment and release.
+
+// Assign
+AssignableReplica replica = assignableReplicas.iterator().next();
+AssignableNode node = assignableNodes.iterator().next();
+clusterModel
+.assign(replica.getResourceName(), replica.getPartitionName(),
replica.getReplicaState(),
+node.getInstanceName());
+
+Assert.assertTrue(
+
clusterModel.getContext().getAssignmentForFaultZoneMap().get(node.getFaultZone())
+
.get(replica.getResourceName()).contains(replica.getPartitionName()));
+
Assert.assertTrue(node.getCurrentAssignmentsMap().get(replica.getResourceName())
+.contains(replica.getPartitionName()));
+
+// Assign a non-exist replication
Review comment:
nonexistent
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
