Integrate statistical measurement in the final OptimalAssignment output by i3wangyi · Pull Request #486 · apache/helix

i3wangyi · 2019-09-19T01:37:22Z

Issues

My PR addresses the following Helix issues and references them in the PR description:

Description

Here are some details about my PR, including screenshots of any UI changes:

(Write a concise description including what, why, how)
Added CV and movements measurement tools for the statistical understanding of the final optimal assignment

Tests

The following tests are written for this issue:

(List the names of added unit/integration tests)

The following is the result of the "mvn test" command on the appropriate module:

(Copy & paste the result of "mvn test")

Commits

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":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Code Quality

My diff has been formatted using helix-style.xml

narendly · 2019-09-19T04:56:21Z

+    for (AssignableNode instance : instances) {
+      Map<String, Integer> capacityUsage = instance.getCapacityUsage();
+      for (String key : capacityUsage.keySet()) {
+        usages.computeIfAbsent(key, k -> new ArrayList<>()).add(capacityUsage.get(key));


Could you spell out lambda variables? Like capacityKey, usageKey, etc.

narendly · 2019-09-19T04:56:25Z

+    }
+
+    return usages.entrySet().stream()
+        .collect(Collectors.toMap(Map.Entry::getKey, e -> getCoefficientOfVariation(e.getValue())));


Could you spell out lambda variables? Like capacityKey, usageKey, etc.

narendly · 2019-09-19T04:57:24Z

+   * @return a simple cumulative count of total movements where differences of movements in terms of
+   *         location, size, etc is ignored
+   */
+  public int getTotalPartitionMovements(Map<String, ResourceAssignment> baseAssignment) {


baseAssignment -> previousAssignment?

Commented in the javadoc, the purpose of the method is to allow caller to pass any resource assignments (baseline or bestpossible) and it will calculate the movements

narendly · 2019-09-19T04:58:09Z

+   */
+  public int getTotalPartitionMovements(Map<String, ResourceAssignment> baseAssignment) {
+    Map<String, ResourceAssignment> optimalAssignment = getOptimalResourceAssignment();
+    int movements = 0;


movements -> movedPartitionCount to be clear.

narendly · 2019-09-19T05:01:59Z

+        ResourceAssignment lastResourceAssignment = baseAssignment.get(resource);
+        for (Partition partition : resourceAssignment.getMappedPartitions()) {
+          Map<String, String> thisInstanceToStates =
+              new HashMap<>(resourceAssignment.getReplicaMap(partition));


Do we really need to copy the map? Possible to synchronize on the assignment or something?

I'm worried that this might add a significant amount of GC pressure since we'd be doing this for the entire assignment.

On my understanding, this logic is for tuning/testing purposes. So if they are not in this file, but in some tuning class instead, it should be fine.

narendly · 2019-09-19T05:03:19Z

        Partition partition = new Partition(replica.getPartitionName());
-        ResourceAssignment resourceAssignment = assignmentMap
-            .computeIfAbsent(resourceName, key -> new ResourceAssignment(resourceName));
+        ResourceAssignment resourceAssignment = assignmentMap.computeIfAbsent(resourceName,


Why does formatting keep changing?

narendly · 2019-09-19T05:05:40Z

-  public String getFailures() {
-    // TODO: format the error string
-    return _failedAssignments.toString();
+  public String getErrorMessage() {


I'm afraid this might pollute the log big time.

What value do we have in printing out all failed assignments? I thought as soon as we encounter a failed assignment, we just terminate? So what we'll see here is always just the first failed assignment?

jiajunwang

In general, the measurement logic is for testing purpose only. It is not a good idea to pollute the production code with testing logic.
Could you please create a separate class for the testing? One option is to create a child class of OptimalAssignment. Then adding the measurement code there. Or you just have a standalone measurement method in the tuning code.

Note that if you put the code in OptimalAssignment, anyone who modifies it will be forced to maintain the measurement code as well.

jiajunwang · 2019-09-19T05:19:05Z

        String errorMessage = String.format(
            "Unable to find any available candidate node for partition %s; Fail reasons: %s",
-            replica.getPartitionName(), optimalAssignment.getFailures());
+            replica.getPartitionName(), optimalAssignment.getErrorMessage());


OptimalAssignment is not an Exception.
The meaning of "ErrorMessage" is not clear.

I would prefer to call it AssignmentFailures or just keep it as it was.

jiajunwang · 2019-09-19T05:21:12Z


+
+  //TODO: will implement by merging other changes
+  public Map<String, Integer> getCapacityUsage() {


Let's don't make it public since it is not supporting the feature.
I would prefer the model class to be clean and no extra method, as long as it is possible.

jiajunwang · 2019-09-19T05:22:37Z

+        ResourceAssignment lastResourceAssignment = baseAssignment.get(resource);
+        for (Partition partition : resourceAssignment.getMappedPartitions()) {
+          Map<String, String> thisInstanceToStates =
+              new HashMap<>(resourceAssignment.getReplicaMap(partition));


On my understanding, this logic is for tuning/testing purposes. So if they are not in this file, but in some tuning class instead, it should be fine.

Integrate statistical measurement in the final OptimalAssignment output

feca8dd

narendly reviewed Sep 19, 2019

View reviewed changes

jiajunwang reviewed Sep 19, 2019

View reviewed changes

i3wangyi closed this Sep 25, 2019



		//TODO: will implement by merging other changes
		public Map<String, Integer> getCapacityUsage() {

Conversation

i3wangyi commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Description

Tests

Commits

Documentation

Code Quality

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiajunwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

i3wangyi commented Sep 19, 2019 •

edited

Loading