Support adding/removing store via AdminRequest #1259

jsjtzyy · 2019-09-09T20:53:11Z

Refactored store control admin request to support adding/removing store.
Changes made in replication manager to support adding/removing replica
Added method in clustermap that allows caller to get detailed info of new replica from Helix PropertyStore.
Changed removeBlobStore in StorageManager to return the store if it is successfully removed.
Moved MockStorageManager to a separate file which can be re-used by other tests.

jsjtzyy · 2019-09-09T20:54:15Z

Still working on unit tests and integration tests. Also this PR partially depends on #1249

codecov-io · 2019-09-09T21:23:05Z

Codecov Report

Merging #1259 into master will increase coverage by 0.05%.
The diff coverage is 88.54%.

@@             Coverage Diff              @@
##             master    #1259      +/-   ##
============================================
+ Coverage     72.09%   72.14%   +0.05%     
- Complexity     6232     6258      +26     
============================================
  Files           449      450       +1     
  Lines         35744    35888     +144     
  Branches       4540     4552      +12     
============================================
+ Hits          25769    25893     +124     
- Misses         8796     8809      +13     
- Partials       1179     1186       +7

Impacted Files	Coverage Δ	Complexity Δ
...va/com.github.ambry.cloud/CloudTokenPersistor.java	`81.81% <ø> (ø)`	`5 <0> (ø)`	⬇️
.../main/java/com.github.ambry.cloud/VcrRequests.java	`100% <ø> (ø)`	`10 <0> (ø)`	⬇️
...om.github.ambry.replication/ReplicationEngine.java	`82.2% <ø> (ø)`	`38 <0> (ø)`	⬇️
...rc/main/java/com.github.ambry.store/BlobStore.java	`90.23% <ø> (ø)`	`97 <0> (ø)`	⬇️
...ithub.ambry.replication/ReplicaTokenPersistor.java	`86.2% <ø> (ø)`	`10 <0> (ø)`	⬇️
...m.github.ambry.replication/DiskTokenPersistor.java	`77.27% <ø> (ø)`	`5 <0> (ø)`	⬇️
....github.ambry.clustermap/StaticClusterManager.java	`71.31% <0%> (+1.35%)`	`83 <0> (+1)`	⬆️
.../com.github.ambry/tools/admin/ServerAdminTool.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...thub.ambry.clustermap/CompositeClusterManager.java	`61.7% <0%> (-0.45%)`	`22 <0> (ø)`
....github.ambry.protocol/BlobStoreControlAction.java	`100% <100%> (ø)`	`1 <1> (?)`
... and 27 more

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 a154380...aaa32c4. Read the comment docs.

jsjtzyy · 2019-09-12T22:59:56Z

I may add more tests into this PR but the production code is ready for review.

jsjtzyy · 2019-09-16T18:47:12Z

This PR contains lots of changes and refactoring in testing. Reviewers can start with production code first. (Now the PR is completely ready for review.)

jsjtzyy · 2019-09-23T22:21:27Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+   * @param replicaId the replica to remove
+   * @return {@code true} if replica is successfully removed. {@code false} otherwise
+   */
+  public boolean removeReplica(ReplicaId replicaId) {


Here same method signature is intentionally adopted for this PR and #1262, which allows us to put an abstract method ReplicationEngine. Both regular ReplicationManager and VcrReplicationManager.

zzmao · 2019-09-25T04:00:54Z

Have some difficulties to understand populateRemoteReplicaAndPartitionInfos and getNewReplica. Need to talk offline.

zzmao

Some comments left. Most of the logic LGTM!

zzmao · 2019-09-26T03:58:42Z

ambry-api/src/main/java/com.github.ambry/clustermap/ClusterMap.java

@@ -122,6 +122,15 @@
   */
  JSONObject getSnapshot();

+  /**
+   * Get new replica of certain partition that resides on given host.


Is it possible that one host have multiple new replicas? If not, can you add some comments regarding the assumption?

It's possible that one host has several new replicas but they should come from different partitions. In this method, neither argument partitionIdStr nor return value ReplicaId is collection, because the method is supposed to be called by state model where each partition get its new replica separately. I will add some comments for this method.

zzmao · 2019-09-26T04:03:54Z

ambry-api/src/main/java/com.github.ambry/clustermap/ClusterMap.java

+  /**
+   * Get new replica of certain partition that resides on given host.
+   * @param partitionIdStr the partition id string
+   * @param hostname the host on which replica should reside.


currentNode.hostname and currentNode.port are passed in, why not use DataNode as argument?

good idea. I will figure out a way to pass in DataNodeId

If you decide to do this, then you can use dataNodeId::compareTo methods in places you are comparing hostname and port combination.

sure, thanks for letting me know.

zzmao · 2019-09-26T04:15:43Z

ambry-protocol/src/main/java/com.github.ambry.protocol/BlobStoreControlAdminRequest.java

+        controlRequestType =
+            stream.readByte() == 1 ? BlobStoreControlRequestType.StartStore : BlobStoreControlRequestType.StopStore;
+        break;
+      case VERSION_V2:


Is VERSION_V2 needed? Look like BlobStoreControlRequestType.values()[stream.readByte()] can fully cover VERSION_V1.

In what case will controlRequestType for VERSION_V1 be not 1? What are the other valid values it can have?

maybe you dont need different cases for two versions.

Thanks @zzmao @ankagrawal for your inputs here. You are right, no need to keep version here. I will simplify this piece of code.

zzmao · 2019-09-26T04:17:54Z

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

@@ -533,6 +533,12 @@ public JSONObject getSnapshot() {
    return snapshot;
  }

+  @Override
+  public ReplicaId getNewReplica(String partitionIdStr, String hostname, Integer port) {
+    logger.warn("Adding new replica is currently not supported in static cluster manager. Return null here");


throw UnsupportedException?

zzmao · 2019-09-26T04:20:42Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+   * @param replicaId the local replica
+   * @return list of {@link RemoteReplicaInfo} associated with local replica.
+   */
+  private List<RemoteReplicaInfo> populateRemoteReplicaAndPartitionInfos(List<? extends ReplicaId> peerReplicas,


It's good to split this function to two: createRemoteReplicaInfos() and updatePartitionInfoMaps

zzmao · 2019-09-26T04:29:19Z

ambry-store/src/main/java/com.github.ambry.store/DiskManager.java

@@ -231,12 +231,13 @@ void shutdown() throws InterruptedException {

  /**
   * @param id the {@link PartitionId} to find the store for.
+   * @param skipStateCheck whether to skip checking state of store.


Introduce skipStateCheck makes the interface complex and hard to understand. Can we alway return store and check isStarted() in the caller?

zzmao · 2019-09-26T04:36:21Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+    if (peerReplicas != null) {
+      remoteReplicaInfos = populateRemoteReplicaAndPartitionInfos(peerReplicas, replicaId);
+    }
+    logger.info("Assigning thread for {}", replicaId.getPartitionId());


Move the logic below under if (peerReplicas != null) {?

zzmao · 2019-09-26T04:46:25Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+    List<RemoteReplicaInfo> remoteReplicaInfos = partitionInfo.getRemoteReplicaInfos();
+    logger.info("Removing remote replicas of {} from replica threads", replicaId.getPartitionId());
+    removeRemoteReplicaInfoFromReplicaThread(remoteReplicaInfos);
+    mountPathToPartitionInfos.get(replicaId.getMountPath()).remove(partitionInfo);


computeIfPresent is atomic for concurrent map. This can be used to avoid heavy copyOnWriteArrayList

good point. Let me remove copyOnWriteArrayList

ankagrawal

Initial comments. Will continue to review.

ankagrawal · 2019-09-27T13:25:09Z

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

@@ -533,6 +533,12 @@ public JSONObject getSnapshot() {
    return snapshot;
  }

+  @Override
+  public ReplicaId getNewReplica(String partitionIdStr, String hostname, Integer port) {
+    logger.warn("Adding new replica is currently not supported in static cluster manager. Return null here");


ankagrawal · 2019-09-27T13:28:52Z

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

    for (PartitionId partitionId : partitions.values()) {
      List<? extends ReplicaId> replicaIds = partitionId.getReplicaIds();
      for (ReplicaId replicaId : replicaIds) {
-        if (replicaId.getDataNodeId().getHostname().compareTo(dataNodeId.getHostname()) == 0
+        if (replicaId.getDataNodeId().getHostname().equals(dataNodeId.getHostname())


My latest commit has changed the way compareTo is implemented for datanode. You can use replicaId.getDataNodeId().compareTo(dataNodeId.getHostName()) == 0

cool, will take your advice.

ankagrawal · 2019-09-27T13:30:22Z

ambry-api/src/main/java/com.github.ambry/clustermap/ClusterMap.java

+  /**
+   * Get new replica of certain partition that resides on given host.
+   * @param partitionIdStr the partition id string
+   * @param hostname the host on which replica should reside.


If you decide to do this, then you can use dataNodeId::compareTo methods in places you are comparing hostname and port combination.

ankagrawal · 2019-09-27T13:33:13Z

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

+  @Override
+  public ReplicaId getNewReplica(String partitionIdStr, String hostname, Integer port) {
+    ReplicaId newReplica = null;
+    for (Map.Entry<Long, PartitionId> entry : partitions.entrySet()) {


we can simplify this by converting partitionIdStr to long partitionId and then do partitions.get(partitionId).

correct, changes are made as you suggested

ankagrawal · 2019-09-27T13:39:57Z

ambry-protocol/src/main/java/com.github.ambry.protocol/BlobStoreControlAdminRequest.java

+        controlRequestType =
+            stream.readByte() == 1 ? BlobStoreControlRequestType.StartStore : BlobStoreControlRequestType.StopStore;
+        break;
+      case VERSION_V2:


In what case will controlRequestType for VERSION_V1 be not 1? What are the other valid values it can have?

ankagrawal · 2019-09-27T13:41:00Z

ambry-protocol/src/main/java/com.github.ambry.protocol/BlobStoreControlAdminRequest.java

  }

-  public BlobStoreControlAdminRequest(short numReplicasCaughtUpPerPartition, boolean enable,
-      AdminRequest adminRequest) {
+  public BlobStoreControlAdminRequest(short numReplicasCaughtUpPerPartition,


javadoc for this constructor?

ankagrawal · 2019-09-27T13:42:28Z

ambry-protocol/src/main/java/com.github.ambry.protocol/BlobStoreControlAdminRequest.java

+        controlRequestType =
+            stream.readByte() == 1 ? BlobStoreControlRequestType.StartStore : BlobStoreControlRequestType.StopStore;
+        break;
+      case VERSION_V2:


maybe you dont need different cases for two versions.

ankagrawal · 2019-09-27T13:57:21Z

ambry-store/src/main/java/com.github.ambry.store/DiskManager.java

   * @return the associated {@link Store}, or {@code null} if the partition is not on this disk, or the store is not
   *         started.
   */
-  Store getStore(PartitionId id) {


There are lot of "getStore(PartitionId, false)" calls throughout the code. I would suggest overload getStore as getStore(PArtitionId) and getStore(PartitionId, boolean). And then getStore(PartitionId) just calls getStore(PartitionId, false);

right, I made the change as you suggested.

Is getStore(PartitionId, false) really needed? Can we add extra logic to check after getStore(PartitionId)?

Less interface is better than more.

I see your point here. I temporarily don't want to change current logic in most cases where getStore(partitionId) is called. However, I do need to get the store that is already shutdown when removing replica. Note that, remove replica always happens after stop replica. Hence, this method allows us to get a "stopped" store.

jsjtzyy · 2019-10-08T17:31:43Z

Gentle reminder to review. Thanks in advance.

ankagrawal

Few minor concerns. Lets quickly discuss.

ankagrawal · 2019-10-09T18:35:39Z

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

+              new AmbryPartition(Long.valueOf(partitionIdStr), partitionClass, helixClusterManagerCallback);
+        }
+        // Check if data node or disk is in current cluster map, if not, set newReplica to null.
+        AmbryDataNode dataNode = instanceNameToAmbryDataNode.get(instanceName);


Who calls getNewReplica? Can it be called for a node when the partition doesnt even exist on the node as per clustermap?

Discussed this offline. Currently AmbryRequests calls this method. Eventually, state model of each partition will call this method.

ankagrawal · 2019-10-09T18:37:12Z

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

+        String mountPathFromHelix = replicaInfos.get(instanceName);
+        Set<AmbryDisk> disks = dataNode != null ? ambryDataNodeToAmbryDisks.get(dataNode) : null;
+        Optional<AmbryDisk> potentialDisk =
+            disks != null ? disks.stream().filter(d -> d.getMountPath().equals(mountPathFromHelix)).findAny()


so when someone creates a new partition, he/she will ensure that the mount path exists on the node?

SRE is supposed to guarantee new added replica has valid mount path otherwise the creation should fail.

ankagrawal · 2019-10-09T18:56:25Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+      v.remove(partitionInfo);
+      return v;
+    });
+    //mountPathToPartitionInfos.get(replicaId.getMountPath()).remove(partitionInfo);


remove this commented line?

ankagrawal · 2019-10-09T19:07:07Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+    if (!partitionToPartitionInfo.containsKey(replicaId.getPartitionId())) {
+      logger.error("{} doesn't exist in replication manager, skipping removing replica request.",
+          replicaId.getPartitionId());
+      return false;


Can the add/remove replica code cause thread safety concerns ? Let's discuss! There are some subtle differences between ConcurrentMap and ConcurrentHashMap and we should try to explicitly use ConcurrentHashMap is we want certain guarantees.

Good point. I decided to add ReadWriteLock in replication manager and disk manager.

cgtz

Continuing to review

cgtz · 2019-09-30T20:16:46Z

ambry-api/src/main/java/com.github.ambry/clustermap/ClusterMap.java

@@ -122,6 +122,14 @@
   */
  JSONObject getSnapshot();

+  /**
+   * Attempt to get new replica of certain partition that resides on given data node.


Could you explain what a "new replica" is in this javadoc? i.e. what makes a "new" replica different from a regular replica.

This is actually "new replica" that will be added to specified datanode. Currently, it is used in HelixClusterManager only because target node needs replica detailed info to create it. The replica info is stored in Helix PropertyStore. Before moving replica, we use Helix Bootstrap tool to upload new replica infos onto Helix PropertyStore, thus target node can fetch this info to create new replica. Let me know if you come up with a better name to disambiguate in this context. Thanks

cgtz · 2019-10-10T01:20:14Z

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

+        Optional<AmbryDisk> potentialDisk =
+            disks != null ? disks.stream().filter(d -> d.getMountPath().equals(mountPathFromHelix)).findAny()
+                : Optional.empty();
+        if (dataNode != null && potentialDisk.isPresent()) {


Can you remove the dataNode != null check since you check that on line 407?

sure, fair point.

cgtz · 2019-10-10T01:22:47Z

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

+  @Override
+  public ReplicaId getNewReplica(String partitionIdStr, DataNodeId dataNodeId) {
+    throw new UnsupportedOperationException(
+        "Adding new replica is currently not supported in static cluster manager. Return null here");


Should this return null like the exception message says?

updated the exception message. Feel like return null is unneeded.

cgtz · 2019-10-10T01:30:03Z

ambry-protocol/src/main/java/com.github.ambry.protocol/BlobStoreControlAdminRequest.java

-      throw new IllegalStateException("Unrecognized version for BlobStoreControlAdminRequest: " + versionId);
-    }
+    // read the version
+    stream.readShort();


So, are we just erasing all remnants of V1 of this request? I guess thats ok since this is an admin operation that isn't called by the router in a live environment. However, you should probably add some notes that say why v1 is not present?

Do you still want to have the check that the version number matches the expected value (2) ?

I feel like maybe we can still use V1 for this request. Current change doesn't really modify serialize/deserialize logic of BlobStoreControlAdminRequest, it interprets stream.readByte() as a new introduced enum type instead of boolean value. I think keeping using V1 makes sense here (also, we can keep checking version number in this method).

Yep, I agree that in this circumstance it makes sense to keep using v1

cgtz · 2019-10-10T01:31:38Z

ambry-protocol/src/main/java/com.github.ambry.protocol/BlobStoreControlRequestType.java

+ * The order of these enums should not be changed since their relative position goes into the serialized form of
+ * requests.
+ */
+public enum BlobStoreControlRequestType {


BlobStoreControlAction?

makes more sense, will change as you suggested.

cgtz · 2019-10-10T01:41:05Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+   */
+  public boolean removeReplica(ReplicaId replicaId) {
+    boolean succeed = false;
+    rwLock.readLock().lock();


why do all of these methods that mutate the data structures take the read lock? shouldn't they take the write lock since they mutate the data structure?

I don't see any usages of the write lock in this class. Typically a readwritelock is used to allow for concurrent readers during normal scenarios (by using the read lock), but lock the entire datastructure (ensure that all readers finish there reads and make future readers wait by using the write lock) when editing.

right, will change this to simple ReentrantLock.

With just a simple mutex, wouldn't you have to synchronize all reads/writes to these data structures? Would it be better to take a read lock when reading from the shared data structures and taking a write lock when editing them? We can discuss in person.

cgtz · 2019-10-10T01:44:22Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+      FindToken findToken =
+          this.tokenHelper.getFindTokenFactoryFromReplicaType(remoteReplica.getReplicaType()).getNewFindToken();
+      RemoteReplicaInfo remoteReplicaInfo = new RemoteReplicaInfo(remoteReplica, replicaId, store, findToken,
+          storeConfig.storeDataFlushIntervalSeconds * SystemTime.MsPerSec * Replication_Delay_Multiplier,


TimeUnit.SECONDS.toMillis(storeConfig.storeDataFlushIntervalSeconds)

cgtz · 2019-10-10T01:47:50Z

ambry-store/src/main/java/com.github.ambry.store/DiskManager.java

    BlobStore store =
        new BlobStore(replica, storeConfig, scheduler, longLivedTaskScheduler, diskIOScheduler, diskSpaceAllocator,
            storeMainMetrics, storeUnderCompactionMetrics, keyFactory, recovery, hardDelete, replicaStatusDelegate,
            time);
+    rwLock.readLock().lock();


Same comment for this class.

cgtz · 2019-10-10T01:58:50Z

ambry-server/src/main/java/com.github.ambry.server/AmbryRequests.java

          }
        } else {
-          logger.debug("Validate request fails for {} with error code {} when trying to stop store", partitionId,
-              error);
+          errorCode = ServerErrorCode.Unknown_Error;


For these handle* methods, it might be more readable to intersperse the method with returns since otherwise there are a ton of nested layers of indentation.
e.g.

if (!success1) { return errorCode1; } if (!success2) { return errorCode2; } ... return No_Error

cgtz · 2019-10-10T02:15:10Z

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

+          logger.info("Partition {} is currently not present in cluster map, creating a new partition.",
+              partitionIdStr);
+          mappedPartition =
+              new AmbryPartition(Long.valueOf(partitionIdStr), partitionClass, helixClusterManagerCallback);


intellij suggests using Long.parseLong here to avoid unnecessary boxing

cgtz · 2019-10-10T02:16:03Z

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

+      Map<String, Map<String, String>> partitionToReplicas = zNRecord.getMapFields();
+      Map<String, String> replicaInfos = partitionToReplicas.get(partitionIdStr);
+      if (replicaInfos != null && replicaInfos.containsKey(instanceName)) {
+        long replicaCapacity = Long.valueOf(replicaInfos.get(REPLICAS_CAPACITY_STR));


cgtz · 2019-10-10T02:18:39Z

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

+        // Check if data node or disk is in current cluster map, if not, set newReplica to null.
+        AmbryDataNode dataNode = instanceNameToAmbryDataNode.get(instanceName);
+        String mountPathFromHelix = replicaInfos.get(instanceName);
+        Set<AmbryDisk> disks = dataNode != null ? ambryDataNodeToAmbryDisks.get(dataNode) : null;


Could also do

Optional<AmbryDisk> potentialDisk = Optional.ofNullable(dataNode != null ? ambryDataNodeToAmbryDisks.get(dataNode) : null) .flatMap(disks -> disks.stream().filter(d -> d.getMountPath().equals(mountPathFromHelix)).findAny());

cgtz · 2019-10-10T02:31:34Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+        logger.info("Removing remote replicas of {} from replica threads", replicaId.getPartitionId());
+        removeRemoteReplicaInfoFromReplicaThread(remoteReplicaInfos);
+        mountPathToPartitionInfos.computeIfPresent(replicaId.getMountPath(), (k, v) -> {
+          v.remove(partitionInfo);


since you are removing arbitrary elements from v, maybe it should be a HashSet instead of ArrayList

cgtz · 2019-10-10T02:34:51Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+      } else {
+        List<? extends ReplicaId> peerReplicas = replicaId.getPeerReplicaIds();
+        List<RemoteReplicaInfo> remoteReplicaInfos = new ArrayList<>();
+        if (peerReplicas != null) {


what is the case where peerReplicas is null?

cgtz

Approved, just a couple more comments to optionally address

cgtz · 2019-10-11T17:59:49Z

ambry-cloud/src/main/java/com.github.ambry.cloud/VcrReplicationManager.java

@@ -175,9 +177,9 @@ void addReplica(PartitionId partitionId) throws ReplicationException {
      partitionToPartitionInfo.put(partitionId, partitionInfo);
      mountPathToPartitionInfos.compute(cloudReplica.getMountPath(), (key, value) -> {


You can simplify this to the way you did it in CloudTokenPersistorTest:

mountPathToPartitionInfoList.computeIfAbsent(cloudReplicaId.getMountPath(), key -> ConcurrentHashMap.newKeySet()).add(partitionInfo);

cgtz · 2019-10-11T18:02:00Z

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

+   * residing on hosts that are not present in clustermap.
+   * The ZNRecord of REPLICA_ADDITION_ZNODE has following format in mapFields.
+   * <p/>
+   * "mapFields": {


minor: put the json in <pre><pre/> tags. Also for the paragraph separator, I think javadoc nags if you use <p/> instead of <p>.

jsjtzyy self-assigned this Sep 9, 2019

jsjtzyy force-pushed the control-store-by-tool branch from c8a8b5f to 3ccfb32 Compare September 12, 2019 05:32

jsjtzyy marked this pull request as ready for review September 12, 2019 22:32

jsjtzyy requested review from ankagrawal and cgtz September 12, 2019 22:32

jsjtzyy mentioned this pull request Sep 16, 2019

Add capability to handle replication requests in vcr #1262

Merged

jsjtzyy requested a review from zzmao September 20, 2019 20:57

jsjtzyy commented Sep 23, 2019

View reviewed changes

jsjtzyy force-pushed the control-store-by-tool branch from 7ffbd3a to a8588df Compare September 24, 2019 20:32

jsjtzyy force-pushed the control-store-by-tool branch from a8588df to 6d88f02 Compare September 25, 2019 16:54

zzmao reviewed Sep 26, 2019

View reviewed changes

ankagrawal reviewed Sep 27, 2019

View reviewed changes

jsjtzyy force-pushed the control-store-by-tool branch 2 times, most recently from 4161813 to 10a5ef7 Compare September 27, 2019 19:01

jsjtzyy added 2 commits October 8, 2019 22:11

Add/Remove replica via ServerAdminTool

07d5ac1

fix intermittent unit test failure

927e0e5

jsjtzyy force-pushed the control-store-by-tool branch from 68517e2 to 927e0e5 Compare October 9, 2019 05:12

ankagrawal reviewed Oct 9, 2019

View reviewed changes

address comments

dafd89e

cgtz suggested changes Oct 10, 2019

View reviewed changes

cgtz reviewed Oct 10, 2019

View reviewed changes

jsjtzyy added 3 commits October 10, 2019 13:10

address comments

d74c737

fix test failure

da18827

rename some variables

b7eba21

cgtz approved these changes Oct 12, 2019

View reviewed changes

jsjtzyy added 2 commits October 11, 2019 17:25

address Casey's comments

9423783

remove unused method

aaa32c4

zzmao approved these changes Oct 14, 2019

View reviewed changes

zzmao merged commit b22b487 into linkedin:master Oct 14, 2019

		@@ -175,9 +177,9 @@ void addReplica(PartitionId partitionId) throws ReplicationException {
		partitionToPartitionInfo.put(partitionId, partitionInfo);
		mountPathToPartitionInfos.compute(cloudReplica.getMountPath(), (key, value) -> {

Support adding/removing store via AdminRequest #1259

Support adding/removing store via AdminRequest #1259

Conversation

jsjtzyy commented Sep 9, 2019 • edited Loading

jsjtzyy commented Sep 9, 2019

codecov-io commented Sep 9, 2019 • edited Loading

Codecov Report

jsjtzyy commented Sep 12, 2019

jsjtzyy commented Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

zzmao commented Sep 25, 2019

zzmao left a comment

Choose a reason for hiding this comment

zzmao Sep 26, 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

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

ankagrawal left a 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

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 commented Oct 8, 2019

ankagrawal left a 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

cgtz left a 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

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 commented Sep 9, 2019 •

edited

Loading

codecov-io commented Sep 9, 2019 •

edited

Loading

jsjtzyy commented Sep 16, 2019 •

edited

Loading

zzmao Sep 26, 2019 •

edited

Loading