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 unknown command issue in sentinel-transport-netty-http module #817

Merged
merged 2 commits into from
Jun 12, 2019

Conversation

cdfive
Copy link
Collaborator

@cdfive cdfive commented Jun 7, 2019

Describe what this PR does / why we need it

If a command whose mapping path and command name has more than one "/",
the CommandHandler can't be found in sentinel-transport-netty-http,
such as:
/cluster/server/flowRules
FetchClusterFlowRulesCommandHandler
will case:
Unknown command "cluster"

Does this pull request fix one issue?

Fixes #810

Describe how you did it

Modify the String parseTarget(String uri) method in HttpServerHandler class.
Don't use uri.split("/") to parse, but use indexOf("/") instead, to remove the
/ in uri, just like the parseRequest method of HttpEventTask class in sentinel-transport-simple-http module.

Describe how to verify it

Add 2 test cases in HttpServerHandlerTest class:
testFetchClusterFlowRulesCommandEmptyRule
testFetchClusterFlowRulesCommandSomeFlowRules
In order to test, sentinel-cluster-server-default dependency is added with test scope.

Run demo in sentinel-demo-cluster-server-alone module,
change the sentinel-transport-simple-http to sentinel-transport-netty-http,
start the ClusterServerDemo and visit http://localhost:8719/cluster/server/flowRules

Special notes for reviews

The previous test case which marked FIXME in HttpServerHandlerTest class has been fixed, I found a way that we can call embeddedChannel.readOutbound() looply to get the whole result.

Some comments in test class have been improved, such as upper case the first letter.

In pom.xml of parent module, sentinel-transport-netty-http is added in <dependencyManagement> , so that we needn't add version number when use it.

@codecov-io
Copy link

codecov-io commented Jun 7, 2019

Codecov Report

Merging #817 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #817      +/-   ##
============================================
+ Coverage     41.71%   41.77%   +0.05%     
- Complexity     1376     1378       +2     
============================================
  Files           304      304              
  Lines          8764     8764              
  Branches       1182     1182              
============================================
+ Hits           3656     3661       +5     
+ Misses         4662     4658       -4     
+ Partials        446      445       -1
Impacted Files Coverage Δ Complexity Δ
...a/csp/sentinel/slots/statistic/base/LeapArray.java 70.29% <0%> (+2.97%) 34% <0%> (+1%) ⬆️
...m/alibaba/csp/sentinel/log/DateFileLogHandler.java 57.57% <0%> (+3.03%) 7% <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 062385f...c1795ae. Read the comment docs.

@sczyh30 sczyh30 added the to-review To review label Jun 8, 2019
…ler for test multiple slash command name via SPI
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 e810ab8 into alibaba:master Jun 12, 2019
@sczyh30
Copy link
Member

sczyh30 commented Jun 12, 2019

Nice work, 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.

cannot access cluster flow rule information from http:ip:port/cluster/server/flowRules
3 participants