Introduce current state in BlobStore to help validate requests #1315

jsjtzyy · 2019-11-20T17:51:52Z

This PR introduces current state in BlobStore that is updated by state
transition (Helix controller initiates the transition). Store's current
state is used to validate requests and reject some of them if frontends
have old view of the store. For example, store has transitted to
INACTIVE state but due to Helix notification delay, some frontends may
still believe store is in STANDBY. To avoid such inconsistent view, we
use current state (which is always up-to-date) to validate coming
requests.

This PR introduces current state in BlobStore that is updated by state transition (Helix controller initiates the transition). Store's current state is used to validate requests and reject some of them if frontends have old view of the store. For example, store has transitted to INACTIVE state but due to Helix notification delay, some frontends may still believe store is in STANDBY. To avoid such inconsistent view, we use current state (which is always up-to-date) to validate coming requests.

codecov-io · 2019-11-20T18:23:33Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2601818). Click here to learn what that means.
The diff coverage is 86.66%.

@@            Coverage Diff            @@
##             master    #1315   +/-   ##
=========================================
  Coverage          ?   72.06%           
  Complexity        ?     6439           
=========================================
  Files             ?      468           
  Lines             ?    36961           
  Branches          ?     4650           
=========================================
  Hits              ?    26635           
  Misses            ?     9088           
  Partials          ?     1238

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com.github.ambry.server/AmbryServer.java	`79.02% <ø> (ø)`	`10 <0> (?)`
...hub.ambry.clustermap/AmbryPartitionStateModel.java	`22.22% <ø> (ø)`	`2 <0> (?)`
...in/java/com.github.ambry.cloud/CloudBlobStore.java	`82.5% <0%> (ø)`	`69 <0> (?)`
...ain/java/com.github.ambry/config/ServerConfig.java	`100% <100%> (ø)`	`1 <0> (?)`
...a/com.github.ambry.server/AmbryServerRequests.java	`92.98% <100%> (ø)`	`82 <1> (?)`
...rc/main/java/com.github.ambry.store/BlobStore.java	`89.95% <60%> (ø)`	`99 <1> (?)`

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 2601818...90a1e1b. Read the comment docs.

cgtz · 2019-11-20T21:30:48Z

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

@@ -76,6 +77,7 @@
  private BlobStoreStats blobStoreStats;
  private boolean started;
  private FileLock fileLock;
+  private ReplicaState currentState;


this should probably be volatile since it will be set from a different thread than where it is read

fair point, will fix

cgtz · 2019-11-20T21:33:33Z

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

+      Store store = storeManager.getStore(id);
+      if (requestType == RequestOrResponseType.PutRequest && !POST_ALLOWED_STORE_STATES.contains(
+          store.getCurrentState())) {
+        logger.error("{} is not allowed because current state of store {} is {}", requestType, id,


Is error the correct log level for this? How often will this occur during normal operation?

Should be infrequent. It happens in two cases:

remove replica: when local replica's state transition has completed but frontends still have old view and still issue PUT requests to the store. Once Helix state change notification arrives at frontend, this should die out quickly.

shutdown server: when server is shut down, local replica is directly set to OFFLINE and some frontends may still attempt to write to this store due to Helix notification delay.

cgtz · 2019-11-20T21:39:01Z

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

@@ -171,6 +173,7 @@ private BlobStore(ReplicaId replicaId, String storeId, StoreConfig config, Sched
    this.thresholdBytesLow = (long) (capacityInBytes * ((threshold - delta) / 100.0));
    ttlUpdateBufferTimeMs = TimeUnit.SECONDS.toMillis(config.storeTtlUpdateBufferTimeSeconds);
    errorCount = new AtomicInteger(0);
+    currentState = ReplicaState.OFFLINE;


I don't see any callers to setCurrentState() in the production code. Will this be added in a future PR?

Right, this will be called in storage manager when state transition occurs. I was planning to make the changes in this PR but then decided to split it into a separate PR.

justinlin-linkedin · 2019-11-22T00:05:23Z

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

  private final StatsManager statsManager;
  private final ConcurrentHashMap<RequestOrResponseType, Set<PartitionId>> requestsDisableInfo =
      new ConcurrentHashMap<>();
+  private final ConcurrentHashMap<PartitionId, ReplicaId> localPartitionToReplicaMap;
+  // POST requests are allowed on stores states: { LEADER, STANDBY }
+  static final Set<ReplicaState> POST_ALLOWED_STORE_STATES = EnumSet.of(ReplicaState.LEADER, ReplicaState.STANDBY);


Shouldn't it be PUT instead of Post?

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

justinlin-linkedin · 2019-11-22T00:09:58Z

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

+  private final ConcurrentHashMap<PartitionId, ReplicaId> localPartitionToReplicaMap;
+  // POST requests are allowed on stores states: { LEADER, STANDBY }
+  static final Set<ReplicaState> POST_ALLOWED_STORE_STATES = EnumSet.of(ReplicaState.LEADER, ReplicaState.STANDBY);
+  // UPDATE requests (including DELETE, TTLUpdate) are allowed on stores states: { LEADER, STANDBY, INACTIVE, BOOTSTRAP }
+  static final Set<ReplicaState> UPDATE_ALLOWED_STORE_STATES =
+      EnumSet.of(ReplicaState.LEADER, ReplicaState.STANDBY, ReplicaState.INACTIVE, ReplicaState.BOOTSTRAP);
+  static final Set<RequestOrResponseType> UPDATE_REQUEST_TYPES =
+      EnumSet.of(RequestOrResponseType.DeleteRequest, RequestOrResponseType.TtlUpdateRequest);


These can just be a Map from RequestOrResponseType to EnumSet of ReplicaState. So in isRequestEnabled method, you don't have to do if-else statement.

This is actually a good idea but we only have two categories of request type to account for, I feel like we don't have to initialize a map and query it. The logic is already simple in isRequestEnabled method.

justinlin-linkedin · 2019-11-22T00:16:24Z

ambry-server/src/test/java/com.github.ambry.server/AmbryServerRequestsTest.java

+    Map<ReplicaState, ReplicaId> stateToReplica = new HashMap<>();
+    int cnt = 0;
+    for (ReplicaState state : EnumSet.complementOf(EnumSet.of(ReplicaState.ERROR))) {
+      stateToReplica.put(state, localReplicas.get(cnt++));


nit: I would discourage using ++ in the same place where you use cnt. This would cause confusion. Can you do

stateToReplica.put(state, localReplicas.get(cnt)); cnt++;

justinlin-linkedin · 2019-11-22T00:17:52Z

ambry-server/src/test/java/com.github.ambry.server/AmbryServerRequestsTest.java

+          System.out.println("replica state = " + entry.getKey() + ", store state = " + storageManager.getStore(
+              entry.getValue().getPartitionId()).getCurrentState());


Remove System.out.println?

jsjtzyy self-assigned this Nov 20, 2019

java docs

fe40970

jsjtzyy marked this pull request as ready for review November 20, 2019 18:26

more unit test cases

e4770be

jsjtzyy requested review from justinlin-linkedin and cgtz November 20, 2019 21:21

cgtz suggested changes Nov 20, 2019

View reviewed changes

justinlin-linkedin reviewed Nov 22, 2019

View reviewed changes

comments

90a1e1b

justinlin-linkedin approved these changes Nov 22, 2019

View reviewed changes

cgtz approved these changes Nov 25, 2019

View reviewed changes

cgtz merged commit 55acc01 into linkedin:master Nov 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce current state in BlobStore to help validate requests #1315

Introduce current state in BlobStore to help validate requests #1315

jsjtzyy commented Nov 20, 2019

codecov-io commented Nov 20, 2019 •

edited

Loading

cgtz Nov 20, 2019

jsjtzyy Nov 20, 2019

cgtz Nov 20, 2019

jsjtzyy Nov 20, 2019

cgtz Nov 20, 2019

jsjtzyy Nov 20, 2019

justinlin-linkedin Nov 22, 2019

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 22, 2019

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 22, 2019

jsjtzyy Nov 22, 2019

justinlin-linkedin Nov 22, 2019

jsjtzyy Nov 22, 2019

		System.out.println("replica state = " + entry.getKey() + ", store state = " + storageManager.getStore(
		entry.getValue().getPartitionId()).getCurrentState());

Introduce current state in BlobStore to help validate requests #1315

Introduce current state in BlobStore to help validate requests #1315

Conversation

jsjtzyy commented Nov 20, 2019

codecov-io commented Nov 20, 2019 • edited Loading

Codecov Report

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

codecov-io commented Nov 20, 2019 •

edited

Loading