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 error threshold and verify domain name for instance in helix tool #2525

Conversation

justinlin-linkedin
Copy link
Collaborator

This PR introduces two changes

  1. Add error or recover threshold to the cluster config
  2. Verify that all the instances have domain field before enabling topology awareness for cluster.

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2023

Codecov Report

Patch coverage: 28.00% and project coverage change: -39.41% ⚠️

Comparison is base (827d592) 72.88% compared to head (54343ab) 33.47%.

Additional details and impacted files
@@              Coverage Diff              @@
##             master    #2525       +/-   ##
=============================================
- Coverage     72.88%   33.47%   -39.41%     
+ Complexity    11237     4999     -6238     
=============================================
  Files           792      792               
  Lines         63701    63714       +13     
  Branches       7788     7790        +2     
=============================================
- Hits          46426    21331    -25095     
- Misses        14725    40705    +25980     
+ Partials       2550     1678      -872     
Files Changed Coverage Δ
...ub/ambry/clustermap/HelixBootstrapUpgradeUtil.java 69.69% <28.00%> (-0.92%) ⬇️

... and 534 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

new HelixConfigScope(HelixConfigScope.ConfigScopeProperty.PARTICIPANT, Collections.singletonList(clusterName),
null);
// This will list all the participant names
List<String> instanceNames = configAccessor.getKeys(scope);
Copy link
Contributor

@Arun-LinkedIn Arun-LinkedIn Jul 31, 2023

Choose a reason for hiding this comment

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

Hmm, when --resources input is given to this tool, I think I called setInstanceConfig only for the instances hosting partitions of those resources (line number 894). But, configAccessor.getKeys(scope) may get all instances in the cluster. May be we need to set DOMAIN field for all instances in the cluster (irrespective of inputted resources) now ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should probably do that, let me update it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@SophieGuo410 SophieGuo410 left a comment

Choose a reason for hiding this comment

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

lgtm.

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