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

Add resource control enable/disable #1179

Merged
merged 2 commits into from
Jun 5, 2019
Merged

Conversation

zzmao
Copy link
Contributor

@zzmao zzmao commented Jun 5, 2019

Fix some issue in #1176
Add controlResource and maintainCluster support.

@codecov-io
Copy link

codecov-io commented Jun 5, 2019

Codecov Report

Merging #1179 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master   #1179     +/-   ##
==========================================
- Coverage     69.69%   69.6%   -0.1%     
+ Complexity     5397    5396      -1     
==========================================
  Files           430     430             
  Lines         33015   33047     +32     
  Branches       4173    4176      +3     
==========================================
- Hits          23010   23001      -9     
- Misses         8869    8906     +37     
- Partials       1136    1140      +4
Impacted Files Coverage Δ Complexity Δ
....github.ambry/clustermap/HelixVcrPopulateTool.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../main/java/com.github.ambry.router/EncryptJob.java 92.1% <0%> (-5.27%) 4% <0%> (-1%)
...java/com.github.ambry.store/CompactionManager.java 87.33% <0%> (-3.34%) 19% <0%> (ø)
...in/java/com.github.ambry.store/BlobStoreStats.java 71.13% <0%> (-0.62%) 103% <0%> (-1%)
...va/com.github.ambry.replication/ReplicaThread.java 74.15% <0%> (-0.38%) 64% <0%> (-1%)
...java/com.github.ambry.network/SSLTransmission.java 69.74% <0%> (+0.31%) 69% <0%> (+1%) ⬆️
.../java/com.github.ambry.router/DeleteOperation.java 93.61% <0%> (+1.41%) 49% <0%> (+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 af1b0d3...12b8316. Read the comment docs.

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.

Few comments/questions, otherwise looks good.

* @param resourceName the resource to enable/disable
* @param enable enable the resource if true
*/
static void controlResource(String destZkString, String destClusterName, String resourceName, boolean enable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible to add tests for the new methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not easy, since helix library doesn't expose getMethod for these status.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. But it's still good to test the different options to get code coverage.

@lightningrob lightningrob self-requested a review June 5, 2019 17:56
@jsjtzyy jsjtzyy merged commit 85fb467 into linkedin:master Jun 5, 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