-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Do we have two dns caches? #12762
Comments
It would be helpful to add link references that jump to where the two implementations are in the code. Also, could extra clarification be added about why and how some platforms may have three mdns caches in the proposed solution? I believe the mention of three implementations is expanding on the problem, as platform packages such as avahi add a third cache implementation, but that is unclear in the bug description. |
Third cache would be the internal cache in the platform impl (not always there - depends on the impl). If you were running avahi, the dns result would be cached in the avahi impl, the platform cache and the controller cache. SDK cache impl is src/lib/dnssd/DnssdCache.h. It's instantiated in both CHIPDeviceController.cpp and Discovery_ImplPlatform. see https://github.com/project-chip/connectedhomeip/blob/master/src/controller/CHIPDeviceController.h line 341 and If we're going to vote one off the island, the one in Discovery_ImplPlatform has the least utility IMO. Though perhaps I'm missing some context from the original author. See #11632 |
@cecille seems like this could be needed for 1.0? |
Could be - it's wasteful at the very least. |
Let's defer until we know this is required? |
Assigned to me as I believe I am touching this as part of mDNS resolution logic (I need to have a 'get best address' logic and the DNS caching is actually in the way in parts). Not working to really fix it, but it may be touched as a sideffect. |
Removed caching. |
Problem
Looks like the case manager introduced one, and we also have one in the platform impl.
Proposed Solution
We should have one or even none - in the case of platform we might actually have three.
The text was updated successfully, but these errors were encountered: