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

Support HTTP POST request for update operations in SentinelApiClient of Sentinel dashboard #620

Merged
merged 4 commits into from
Apr 12, 2019

Conversation

jasonjoo2010
Copy link
Collaborator

@jasonjoo2010 jasonjoo2010 commented Mar 27, 2019

Describe what this PR does / why we need it

Feature for transport module: Make transport-netty-http/transport-simple-http support posting (split to #667)

For dashboard:

  • Restructure: refine SentinelApiClient to support posting and make it simple
  • Enhancement: SentinelVersion parsing logic
  • Bug fix: comparison bug of SentinelVersion

Does this pull request fix one issue?

Relevant issue #618

Describe how you did it

Make netty-http support both form-encoded / multipart post requests.
Make simple-http support form-encoded. Multipart requests can also be support too if necessary.

Form-encoded would be the standard support for transport modules.

These have been split to #667

Describe how to verify it

Submit big rules collection. For example, more than 30 entries which will overflow the URI limitation in RFC.

@sczyh30 sczyh30 added to-review To review size/XL Indicate a PR that changes 500-999 lines. labels Mar 27, 2019
@jasonjoo2010 jasonjoo2010 changed the title Relevant issue #618 Make transport-netty-http/transport-simple-http support HTTP Post Mar 27, 2019
@sczyh30 sczyh30 added this to the 1.6.0 milestone Mar 27, 2019
@codecov-io
Copy link

codecov-io commented Mar 27, 2019

Codecov Report

Merging #620 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #620      +/-   ##
============================================
- Coverage      41.1%   41.06%   -0.04%     
- Complexity     1216     1332     +116     
============================================
  Files           267      284      +17     
  Lines          7854     8925    +1071     
  Branches       1062     1193     +131     
============================================
+ Hits           3228     3665     +437     
- Misses         4233     4794     +561     
- Partials        393      466      +73
Impacted Files Coverage Δ Complexity Δ
...a/csp/sentinel/slots/statistic/base/LeapArray.java 67.64% <0%> (-2.95%) 33% <0%> (-1%)
...a/com/alibaba/csp/sentinel/node/StatisticNode.java 56.17% <0%> (-1.13%) 28% <0%> (-1%)
.../alibaba/csp/sentinel/eagleeye/FastDateFormat.java 78.37% <0%> (ø) 6% <0%> (?)
...com/alibaba/csp/sentinel/eagleeye/TokenBucket.java 35.29% <0%> (ø) 1% <0%> (?)
...a/csp/sentinel/slots/statistic/base/Striped64.java 60.41% <0%> (ø) 10% <0%> (?)
...libaba/csp/sentinel/eagleeye/EagleEyeAppender.java 14.28% <0%> (ø) 1% <0%> (?)
...sentinel/eagleeye/EagleEyeRollingFileAppender.java 28.9% <0%> (ø) 11% <0%> (?)
...a/com/alibaba/csp/sentinel/eagleeye/StatEntry.java 20.21% <0%> (ø) 8% <0%> (?)
...om/alibaba/csp/sentinel/eagleeye/SyncAppender.java 53.57% <0%> (ø) 5% <0%> (?)
...ibaba/csp/sentinel/eagleeye/StatLogController.java 65.93% <0%> (ø) 5% <0%> (?)
... and 9 more

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 b02ef82...2acc532. Read the comment docs.

@jasonjoo2010 jasonjoo2010 changed the title Make transport-netty-http/transport-simple-http support HTTP Post Make transport-netty-http/transport-simple-http support HTTP POST Mar 27, 2019
@sczyh30
Copy link
Member

sczyh30 commented Mar 28, 2019

Good job. I'll review these days :)

@sczyh30
Copy link
Member

sczyh30 commented Apr 3, 2019

I've reviewed the code about setRules in SentinelApiClient and summarized the following compatibility table:

transport server < 1.6.0 transport server >= 1.6.0
Dashboard < 1.6.0 GET GET
Dashboard >= 1.6.0 GET POST

So for setRules API, every version of transport server and dashboard is compatible. Is it correct? Just to confirm it :) @jasonjoo2010

@jasonjoo2010
Copy link
Collaborator Author

I've reviewed the code about setRules in SentinelApiClient and summarized the following compatibility table:

transport server < 1.6.0 transport server >= 1.6.0
Dashboard < 1.6.0 GET GET
Dashboard >= 1.6.0 GET POST
So for setRules API, every version of transport server and dashboard is compatible. Is it correct? Just to confirm it :) @jasonjoo2010

Not only for setRules but for All api with large data submitted like setRules

Yes.
Transport layer will support parameters in both GET and POST methods from 1.6.0.
While dashboard will only enable POST feature when the machine's transport >= 1.6.0.

@sczyh30
Copy link
Member

sczyh30 commented Apr 11, 2019

And could you please split your commit into two parts, one about changes for the transport module, another for the dashboard. Commits about dashboard should be prefixed with dashboard: .

@CLAassistant
Copy link

CLAassistant commented Apr 12, 2019

CLA assistant check
All committers have signed the CLA.

@sczyh30 sczyh30 changed the title Make transport-netty-http/transport-simple-http support HTTP POST Support HTTP POST request for update operations in SentinelApiClient of Sentinel dashboard Apr 12, 2019
@sczyh30 sczyh30 added the area/dashboard Issues or PRs about Sentinel Dashboard label Apr 12, 2019
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 359e659 into alibaba:master Apr 12, 2019
@sczyh30
Copy link
Member

sczyh30 commented Apr 12, 2019

Thanks for contributing!

CST11021 pushed a commit to CST11021/Sentinel that referenced this pull request Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dashboard Issues or PRs about Sentinel Dashboard size/XL Indicate a PR that changes 500-999 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants