Skip to content

Conversation

@jasonjoo2010
Copy link
Collaborator

Make transport layer support parameters in post body

Describe what this PR does / why we need it

include netty-http and simple-http

Does this pull request fix one issue?

#618

@CLAassistant
Copy link

CLAassistant commented Apr 12, 2019

CLA assistant check
All committers have signed the CLA.

@jasonjoo2010 jasonjoo2010 force-pushed the transport-support-post branch from 4209e6a to b30c273 Compare April 12, 2019 01:25
@codecov-io
Copy link

codecov-io commented Apr 12, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #667      +/-   ##
============================================
+ Coverage      41.1%   41.13%   +0.03%     
- Complexity     1216     1335     +119     
============================================
  Files           267      284      +17     
  Lines          7854     8925    +1071     
  Branches       1062     1193     +131     
============================================
+ Hits           3228     3671     +443     
- Misses         4233     4791     +558     
- Partials        393      463      +70
Impacted Files Coverage Δ Complexity Δ
...l/cluster/server/connection/ConnectionManager.java 74% <0%> (-2%) 10% <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 61.45% <0%> (ø) 11% <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 10 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...211bd06. Read the comment docs.

@jasonjoo2010
Copy link
Collaborator Author

Because another unstable unit test WarmUpRateLimiterControllerTest this PR also make it stable to pass travis

@sczyh30
Copy link
Member

sczyh30 commented Apr 12, 2019

Because another unstable unit test WarmUpRateLimiterControllerTest this PR also make it stable to pass travis

👍

@jasonjoo2010 jasonjoo2010 force-pushed the transport-support-post branch from f973e02 to 667baaa Compare April 12, 2019 01:39
@sczyh30 sczyh30 added the to-review To review label Apr 12, 2019
@sczyh30
Copy link
Member

sczyh30 commented Apr 12, 2019

Seems that ClusterNodeTest.testGetOrCreateOriginNodeMultiThread is also unstable. I think it might be caused by line 98:

Node node = clusterNode.getOrCreateOriginNode(origins.get(random.nextInt(origins.size())));

The line will get origin randomly from the origin array. In rare circumstances, not all origins can be visited. Could you please fix it?

@jasonjoo2010
Copy link
Collaborator Author

Seems that ClusterNodeTest.testGetOrCreateOriginNodeMultiThread is also unstable. I think it might be caused by line 98:

Node node = clusterNode.getOrCreateOriginNode(origins.get(random.nextInt(origins.size())));

The line will get origin randomly from the origin array. In rare circumstances, not all origins can be visited.

Yeah, i found i seems like suffering unstable tests now

@jasonjoo2010
Copy link
Collaborator Author

Seems that ClusterNodeTest.testGetOrCreateOriginNodeMultiThread is also unstable. I think it might be caused by line 98:

Node node = clusterNode.getOrCreateOriginNode(origins.get(random.nextInt(origins.size())));

The line will get origin randomly from the origin array. In rare circumstances, not all origins can be visited.

I think in rare condition this random algorithm may miss the assertion.

eg.

What is expected as:

origin count
origin1 7
origin2 7
origin3 6

But maybe happen:

origin count
origin1 10
origin2 10
origin3 0

And it will fail.

Should we consist on the random algorithm and introduce sth. like Set to indicate what has been actually hit or just make it a RoundRobin like algorithm?

@sczyh30
Copy link
Member

sczyh30 commented Apr 12, 2019

Seems that ClusterNodeTest.testGetOrCreateOriginNodeMultiThread is also unstable. I think it might be caused by line 98:

Node node = clusterNode.getOrCreateOriginNode(origins.get(random.nextInt(origins.size())));

The line will get origin randomly from the origin array. In rare circumstances, not all origins can be visited.

I think in rare condition this random algorithm may miss the assertion.

eg.

What is expected as:

origin count
origin1 7
origin2 7
origin3 6
But maybe happen:

origin count
origin1 10
origin2 10
origin3 0
And it will fail.

Should we consist on the random algorithm and introduce sth. like Set to indicate what has been actually hit or just make it a RoundRobin like algorithm?

Yes, maybe we can just make it a round robin (taskCount % originSize).

@jasonjoo2010
Copy link
Collaborator Author

Done with it.

And it's strange only this PR suffers unstable unit tests frequently.

@sczyh30
Copy link
Member

sczyh30 commented Apr 12, 2019

Good job. Unstable test cases often fail when merging into master (and we have to manually restart the CI job to retry)

@sczyh30
Copy link
Member

sczyh30 commented Apr 12, 2019

Seems that the CLA status is expired? Could you please check the CLA status of your GitHub account?

@jasonjoo2010 jasonjoo2010 force-pushed the transport-support-post branch from 2b5aa9d to 211bd06 Compare April 12, 2019 02:32
@jasonjoo2010
Copy link
Collaborator Author

Seems that the CLA status is expired? Could you please check the CLA status of your GitHub account?

It's just because default email is internal address and i forgot to override it when commit.

Now it's ok

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 73f166e into alibaba:master Apr 12, 2019
@sczyh30
Copy link
Member

sczyh30 commented Apr 12, 2019

Nice work. Thanks for contributing!

@sczyh30 sczyh30 removed the to-review To review label Apr 12, 2019
@sczyh30 sczyh30 added this to the 1.6.0 milestone Apr 12, 2019
@cdfive
Copy link
Collaborator

cdfive commented Apr 12, 2019

@jasonjoo2010 @sczyh30 Oh, I'm sorry for make ClusterNodeTest.testGetOrCreateOriginNodeMultiThread unstable, neglecting the random rare condition. Thanks to fix it!

@jasonjoo2010 jasonjoo2010 deleted the transport-support-post branch March 24, 2020 00:35
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.

5 participants