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

Fix internal bug when amount of context exceeds the threshold #152

Merged
merged 3 commits into from
Sep 19, 2018

Conversation

sczyh30
Copy link
Member

@sczyh30 sczyh30 commented Sep 19, 2018

Describe what this PR does / why we need it

Fix internal bug when amount of context exceeds the threshold.

Does this pull request fix one issue?

Fixes #151

Describe how you did it

  • When amount of context exceeds the threshold, the NullContext will be set to current thread local. Thus, when checking context in CtSph#entry, once NullContext detected, the entry will not do rule checking on the slot chain.
  • Cache the default context during initialization. Then when amount of context exceeds the threshold, entries under default context can do rule checking under default context.
  • Enhance the error message.

Describe how to verify it

See test cases.

- NullContext will take effect
- Default context will be cached during initialization

Signed-off-by: Eric Zhao <sczyh16@gmail.com>
@sczyh30 sczyh30 added the to-review To review label Sep 19, 2018
@sczyh30 sczyh30 self-assigned this Sep 19, 2018
@codecov-io
Copy link

Codecov Report

Merging #152 into master will decrease coverage by 0.04%.
The diff coverage is 46.15%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #152      +/-   ##
============================================
- Coverage     49.01%   48.97%   -0.05%     
  Complexity      661      661              
============================================
  Files           122      122              
  Lines          4172     4180       +8     
  Branches        573      574       +1     
============================================
+ Hits           2045     2047       +2     
- Misses         1879     1883       +4     
- Partials        248      250       +2
Impacted Files Coverage Δ Complexity Δ
.../main/java/com/alibaba/csp/sentinel/Constants.java 80% <ø> (ø) 1 <0> (ø) ⬇️
.../src/main/java/com/alibaba/csp/sentinel/CtSph.java 50.61% <ø> (ø) 10 <0> (ø) ⬇️
...java/com/alibaba/csp/sentinel/context/Context.java 67.64% <0%> (-2.05%) 15 <0> (ø)
.../com/alibaba/csp/sentinel/context/NullContext.java 100% <100%> (ø) 1 <1> (ø) ⬇️
.../com/alibaba/csp/sentinel/context/ContextUtil.java 76.47% <33.33%> (-2.26%) 11 <0> (ø)
...rc/main/java/com/alibaba/csp/sentinel/CtEntry.java 89.47% <60%> (-4.82%) 13 <0> (ø)
...a/csp/sentinel/slots/statistic/base/LeapArray.java 72.41% <0%> (-1.73%) 18% <0%> (ø)

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 91f2797...864c003. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #152 into master will decrease coverage by 0.04%.
The diff coverage is 46.15%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #152      +/-   ##
============================================
- Coverage     49.01%   48.97%   -0.05%     
  Complexity      661      661              
============================================
  Files           122      122              
  Lines          4172     4180       +8     
  Branches        573      574       +1     
============================================
+ Hits           2045     2047       +2     
- Misses         1879     1883       +4     
- Partials        248      250       +2
Impacted Files Coverage Δ Complexity Δ
.../main/java/com/alibaba/csp/sentinel/Constants.java 80% <ø> (ø) 1 <0> (ø) ⬇️
.../src/main/java/com/alibaba/csp/sentinel/CtSph.java 50.61% <ø> (ø) 10 <0> (ø) ⬇️
...java/com/alibaba/csp/sentinel/context/Context.java 67.64% <0%> (-2.05%) 15 <0> (ø)
.../com/alibaba/csp/sentinel/context/NullContext.java 100% <100%> (ø) 1 <1> (ø) ⬇️
.../com/alibaba/csp/sentinel/context/ContextUtil.java 76.47% <33.33%> (-2.26%) 11 <0> (ø)
...rc/main/java/com/alibaba/csp/sentinel/CtEntry.java 89.47% <60%> (-4.82%) 13 <0> (ø)
...a/csp/sentinel/slots/statistic/base/LeapArray.java 72.41% <0%> (-1.73%) 18% <0%> (ø)

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 91f2797...864c003. Read the comment docs.

Signed-off-by: Eric Zhao <sczyh16@gmail.com>
@codecov-io
Copy link

codecov-io commented Sep 19, 2018

Codecov Report

Merging #152 into master will increase coverage by 1.04%.
The diff coverage is 65.21%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #152      +/-   ##
============================================
+ Coverage     49.01%   50.05%   +1.04%     
- Complexity      661      672      +11     
============================================
  Files           122      122              
  Lines          4172     4193      +21     
  Branches        573      575       +2     
============================================
+ Hits           2045     2099      +54     
+ Misses         1879     1833      -46     
- Partials        248      261      +13
Impacted Files Coverage Δ Complexity Δ
.../main/java/com/alibaba/csp/sentinel/Constants.java 80% <ø> (ø) 1 <0> (ø) ⬇️
.../src/main/java/com/alibaba/csp/sentinel/CtSph.java 50.61% <ø> (ø) 10 <0> (ø) ⬇️
...java/com/alibaba/csp/sentinel/context/Context.java 67.64% <0%> (-2.05%) 15 <0> (ø)
.../com/alibaba/csp/sentinel/context/NullContext.java 100% <100%> (ø) 1 <1> (ø) ⬇️
...rc/main/java/com/alibaba/csp/sentinel/CtEntry.java 83.33% <37.5%> (-10.96%) 13 <2> (ø)
.../com/alibaba/csp/sentinel/context/ContextUtil.java 83.33% <84.61%> (+4.6%) 14 <2> (+3) ⬆️
...ava/com/alibaba/csp/sentinel/node/DefaultNode.java 60.34% <0%> (+3.44%) 12% <0%> (+1%) ⬆️
...a/csp/sentinel/slots/statistic/base/LongAdder.java 34.04% <0%> (+17.02%) 11% <0%> (+7%) ⬆️
...ba/csp/sentinel/slots/statistic/base/Stripe64.java 56.25% <0%> (+30.2%) 0% <0%> (ø) ⬇️

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 91f2797...beae856. Read the comment docs.

}

private static void initDefaultContext() {
contextNameNodeMap.put(Constants.CONTEXT_DEFAULT_NAME, new EntranceNode(
Copy link
Contributor

Choose a reason for hiding this comment

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

Default Context should be child of ROOT, use Constants.ROOT.addChild(node) to add.

@@ -75,8 +79,9 @@ protected void exitForContext(Context context, int count, Object... args) throws
if (parent != null) {
((CtEntry)parent).child = null;
}
if (parent == null) {
// Auto-created entry indicates immediate exit.
if (parent == null && !(context instanceof NullContext)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check NullContext at the beginning of this method, if current context is NullContext, this method should return at once.

Copy link
Contributor

@CarpenterLee CarpenterLee 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 removed the to-review To review label Sep 19, 2018
@sczyh30 sczyh30 merged commit be43a31 into master Sep 19, 2018
@sczyh30 sczyh30 deleted the fix/exceeded-context branch September 19, 2018 06:42
Arlmls pushed a commit to Arlmls/Sentinel that referenced this pull request Jan 8, 2019
…a#152)

- When amount of context exceeds the threshold, the `NullContext` will be set to current thread local. Thus, when checking context in `CtSph#entry`, once `NullContext` detected, the entry will not do rule checking on the slot chain.
- Cache the default context during initialization. Then when amount of context exceeds the threshold, entries under default context can do rule checking under default context.
- Enhance the error message

Signed-off-by: Eric Zhao <sczyh16@gmail.com>
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.

[Bug] Error occurs internal when context count exceeds MAX_CONTEXT_NAME_SIZE
3 participants