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

cf service-access performance #483

Merged
merged 4 commits into from Jun 23, 2015
Merged

cf service-access performance #483

merged 4 commits into from Jun 23, 2015

Conversation

Gerg
Copy link
Member

@Gerg Gerg commented Jun 18, 2015

To improve performance of the service access command we changed the way that we:

  1. Get service offerings for brokers
  2. Get service plans for service offerings
  3. Get organization names for service plan visibilities

Each change has its own commit with further details in the commit message. Let us know if you have any questions about our changes.

Tracker Story: https://www.pivotaltracker.com/story/show/96912380

Gerg and others added 3 commits June 18, 2015 15:38
- Get all service offerings in one request instead of a request per
  broker

[#96912380]
- Get all service plans in one request instead of a request per service
  offering

[#96912380]
- To map org guids to org names, we make individual requests for each
  org instead of requesting all orgs.
  - This is optimized for the case where there are fewer orgs associated
    with service_plan_visibilities than the total number of org pages.
    This seemed to be the case on all environments we checked.
  - /v2/organizations does not support filtering on a list of org or
    service_plan_visiblility guids, so we have to make separate GETs
- In plan_builder, there are package variables that are used to memoize
  maps. This causes pollution plan_builder tests, so we nil them in test
  setup

[#96912380]
@cfdreddbot
Copy link

Hey Gerg!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/97344958.

@@ -43,6 +44,22 @@ func (repo CloudControllerServicePlanRepository) Update(servicePlan models.Servi
return repo.gateway.UpdateResource(repo.config.ApiEndpoint(), url, strings.NewReader(body))
}

func (repo CloudControllerServicePlanRepository) ListPlansFromManyServices(serviceGuids []string) (plans []models.ServicePlanFields, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to avoid using inline return variable declarations (like plans and err are here) because it can be hella confusing. Would you guys be ok with changing this to just have a generic return types and then declare plans in the function?

@jberkhahn
Copy link
Contributor

Hey guys, just had a few stylistic issues. We try to shy away from inline declaration of return variables because it can be hard to parse.

@Gerg
Copy link
Member Author

Gerg commented Jun 22, 2015

Your word is my command.

- And return them by name
@Gerg
Copy link
Member Author

Gerg commented Jun 23, 2015

We fixed the style issues.

jberkhahn added a commit that referenced this pull request Jun 23, 2015
@jberkhahn jberkhahn merged commit a4ecfe6 into master Jun 23, 2015
@simonleung8 simonleung8 deleted the service_access_performance branch October 30, 2015 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants