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.

Sign up for GitHub

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

Jump to bottom

Return status 429 in the default block handler #1112

Merged
merged 3 commits into from
Nov 6, 2019
Merged

Return status 429 in the default block handler #1112

merged 3 commits into from
Nov 6, 2019

Conversation

zhaoyuguang
Copy link
Collaborator

resolve #1110

sczyh30
sczyh30 reviewed Oct 23, 2019
View reviewed changes
sentinel-core/src/main/java/com/alibaba/csp/sentinel/config/SentinelConfig.java Outdated
@@ -48,12 +48,14 @@
public static final String TOTAL_METRIC_FILE_COUNT = "csp.sentinel.metric.file.total.count";
public static final String COLD_FACTOR = "csp.sentinel.flow.cold.factor";
public static final String STATISTIC_MAX_RT = "csp.sentinel.statistic.max.rt";
public static final String BLOCK_PAGE_HTTP_STATUS = "csp.sentinel.web.servlet.block.page.http.status";
Copy link
Member

Choose a reason for hiding this comment

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

Usually, we don't put the property key related to adapters and extensions to the SentinelConfig in sentinel-core. We could put this to WebServletConfig instead (like the former block page item). What do you think of 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.

That looks clearer, I had fix it, review code again, thanks~

...-servlet/src/main/java/com/alibaba/csp/sentinel/adapter/servlet/config/WebServletConfig.java Outdated
* @return http status
*/
public static int getBlockPageHttpStatus() {
return Integer.parseInt(SentinelConfig.getConfig(BLOCK_PAGE_HTTP_STATUS));
Copy link
Member

Choose a reason for hiding this comment

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

We need to handle the parsing error or empty value here. If an error occurs or the item is absent, then we could use the default 429.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make sense, thanks for guiding.
When csp.sentinel.web.servlet.block.page.http.status is empty or not number, the block page http status will be automatically set to 429(Too Many Requests).
Review code again, please~

@sczyh30 sczyh30 added kind/enhancement Category issues or prs related to enhancement. to-review To review labels Oct 23, 2019
@codecov-io
Copy link

codecov-io commented Oct 24, 2019 •
edited
Loading

Codecov Report

Merging #1112 into master will decrease coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1112      +/-   ##
============================================
- Coverage     42.91%   42.88%   -0.03%     
- Complexity     1502     1503       +1     
============================================
  Files           320      320              
  Lines          9400     9411      +11     
  Branches       1283     1284       +1     
============================================
+ Hits           4034     4036       +2     
- Misses         4870     4878       +8     
- Partials        496      497       +1
Impacted Files Coverage Δ Complexity Δ
.../csp/sentinel/adapter/servlet/util/FilterUtil.java 64.47% <100%> (+0.47%) 15 <1> (ø) ⬇️
...tinel/adapter/servlet/config/WebServletConfig.java 71.42% <70%> (-3.58%) 5 <3> (+3)
...m/alibaba/csp/sentinel/log/DateFileLogHandler.java 54.54% <0%> (-3.04%) 6% <0%> (-1%)
...tinel/slots/block/flow/param/ParamFlowChecker.java 52.7% <0%> (-2.71%) 28% <0%> (-1%)
...inel/datasource/zookeeper/ZookeeperDataSource.java 66.33% <0%> (-1%) 15% <0%> (-1%)
...ava/com/alibaba/csp/sentinel/node/ClusterNode.java 100% <0%> (+4.76%) 8% <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 74a40aa...342ab06. Read the comment docs.

@zhaoyuguang
Copy link
Collaborator Author

@sczyh30 Please review again~

@sczyh30
Copy link
Member

sczyh30 commented Nov 2, 2019

Friendly ping :)

sczyh30
sczyh30 approved these changes Nov 6, 2019
View reviewed changes
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 a451d4b into alibaba:master Nov 6, 2019
@sczyh30 sczyh30 removed the to-review To review label Nov 6, 2019
@sczyh30 sczyh30 added this to the 1.7.0 milestone Nov 6, 2019
@sczyh30
Copy link
Member

sczyh30 commented Nov 6, 2019

Thanks for contributing!

@sczyh30 sczyh30 added the area/integrations Issues or PRs related to integrations with open-source components label Nov 6, 2019
hughpearse pushed a commit to hughpearse/Sentinel that referenced this pull request Jun 2, 2021
@zhaoyuguang @sczyh30
Support customized block page HTTP status in sentinel-web-servlet-ada…
56d8856 
…pter (alibaba#1112)

* Return canonical status 429 in the default block handler of sentinel-web-servlet-adapter.
* Add a `csp.sentinel.web.servlet.block.page.http.status` property to support customized block status configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sczyh30 sczyh30 sczyh30 approved these changes

Assignees
No one assigned
Labels
area/integrations Issues or PRs related to integrations with open-source components kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.

Return status 429 in the default block handler of sentinel-web-servlet-adapter
3 participants
@zhaoyuguang @codecov-io @sczyh30