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

Reduce the number of DB requests made in ValidateServerCapabilities() #5212

Closed
rawlinp opened this issue Oct 28, 2020 · 3 comments · Fixed by #6077
Closed

Reduce the number of DB requests made in ValidateServerCapabilities() #5212

rawlinp opened this issue Oct 28, 2020 · 3 comments · Fixed by #6077
Labels
good first issue first-time committers will find this easy to resolve improvement The functionality exists but it could be improved in some way. performance impacts/improves/measures performance Traffic Ops related to Traffic Ops

Comments

@rawlinp
Copy link
Contributor

rawlinp commented Oct 28, 2020

I'm submitting a ...

  • improvement request (usability, performance, tech debt, etc.)

Traffic Control components affected ...

  • Traffic Ops

Current behavior:

TO currently makes a DB request per server in the ValidateServerCapabilities() function:

sCaps, err = dbhelpers.GetServerCapabilitiesFromName(name, tx)
. When a delivery service is being assigned to a large number of servers, this slows down the total execution time unnecessarily.

New behavior:

Instead of making a DB request per server, the function should make a single DB request to get data for all the servers at once, then perform the validation. This will make it much faster to assign a delivery service to large numbers of servers at a time.

Minimal reproduction of the problem with instructions:

Assign a delivery service with required capabilities to a large number of servers with those capabilities at a time.

@rawlinp rawlinp added Traffic Ops related to Traffic Ops good first issue first-time committers will find this easy to resolve performance impacts/improves/measures performance improvement The functionality exists but it could be improved in some way. labels Oct 28, 2020
@RaviTezu
Copy link

@rawlinp I would like to take this up. Could you please assign this to me?

@RaviTezu
Copy link

RaviTezu commented Oct 29, 2020

@rawlinp Just want to check that I am going in the right direction before I start on this.

  • I am thinking of modifying GetServerCapabilitiesFromName method to take a list of server names and then modify the query in way that it can fetch the data/capabilities for all the servers and return them for validation.
  • Also, I would like to know where I can get more information on how to construct the query. Thanks.

@rawlinp
Copy link
Contributor Author

rawlinp commented Oct 29, 2020

@RaviTezu that sounds like a reasonable idea. This query should be a good example for you to modify for GetServerCapabilitiesFromName:

SELECT
s.id,
s.cdn_id,
c.name,
ARRAY_REMOVE(ARRAY_AGG(ssc.server_capability ORDER BY ssc.server_capability), NULL) AS capabilities
FROM server s
LEFT JOIN server_server_capability ssc ON ssc.server = s.id
JOIN cachegroup c ON c.id = s.cachegroup
JOIN topology_cachegroup tc ON tc.cachegroup = c.name
WHERE
s.cdn_id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)
AND tc.topology = $2
GROUP BY s.id, s.cdn_id, c.name

rawlinp added a commit to rawlinp/trafficcontrol that referenced this issue Jul 30, 2021
Also, fix some cases where it was possible to delete/unassign all
`EDGE`-type servers from a delivery service if there were also
`ORG`-type servers assigned.

Closes: apache#6069
Closes: apache#5212
zrhoffman pushed a commit that referenced this issue Aug 3, 2021
…d DS (#6077)

* Prevent unassigning all ONLINE ORG servers from active MSO-enabled DS

Also, fix some cases where it was possible to delete/unassign all
`EDGE`-type servers from a delivery service if there were also
`ORG`-type servers assigned.

Closes: #6069
Closes: #5212

* Fix v2 and v3 TO API integration tests

* Address review comments

Simplify queries, remove double-space in alert message, and change if to
else-if.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue first-time committers will find this easy to resolve improvement The functionality exists but it could be improved in some way. performance impacts/improves/measures performance Traffic Ops related to Traffic Ops
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants