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

Modify variable names and Optimize methods #871

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Modify variable names and Optimize methods #871

merged 1 commit into from
Jun 27, 2019

Conversation

yidadi
Copy link
Contributor

@yidadi yidadi commented Jun 27, 2019

Modify variable names and Optimize methods

 - Optimize methods
@CLAassistant
Copy link

CLAassistant commented Jun 27, 2019

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

Codecov Report

Merging #871 into master will increase coverage by 0.03%.
The diff coverage is 54.54%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #871      +/-   ##
============================================
+ Coverage     42.16%   42.19%   +0.03%     
- Complexity     1416     1418       +2     
============================================
  Files           307      307              
  Lines          8897     8894       -3     
  Branches       1204     1203       -1     
============================================
+ Hits           3751     3753       +2     
+ Misses         4685     4682       -3     
+ Partials        461      459       -2
Impacted Files Coverage Δ Complexity Δ
...baba/csp/sentinel/slotchain/SlotChainProvider.java 63.15% <54.54%> (-0.48%) 5 <3> (ø)
...inel/datasource/zookeeper/ZookeeperDataSource.java 67.32% <0%> (+0.99%) 16% <0%> (+1%) ⬆️
...a/csp/sentinel/slots/statistic/base/LeapArray.java 70.29% <0%> (+2.97%) 34% <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 ba14676...f269376. Read the comment docs.

@sczyh30
Copy link
Member

sczyh30 commented Jun 27, 2019

Hi, could you please illustrate why these changes are necessary?

@sczyh30 sczyh30 added the to-review To review label Jun 27, 2019
@yidadi
Copy link
Contributor Author

yidadi commented Jun 27, 2019

This method #resolveSlotChainBuilder just get the first SlotChainBuilder from the list.

@sczyh30
Copy link
Member

sczyh30 commented Jun 27, 2019

Maybe your change is not compatible with the current design:

  • If there is no user-defined SlotChainBuilder, then use the default slot chain builder
  • If user-defined SlotChainBuilder exists, the provider will pick the first loaded customized slot chain builder, then cache the builder.

See #145 for details.

@yidadi
Copy link
Contributor Author

yidadi commented Jun 27, 2019

Why? Is it necessary to put an object in a list and get only the first one?

@sczyh30
Copy link
Member

sczyh30 commented Jun 27, 2019

The list was initially designed for different strategies (e.g. pick the first customized, pick the final customized or ordered by @SpiOrder) but is not actually used indeed... I think it's okay to remove the list, but support for @SpiOrder should also be considered (we could leverage the SpiLoader class). For further improvement of the slot SPI, we could refer to #316

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit c14e329 into alibaba:master Jun 27, 2019
@sczyh30 sczyh30 removed the to-review To review label Jun 27, 2019
@sczyh30
Copy link
Member

sczyh30 commented Jun 27, 2019

Thanks!

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