Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Offline-To-Bootstrap state transition in StorageManager #1326

Merged
merged 4 commits into from
Dec 5, 2019

Conversation

jsjtzyy
Copy link
Contributor

@jsjtzyy jsjtzyy commented Nov 30, 2019

  1. implemented Offline-To-Bootstrap logic in storage manager
  2. introduced State Transition Exception and Transition Error Code
  3. introduced bootstrap-in-progress file to determine if a store is new added and still in bootstrap process
  4. added a config to switch on/off state change listeners in partition state model

@jsjtzyy jsjtzyy self-assigned this Nov 30, 2019
@codecov-io
Copy link

codecov-io commented Nov 30, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1326   +/-   ##
=========================================
  Coverage          ?   72.19%           
  Complexity        ?     6496           
=========================================
  Files             ?      471           
  Lines             ?    37220           
  Branches          ?     4696           
=========================================
  Hits              ?    26871           
  Misses            ?     9108           
  Partials          ?     1241
Impacted Files Coverage Δ Complexity Δ
...main/java/com.github.ambry.server/AmbryServer.java 78.87% <ø> (ø) 10 <0> (?)
...b.ambry.clustermap/StaticClusterAgentsFactory.java 40% <0%> (ø) 4 <0> (?)
...rc/main/java/com.github.ambry.store/BlobStore.java 89.77% <100%> (ø) 100 <1> (?)
...java/com.github.ambry/config/ClusterMapConfig.java 100% <100%> (ø) 3 <0> (?)
...hub.ambry/clustermap/StateTransitionException.java 100% <100%> (ø) 2 <2> (?)
...ithub.ambry/clustermap/StateModelListenerType.java 100% <100%> (ø) 1 <1> (?)
...ry.replication/CloudToStoreReplicationManager.java 57.03% <20%> (ø) 10 <0> (?)
...hub.ambry.clustermap/AmbryPartitionStateModel.java 21.95% <25%> (ø) 2 <1> (?)
.../com.github.ambry.clustermap/HelixParticipant.java 72.09% <39.13%> (ø) 25 <4> (?)
...ithub.ambry.clustermap/AmbryStateModelFactory.java 81.81% <60%> (ø) 3 <3> (?)
... and 2 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 b9fa880...b099ee5. Read the comment docs.

@jsjtzyy jsjtzyy force-pushed the startup-state-transition branch from 07de3a1 to 0a605eb Compare November 30, 2019 22:31
This changes integrates replica state transition logic with server
regular startup. For new added replica, state transition should perform
some bootstrap work. For existing replicas, their regular startup won't
be affected.
@jsjtzyy jsjtzyy force-pushed the startup-state-transition branch from 0a605eb to 12f625a Compare December 1, 2019 00:50
@jsjtzyy jsjtzyy changed the title WIP: Integrate state transition logic with server startup WIP: Implement Offline-To-Bootstrap state transition in StorageManager Dec 1, 2019
@jsjtzyy jsjtzyy marked this pull request as ready for review December 1, 2019 19:07
@jsjtzyy jsjtzyy changed the title WIP: Implement Offline-To-Bootstrap state transition in StorageManager Implement Offline-To-Bootstrap state transition in StorageManager Dec 1, 2019
import org.apache.helix.store.zk.ZkHelixPropertyStore;


public class MockHelixManagerFactory extends HelixFactory {
Copy link
Contributor Author

@jsjtzyy jsjtzyy Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved MockHelixManagerFactory out of HelixParticipantTest.java. No specific change in this file. (This single file contributes 426 lines and changes in rest files should be less than 600 lines, which is not hard to review)

Copy link
Contributor

@lightningrob lightningrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial comments added.

Copy link
Collaborator

@ankagrawal ankagrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of minor comments. Looks good to me otherwise.

@@ -43,25 +43,26 @@
/**
* An implementation of {@link ClusterParticipant} that registers as a participant to a Helix cluster.
*/
class HelixParticipant implements ClusterParticipant, PartitionStateChangeListener {
private final Logger logger = LoggerFactory.getLogger(getClass());
public class HelixParticipant implements ClusterParticipant, PartitionStateChangeListener {
private final String clusterName;
private final String zkConnectStr;
private final HelixFactory helixFactory;
Copy link
Collaborator

@ankagrawal ankagrawal Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR: since we are already in this file, looks like helixfactory is not being used. We can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, removed.

public void onPartitionStateChangeToStandbyFromLeader(String partitionName) {
for (PartitionStateChangeListener partitionStateChangeListener : partitionStateChangeListeners) {
partitionStateChangeListener.onPartitionStateChangeToStandbyFromLeader(partitionName);
public void onPartitionBecomeStandbyFromLeader(String partitionName) {
Copy link
Collaborator

@ankagrawal ankagrawal Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR: 

maybe we can replace this by
if(partitionstateChangeListeners.containsKey(StateModelListenerType.CloudToStoreReplicationManagerListener) {
partitionStateChangeListeners.get(StateModelListenerType.CloudToStoreReplicationManagerListener).onPartitionBecomeStandbyFromLeader(partitionName)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate your suggestion but I feel like it loses a little bit of readability. What do you think?

Copy link
Collaborator

@ankagrawal ankagrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@lightningrob lightningrob merged commit 4bf702f into linkedin:master Dec 5, 2019
zzmao pushed a commit that referenced this pull request Dec 6, 2019
…rs (#1327)

introduced ClusterParticipant into ReplicationManager and StatsManager to register state change listeners
implemented Offline-To-Bootstrap logic in Replication/Stats Manager.
(Note: this change is remaining part of PR #1326)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants