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

[ISSUE #6873] Not update controller address list when the resolved result is empty #6874

Closed
wants to merge 10 commits into from

Conversation

weihubeats
Copy link
Member

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.90%. Comparing base (1b42515) to head (9eb378e).

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #6874   +/-   ##
==========================================
  Coverage      42.90%   42.90%           
+ Complexity     10370    10368    -2     
==========================================
  Files           1270     1270           
  Lines          88721    88721           
  Branches       11405    11405           
==========================================
+ Hits           38062    38070    +8     
+ Misses         45954    45946    -8     
  Partials        4705     4705           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@TheR1sing3un TheR1sing3un left a comment

Choose a reason for hiding this comment

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

LGTM~

@xdkxlk
Copy link
Contributor

xdkxlk commented Jun 9, 2023

I think we should differentiate between whether it throws an exception or actually returns null. And dnsLookupAddressByDomain is also used by updateNameServerAddressListByDnsLookup in BrokerOuterAPI .

@weihubeats
Copy link
Member Author

I think even if the actual is null, it should not be updated to null, because the actual is null is also abnormal.
Exceptions or actual null can be distinguished by logging

@RongtongJin RongtongJin changed the title Adding null does not update [ISSUE #6873] Adding null does not update Jun 9, 2023
Copy link
Contributor

@RongtongJin RongtongJin left a comment

Choose a reason for hiding this comment

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

If the parsing fails, it should be printed in the log. What are the benefits of setting it to null here?

@weihubeats
Copy link
Member Author

weihubeats commented Jun 9, 2023

I don't understand what you mean, here is the parsing failure will return null, so if it is null then it is not updated. The advantage of this is that if the dns exception will not replace the normal address of the controller with null

@lizhimins lizhimins changed the title [ISSUE #6873] Adding null does not update [ISSUE #6873] Not update controller address list when the resolved result is empty Jun 9, 2023
@ferrirW
Copy link
Contributor

ferrirW commented Jun 14, 2023

If parsing fails, the api will return the empty list, not null

you refered case can be provide more information for repetition ?

@weihubeats weihubeats closed this May 22, 2024
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.

6 participants