Stop using ResolverProxy for resolving node lookups #29264
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy and
joonhaengHeo
|
PR #29264: Size comparison from 3400d1d to 0011142 Increases (7 builds for k32w, nrfconnect, qpg)
Decreases (7 builds for cc32xx, k32w, nrfconnect, qpg)
Full report (10 builds for cc32xx, k32w, mbed, nrfconnect, qpg)
|
Pinged @kkasperczyk-no as the original author. I am unsure, but at least tracking does make sense: we should update/fix that file or delete if unused. |
|
PR #29264: Size comparison from 3400d1d to 575f8b1 Increases (33 builds for bl702, bl702l, cc13x4_26x4, cyw30739, k32w, nrfconnect, qpg, telink)
Decreases (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Full report (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
We use these shell commands in our internal tests on hardware. Maybe the command should use AddressResolve instead, since this is currently the "public" API for node address resolution? |
I think that we do not use it anymore in our tests. Nevertheless I can recall it was recommended to use this commands during Matter 1.0 certification tests. I don't remember what test cases they were, however the commands were used to verify that the device is able to properly resolve Matter DNS services. It was also very useful tool for troubleshooting of the problems with DNS and SRP compatibility between OpenThread and other implementations we had in the past. All in all this is quite useful, but only a debug utility and probably it is currently not used very often. If it would be possible to align it to use the new API, it would be great, but otherwise we should think what brings more value to the project, as I would not like to block potentially good changes to the API. |
…and more cleanups on members/methods
Replaced that with AddressResolver, resolution should now be possible with Dns.cpp commands. Difference is that AddressResolver will only return one address instead of many, however that is likely sufficient for debug purposes and checking that dnssd works. |
Updated to use AddressResolve. |
bzbarsky-apple
approved these changes
Sep 15, 2023
|
PR #29264: Size comparison from 3400d1d to 3d857ca Increases above 0.2%:
Increases (17 builds for bl602, bl702, cc13x4_26x4, cyw30739, esp32, linux, psoc6, telink)
Decreases (64 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Full report (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
yunhanw-google
approved these changes
Sep 15, 2023
|
PR #29264: Size comparison from 3400d1d to b030b98 Increases above 0.2%:
Increases (22 builds for bl602, bl702, cyw30739, esp32, linux, psoc6, telink)
Decreases (64 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Full report (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
* Stop using ResolverProxy for performing Operational node discovery * Switch avahi to be capable to keep track of contexts and handle stopping node resolution * Restyle * Remove obsolete todo * Make sure stop resolve actuall clears multiple entries if they exist * increase log level verbositity for openiot to see progress in pytest * A bit more rich logging in the openiot utils - show successes and failures * report failure on commissioning (likely timeout, but show it since it is not none/bool * Restyle * Switch Dns.cpp to use address resolver for dns resolution * Restyle * remove operational delegate functionality from ResolverDelegateProxy and more cleanups on members/methods * Restyle * Schedule next result to display all results of a discovery * Fix namespacing * Code review update --------- Co-authored-by: Andrei Litvin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #28694 (memory leak because stopping resolution was not implemented).
We currently use AddressResolve to handle node resolution, so a separate singleton resolver proxy should not be needed for handling node resolution.
Actual changes:
Dns.cppto use AddressResolver interface instead of ResolverProxy for operational discovery.Tested
validated that node resolution still works when node is advertised (CI should also validate that) and that there are no memory leaks if we stop a resolution that does not find anything (i.e. cancel works and memory is freed up).
TODO notes