Improve GetWritablePartitionIds Performance for PUT Requests #1197

ankagrawal · 2019-06-19T22:34:33Z

For each PUT request, instead of looping through all the partitions, and instead of checking replica states of all the partitions before selecting a random partition for put; this change gets a partition at random, and then checks if it is writable and if it’s replicas are available. The motivation for this change is present at https://docs.google.com/document/d/1DWXlUD1fxYkvVekl6tNFsxA7l_eaQrhb6-ht1rIb4G0/edit?ts=5d0ab0d1#

NOTE: For CompositeClusterManager, we need metrics for comparing HelixClusterManager state and StaticClusterManager state. Since the CompositeClusterManager is used only during migrations, this change trades-off PUT latency during migration for the additional metrics. Hence, for HelixClusterManager, getting a writable partition for PUT involves getting all the partitions and checking each partition's replica state, before selecting a writable partition at random.

codecov-io · 2019-06-19T22:52:25Z

Codecov Report

Merging #1197 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1197   +/-   ##
=========================================
  Coverage     83.62%   83.62%           
  Complexity       57       57           
=========================================
  Files             6        6           
  Lines           348      348           
  Branches         38       38           
=========================================
  Hits            291      291           
  Misses           45       45           
  Partials         12       12

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 367c57c...2e4e1cb. Read the comment docs.

jsjtzyy

Will take a look at testing soon.
(Please format all the files)

jsjtzyy · 2019-06-21T19:16:49Z

ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java

+      List<PartitionId> partitionsInClass = getPartitionsInClass(partitionClass, true);
+
+      int workingSize = partitionsInClass.size();
+      while(true) {


How about changing this to while (workingSize > 0) and remove if-statement at line 424.

jsjtzyy · 2019-06-21T19:17:12Z

ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java

@@ -405,6 +406,42 @@ void updatePartitions(Collection<? extends PartitionId> allPartitions, String lo
      return healthyWritablePartitions.isEmpty() ? writablePartitions : healthyWritablePartitions;
    }

+    /**


minor: format this file

jsjtzyy · 2019-06-21T19:31:25Z

ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java

+        if(partitionsToExclude == null || partitionsToExclude.size() == 0 || !partitionsToExclude.contains(selected)) {
+          if (selected.getPartitionState() == PartitionState.READ_WRITE) {
+            anyWritablePartition = selected;
+            if(areAllReplicasForPartitionUp(selected)) {


I remember during offline discussion, you even proposed to only check if local replicas are up. Will we try that in the future PR?

jsjtzyy · 2019-06-21T19:34:36Z

ambry-clustermap/src/main/java/com.github.ambry.clustermap/CompositeClusterManager.java

+   * getWritablePartitionIds.
+   * @param partitionClass the partition class whose writable partitions are required. Can be {@code null}
+   * @param partitionsToExclude A list of partitions that should be excluded from consideration.
+   * @return A writable parition id object. Can be {@code null}


minor: typo in partition

jsjtzyy · 2019-06-21T19:35:34Z

ambry-clustermap/src/main/java/com.github.ambry.clustermap/CompositeClusterManager.java

+  public PartitionId getRandomWritablePartition(String partitionClass, List<PartitionId> partitionsToExclude) {
+    List<? extends PartitionId> partitions = getWritablePartitionIds(partitionClass);
+    partitions.removeAll(partitionsToExclude);
+    return partitions.isEmpty()?null:partitions.get(ThreadLocalRandom.current().nextInt(partitions.size()));


minor: format this file

fixed. I am assuming that you meant formatting for line 128.

ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java

ambry-clustermap/src/main/java/com.github.ambry.clustermap/CompositeClusterManager.java

jsjtzyy · 2019-06-21T22:08:21Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/ClusterMapUtilsTest.java

    assertCollectionEquals("Partitions returned not as expected", expectedReturnForClassBeingTested,
        psh.getWritablePartitions(classBeingTested));
+    assertInCollection("Partitions returned not as expected", allPartitionIds,


The message should be Random partition returned not as expected if you want to make it consistent in this test.

jsjtzyy · 2019-06-21T22:12:08Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/ClusterMapUtilsTest.java

+    assertInCollection("Partitions returned not as expected", allPartitionIds,
+        psh.getRandomWritablePartition(null, null));
+    assertNull("Partitions returned not as expected",
+        psh.getRandomWritablePartition(classBeingTested, null));


This method contains some duplicated assertions. For example: line241-248, line255-262, line268-275 and line289-296. How about moving them to a separate method and simplify this test?

jsjtzyy · 2019-06-24T18:01:34Z

@ankagrawal Looks like we need to fix failure in listenCrossColoTest with new changes.

ankagrawal · 2019-06-24T18:28:10Z

@ankagrawal Looks like we need to fix failure in listenCrossColoTest with new changes.

Yeah.. this change for local DC was work in progress. I forgot that push to a branch generates a new review. Sorry about that.

…t latency by not iterating through all the partitions. Add a method getRandomWritablePartition to ClusterMap and add its implementation to HelixClusterManager, StaticClusterManager and CompositeClusterManager.

Improve get random partition logic

ankagrawal · 2019-06-25T20:53:44Z

@ankagrawal Looks like we need to fix failure in listenCrossColoTest with new changes.

Yeah.. this change for local DC was work in progress. I forgot that push to a branch generates a new review. Sorry about that.

The new tests have passed without any changes. I guess this was an intermitted issue. We should keep an eye if this listenCrossColo test fails again, and root-cause this.

lightningrob · 2019-06-26T01:45:19Z

ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java

+      List<PartitionId> partitionsInClass = getPartitionsInClass(partitionClass, true);
+
+      int workingSize = partitionsInClass.size();
+      while(workingSize > 0) {


Looks like formatting is still off. Should have space between while/if and (

fixed all the formatting errors.

lightningrob · 2019-06-26T01:52:01Z

ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java

+            anyWritablePartition = selected;
+            if(areAllLocalReplicasForPartitionUp(selected, localDatacenterName)) {
+              selectedPartition = selected;
+              break;


Can't we just return selected here?

jsjtzyy

Mostly looks good to me. One minor comment regarding improvement on checking state of local replicas.

jsjtzyy · 2019-06-26T03:43:54Z

ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java

+        logger.debug("Replica [{}] on {} {} is down", replica.getPartitionId().toPathString(),
+            replica.getDataNodeId().getHostname(), replica.getMountPath());
+        return false;
+      }


I think you can maintain a Map inside PartitionSelectionHelper, which will be populated during initialization. The Map keeps each partition and a list of its local replicas, i.e Map<PartitionId, List<ReplicaId>>.
The benefit is, when this method is called, it only needs to check 3 replicas directly rather than 12 replicas in worst case.

@ankagrawal, did you want to make this change or should I merge the PR?

ankagrawal requested review from cgtz, lightningrob and jsjtzyy June 20, 2019 18:03

jsjtzyy reviewed Jun 21, 2019

View reviewed changes

cgtz suggested changes Jun 21, 2019

View reviewed changes

jsjtzyy reviewed Jun 21, 2019

View reviewed changes

ankagrawal added 5 commits June 24, 2019 18:29

Choose a writable partition at random for put calls. This improved pu…

a6a3dfb

…t latency by not iterating through all the partitions. Add a method getRandomWritablePartition to ClusterMap and add its implementation to HelixClusterManager, StaticClusterManager and CompositeClusterManager.

cleanup dead code

73daf0e

cleanup

1a23b23

Add unit tests

6e8ce87

Improve get random partition logic

Check only local dataenter replica status for PUT

7d3c702

ankagrawal force-pushed the FixGetwritablePatitionId branch from 3089002 to 7d3c702 Compare June 25, 2019 17:59

ankagrawal closed this Jun 25, 2019

ankagrawal reopened this Jun 25, 2019

ankagrawal added 2 commits June 25, 2019 15:26

Format

a527757

Refactor and comments

6573b41

lightningrob reviewed Jun 26, 2019

View reviewed changes

Format and cleanup

58ca0e0

jsjtzyy approved these changes Jun 26, 2019

View reviewed changes

cgtz approved these changes Jun 28, 2019

View reviewed changes

Minor improvement by prepopulating a map of local replicas per partition

2e4e1cb

jsjtzyy merged commit a24244c into linkedin:master Jul 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve GetWritablePartitionIds Performance for PUT Requests #1197

Improve GetWritablePartitionIds Performance for PUT Requests #1197

ankagrawal commented Jun 19, 2019

codecov-io commented Jun 19, 2019 •

edited

Loading

jsjtzyy left a comment

jsjtzyy Jun 21, 2019 •

edited

Loading

ankagrawal Jun 25, 2019

jsjtzyy Jun 21, 2019

ankagrawal Jun 25, 2019

jsjtzyy Jun 21, 2019

ankagrawal Jun 25, 2019

jsjtzyy Jun 21, 2019

ankagrawal Jun 25, 2019

jsjtzyy Jun 21, 2019

ankagrawal Jun 25, 2019

jsjtzyy Jun 21, 2019

ankagrawal Jun 26, 2019

jsjtzyy Jun 21, 2019 •

edited

Loading

ankagrawal Jun 25, 2019

ankagrawal Jun 26, 2019

jsjtzyy commented Jun 24, 2019

ankagrawal commented Jun 24, 2019

ankagrawal commented Jun 25, 2019

lightningrob Jun 26, 2019

ankagrawal Jun 26, 2019

lightningrob Jun 26, 2019

ankagrawal Jun 26, 2019

jsjtzyy left a comment

jsjtzyy Jun 26, 2019

cgtz Jun 28, 2019

Improve GetWritablePartitionIds Performance for PUT Requests #1197

Improve GetWritablePartitionIds Performance for PUT Requests #1197

Conversation

ankagrawal commented Jun 19, 2019

codecov-io commented Jun 19, 2019 • edited Loading

Codecov Report

jsjtzyy left a comment

Choose a reason for hiding this comment

jsjtzyy Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjtzyy Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjtzyy commented Jun 24, 2019

ankagrawal commented Jun 24, 2019

ankagrawal commented Jun 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjtzyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 19, 2019 •

edited

Loading

jsjtzyy Jun 21, 2019 •

edited

Loading

jsjtzyy Jun 21, 2019 •

edited

Loading