Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

updated /deliveryservices/{{ID}}/servers and /deliveryservices/{{ID}}…#4676

Merged
ocket8888 merged 9 commits intoapache:masterfrom
mattjackson220:DHdsIdServer
Jun 15, 2020
Merged

updated /deliveryservices/{{ID}}/servers and /deliveryservices/{{ID}}…#4676
ocket8888 merged 9 commits intoapache:masterfrom
mattjackson220:DHdsIdServer

Conversation

@mattjackson220
Copy link
Copy Markdown
Contributor

@mattjackson220 mattjackson220 commented Apr 30, 2020

…/servers/eligible to use multiple interfaces

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Ops

What is the best way to verify this PR?

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

  • This PR includes tests
  • This PR includes documentation
  • This PR includes an update to CHANGELOG.md
  • This PR includes any and all required license headers
  • This PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

@mattjackson220 mattjackson220 added documentation related to documentation Traffic Ops related to Traffic Ops Traffic Ops API Next Improvements to Traffic Ops API - particularly breaking changes labels Apr 30, 2020
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually be "null means 'no limit'" - that's one thing that's changed since the blueprint was merged

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above RE null vs 0

Comment thread lib/go-tc/deliveryservice_servers.go Outdated
Comment on lines 104 to 118
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GoDoc on these?

Comment thread lib/go-tc/servers.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: GoDocs should end with a period.

Comment thread lib/go-tc/servers.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above RE: period

Comment thread lib/go-tc/servers.go Outdated
Comment on lines 338 to 339
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: GoDocs should consist of complete sentences with proper capitalization and terminated by periods.

Comment thread lib/go-tc/servers.go Outdated
Comment on lines 345 to 346
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above RE: complete sentences

Comment thread lib/go-tc/servers.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above RE: period.

Comment thread lib/go-tc/deliveryservice_servers.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make DSServer point to the current definition of a Delivery Service Server? Coupling to the API version means that things stop making sense unless we update that every time the API version changes; like if we have DSServerV30 then APIv3.1 comes out and no changes are made to the structure, we're either using a confusingly version structure, or we have to remember to do something like type DSServer31 = DSServer30 and then do a find/replace to update things.

Comment thread traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go Outdated
Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All unit and api/client integration tests passing, manually verified old-format output in api v1/v2 and new-format output in v3

@ocket8888 ocket8888 merged commit 78b6637 into apache:master Jun 15, 2020
s.ip_gateway,
s.ip_netmask,
ARRAY (
SELECT ( json_build_object (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this does worry me a bit still. When reviewing @ocket8888's PR I found that this query using json_build_object is like 12x slower than querying the interfaces and IPs separately (without using json_build_object) and manually populating the servers structs with that data. Also, we should really be making the same query here as we would in the /servers API. It seems like we could make this better by sharing that code/query here.

Locally (which I typically assume performs much better than in Prod), for a DS w/ ~1000 servers assigned, this API takes 850ms for me, whereas before this PR, it takes ~75ms.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True enough. This is also still how /servers/details is being done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation related to documentation Traffic Ops API Next Improvements to Traffic Ops API - particularly breaking changes Traffic Ops related to Traffic Ops

Projects

None yet

3 participants