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

Migrage the console server's specifying logic into transport-common #926

Merged
merged 1 commit into from Feb 24, 2020

Conversation

jasonjoo2010
Copy link
Collaborator

Describe what this PR does / why we need it

Because there are similar implementation in both simple-http and netty-http submodules on processing console server's address so migrate it into transport-common.

Does this pull request fix one issue?

NO

Special notes for reviews

Should actually run for verifying.

@codecov-io
Copy link

codecov-io commented Jul 17, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #926      +/-   ##
============================================
- Coverage     43.03%   42.58%   -0.46%     
+ Complexity     1568     1444     -124     
============================================
  Files           337      310      -27     
  Lines          9877     8993     -884     
  Branches       1332     1222     -110     
============================================
- Hits           4251     3830     -421     
+ Misses         5097     4696     -401     
+ Partials        529      467      -62
Impacted Files Coverage Δ Complexity Δ
...sp/sentinel/slots/system/SystemBlockException.java 0% <0%> (-44.45%) 0% <0%> (-2%)
...a/csp/sentinel/slots/system/SystemRuleManager.java 43.16% <0%> (-21.84%) 7% <0%> (-12%)
.../alibaba/csp/sentinel/slots/system/SystemRule.java 28.57% <0%> (-16.33%) 8% <0%> (-4%)
...libaba/csp/sentinel/slotchain/ResourceWrapper.java 71.42% <0%> (-15.24%) 3% <0%> (-3%)
...m/alibaba/csp/sentinel/node/metric/MetricNode.java 51.94% <0%> (-13.32%) 18% <0%> (-5%)
...aba/csp/sentinel/adapter/servlet/CommonFilter.java 75% <0%> (-12.81%) 10% <0%> (-1%)
...inel/adapter/gateway/sc/SentinelGatewayFilter.java 26.92% <0%> (-8.57%) 3% <0%> (-1%)
.../java/com/alibaba/csp/sentinel/util/SpiLoader.java 8.64% <0%> (-7.69%) 2% <0%> (-2%)
...nel/adapter/reactor/SentinelReactorSubscriber.java 80% <0%> (-6.89%) 19% <0%> (-2%)
...baba/csp/sentinel/slotchain/SlotChainProvider.java 63.15% <0%> (-6.85%) 5% <0%> (+2%)
... and 66 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 20ad3c8...eb9b861. Read the comment docs.

@sczyh30 sczyh30 added to-review To review kind/enhancement Category issues or prs related to enhancement. labels Jul 17, 2019
try {
port = Integer.parseInt(ipPortStr.substring(index + 1));
if (port <= 1 || port >= 65535) {
throw new RuntimeException("Port number [" + port + "] over range");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here we could just ignore the invalid item, rather than fail directly. 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.

The dealing in old logics,
it will be just ignored in simple-http while in netty-http an exception is generated (in InetSocketAddress.checkPort()) but also caught immediately in place, log a warning in record.log, print a stack trace to standard output.

Because the logging of sentinel can not be merged into the biz project's logging so the warning may be ignored. So the malformed :port is more like to be eaten and initializing flow will go on without the correct dashboard target.
That's why some guys suffer from no heartbeat / no application registered.

I like the idea of stopping to fix rather than ignore but logging.
But I will also feel ok on keeping the logic the same as how it acted in netty-http.

Copy link
Member

Choose a reason for hiding this comment

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

It's okay for me. I'll review the PR these days.

@sczyh30
Copy link
Member

sczyh30 commented Jan 22, 2020

Could you please resolve the conflicts?

@jasonjoo2010
Copy link
Collaborator Author

Could you please resolve the conflicts?

They have been resolved.

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 cd1d9be into alibaba:master Feb 24, 2020
@sczyh30
Copy link
Member

sczyh30 commented Feb 24, 2020

Thanks!

@sczyh30 sczyh30 removed the to-review To review label Feb 24, 2020
@sczyh30 sczyh30 added this to the 1.7.2 milestone Feb 24, 2020
cdfive pushed a commit to cdfive/Sentinel that referenced this pull request Feb 25, 2020
hughpearse pushed a commit to hughpearse/Sentinel that referenced this pull request Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants