Skip to content
This repository has been archived by the owner on Feb 27, 2020. It is now read-only.

Remove connection pooling #1947

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Remove connection pooling #1947

wants to merge 2 commits into from

Conversation

MatMeredith
Copy link
Contributor

UTs run and tested in Kubernetes deployment. Don't think I understand the implications or the background to why we use connection pooling in Bono well enough to know if this is the right thing to do though.

We'd also have to retire the BonoConnectedSprouts table from the MIB if we were to merge this.

@MatMeredith
Copy link
Contributor Author

@johadalin @AlexHockey FYI... Not clear whether we should be looking to merge at this time -- just wanted to see if it solved the Kubernetes issue...

@AlexHockey
Copy link
Contributor

Does this cause Bono to do DNS lookups using the BaseResolver (good) or does it cause the resolution to happen in PJSIP (bad)?

@MatMeredith
Copy link
Contributor Author

I don't fully understand the code, but from trace on a node with this patch...

02-10-2017 14:19:45.886 UTC Debug pjutils.cpp:474: Next hop node is encoded in top route header
02-10-2017 14:19:45.886 UTC Debug sipresolver.cpp:62: SIPResolver::resolve for name sprout.mgm.svc.cw-k8s.test, port 5052, transport 6, family 2
02-10-2017 14:19:45.886 UTC Debug utils.cpp:418: Attempt to parse sprout.mgm.svc.cw-k8s.test as IP address
02-10-2017 14:19:45.886 UTC Debug sipresolver.cpp:125: Port is specified
02-10-2017 14:19:45.886 UTC Debug sipresolver.cpp:293: Perform A/AAAA record lookup only, name = sprout.mgm.svc.cw-k8s.test
02-10-2017 14:19:45.886 UTC Verbose dnscachedresolver.cpp:468: Check cache for sprout.mgm.svc.cw-k8s.test type 1
02-10-2017 14:19:45.886 UTC Debug dnscachedresolver.cpp:474: No entry found in cache
02-10-2017 14:19:45.886 UTC Debug dnscachedresolver.cpp:477: Create cache entry pending query

I think that's what we want?

@MatMeredith
Copy link
Contributor Author

@AlexHockey ^^

@AlexHockey
Copy link
Contributor

Yep, looks good to me.

@johadalin
Copy link
Contributor

Should this be merged down?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants