-
Notifications
You must be signed in to change notification settings - Fork 881
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
replace individual endpoint_cnt read from store with 1 bulk read #1632
Conversation
fe4b61c
to
c109fcf
Compare
@@ -152,21 +153,24 @@ func (c *controller) getNetworksFromStore() ([]*network, error) { | |||
continue | |||
} | |||
|
|||
kvep, err := store.Map(datastore.Key(epCntKeyPrefix), &endpointCnt{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the diffs, I am realizing there was no need for getNetworks()
to reconstruct the epCount for each pulled network from store. Whatever code that makes a decision about the epCount, it does pull the latest epCount from store itself. There is no need to retrieve and construct the internal epCounts in the networks retrieved as list.
My advice is to simply remove the existing code which pulls the epCount in the loop below.
It is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goal now is to improve the performance of current code in respect to the getNetworksFromStore().
Given getNetworksFromStore()
retrieves the list of networks in a batch, it makes a lot of sense to retrieve the epCnt as a batch along it.
We can address the code removal I was suggesting in a subsequent PR outside of the release cycle.
endpoint_info.go
Outdated
// endpoint hasn't joined any sandbox. | ||
// Just return the endpoint | ||
return ep | ||
n, err := ep.getNetworkFromStore() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please push this optimization as a separate commit inside this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. will do
9480533
to
260f0fa
Compare
getNetworksFromStore reads networks and endpoint_cnt from the kvstores. endpoint_cnt especially is read in a for-loop for each network and that causes a lot of stress in poorly performing KV-Stores. This fix eases the load on the kvstore by fetching all the endpoint_cnt in a single read and the operation is performed on it. Signed-off-by: Madhu Venugopal <[email protected]>
Signed-off-by: Madhu Venugopal <[email protected]>
LGTM |
getNetworksFromStore reads networks and endpoint_cnt from the kvstores.
endpoint_cnt especially is read in a for-loop for each network and that
causes a lot of stress in poorly performing KV-Stores.
This fix eases the load on the kvstore by fetching all the endpoint_cnt
in a single read and the operation is performed on it.
Signed-off-by: Madhu Venugopal [email protected]