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

Introducing Ambry partition state model and update Helix tool #1228

Merged
merged 5 commits into from
Aug 8, 2019

Conversation

jsjtzyy
Copy link
Contributor

@jsjtzyy jsjtzyy commented Aug 5, 2019

  1. Introduce new state model that incorporates BOOTSTRAP and INACTIVE
    states.
  2. Add clustermap config that allows participant to choose which state
    model to use. Previous LeadStandby Model is default option.
  3. Update Helix bootstrap tool to supporting specifying state model when
    creating new cluster or adding new state model to existing cluster.

@jsjtzyy jsjtzyy self-assigned this Aug 5, 2019
@jsjtzyy jsjtzyy requested a review from zzmao August 6, 2019 00:06
jsjtzyy added 3 commits August 6, 2019 11:49
1. Introduce new state model that incorporates BOOTSTRAP and INACTIVE
states.
2. Add clustermap config that allows participant to choose which state
model to use. Previous LeadStandby Model is default option.
3. Update Helix bootstrap tool to supporting specifying state model when
creating new cluster or adding new state model to existing cluster.
@codecov-io
Copy link

codecov-io commented Aug 6, 2019

Codecov Report

Merging #1228 into master will increase coverage by 14.1%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1228      +/-   ##
============================================
+ Coverage     69.51%   83.62%   +14.1%     
+ Complexity     5552       57    -5495     
============================================
  Files           432        6     -426     
  Lines         33943      348   -33595     
  Branches       4342       38    -4304     
============================================
- Hits          23596      291   -23305     
+ Misses         9146       45    -9101     
+ Partials       1201       12    -1189
Impacted Files Coverage Δ Complexity Δ
...ava/com.github.ambry/config/PerformanceConfig.java
...main/java/com.github.ambry.cloud/CloudReplica.java
...m.github.ambry.replication/ReplicationMetrics.java
...ava/com.github.ambry/frontend/SecurityService.java
...b.ambry.cloud/CloudBlobCryptoAgentFactoryImpl.java
...ain/java/com.github.ambry/frontend/Operations.java
...n/java/com.github.ambry.clustermap/Datacenter.java
...github.ambry.messageformat/TtlUpdateSubRecord.java
...in/java/com.github.ambry.cloud/CloudFindToken.java
...i/src/main/java/com.github.ambry/network/Port.java
... and 410 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 2f386ac...869f3cb. Read the comment docs.

Copy link
Contributor

@zzmao zzmao left a comment

Choose a reason for hiding this comment

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

PROD looks good. Will look at test soon.

Copy link

@jiajunwang jiajunwang left a comment

Choose a reason for hiding this comment

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

For the reset() method in the state model, probably you will need to implement in the future.
"no op" seems to be not right, just a reminder : )



@StateModelInfo(initialState = "OFFLINE", states = {"BOOTSTRAP", "LEADER", "STANDBY", "INACTIVE"})
public class AmbryPartitionStateModel extends StateModel {

Choose a reason for hiding this comment

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

I think you might need to override the following method:

@transition(to = "DROPPED", from = "ERROR")
public void onBecomeDroppedFromError(Message message, NotificationContext context)
throws Exception {
logger.info("Default ERROR->DROPPED transition invoked.");
}

Otherwise, the default clean up behavior won't do anything other than updating the state model.

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, will add that method. Thanks for reminding me of this.

case AmbryStateModelDefinition.AMBRY_LEADER_STANDBY_MODEL:
stateModelToReturn = new AmbryPartitionStateModel();
break;
default:

Choose a reason for hiding this comment

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

Better to explicitly check DEFAULT_STATE_MODEL_DEF here. For the other cases, shall throw an exception the earlier to better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will also add a test case for the change.

@zzmao zzmao merged commit 461f652 into linkedin:master Aug 8, 2019
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