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: https://github.com/ctripcorp/apollo/issues/2111 #2226

Closed
wants to merge 4 commits into from

Conversation

bkblack
Copy link
Contributor

@bkblack bkblack commented May 13, 2019

fix the issue : #2111

@codecov-io
Copy link

codecov-io commented May 13, 2019

Codecov Report

Merging #2226 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2226      +/-   ##
============================================
- Coverage        50%   49.94%   -0.06%     
+ Complexity     1982     1979       -3     
============================================
  Files           402      402              
  Lines         12400    12407       +7     
  Branches       1273     1273              
============================================
- Hits           6200     6197       -3     
- Misses         5758     5766       +8     
- Partials        442      444       +2
Impacted Files Coverage Δ Complexity Δ
...igservice/controller/NotificationControllerV2.java 86.84% <100%> (+0.63%) 31 <3> (ø) ⬇️
...work/apollo/biz/message/DatabaseMessageSender.java 56.25% <0%> (-10.42%) 6% <0%> (-2%)
...mework/apollo/portal/component/PortalSettings.java 65.07% <0%> (-4.77%) 5% <0%> (ø)
.../apollo/internals/RemoteConfigLongPollService.java 79.01% <0%> (-1.24%) 27% <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 989f37b...bb50939. Read the comment docs.

@bkblack bkblack closed this May 13, 2019
@bkblack bkblack reopened this May 13, 2019
@nobodyiam
Copy link
Member

Thanks, will take a detailed look soon!

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

Thanks! The logic looks good to me, however, there are many irrelevant changes, would you please import the apollo code style and update this commit?

@bkblack
Copy link
Contributor Author

bkblack commented May 18, 2019 via email

//1、set deferredResult before check, for avoid more waiting
//If check before set deferredResult and release after check and release before set deferredResult,
//it will wait for more time.
{
Copy link
Member

@nobodyiam nobodyiam May 18, 2019

Choose a reason for hiding this comment

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

This code block is not necessary, it's also better to remove it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 53.526% when pulling bb50939 on bkblack:master into 989f37b on ctripcorp:master.

@bkblack
Copy link
Contributor Author

bkblack commented May 20, 2019

 Sorry! The git commit history has been contaminated. I will resubmit pr.

@bkblack bkblack closed this May 20, 2019
@bkblack
Copy link
Contributor Author

bkblack commented May 20, 2019

new pr: #2255

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.

None yet

4 participants