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

Implements sharing a route across spaces #2803

Merged
merged 1 commit into from
May 20, 2022
Merged

Implements sharing a route across spaces #2803

merged 1 commit into from
May 20, 2022

Conversation

klakin-pivotal
Copy link
Contributor

@klakin-pivotal klakin-pivotal commented May 13, 2022

This commit adds 'POST /v3/routes/:guid/relationships/shared_spaces'.
The endpoint's behavior currently matches service instance sharing behavior.
Route sharing does not handle isolation segments.

This endpoint is currently experimental.

This commit also makes minor changes to the error message transmitted
when you fail to share a Service Instance. The existing message didn't
tell you what instance failed to share, or the space that caused the
failure, which are both nice things for an operator to know.

Here is a note about some implementation detail for future programmers:

Route.shared? does not make use of the class's shared_spaces
member variable. This is because shared_spaces is an Array, which
indicates that -behind the scenes- Sequel will first fetch all of the
data for all of the matching objects in the association, wrap that in
an array, and then hand it to us. Given that we only want to know if
there are more than zero items in that array, this could be a huge
waste on Foundations that have a route shared between many spaces.

So, we explicitly search for Spaces associated with the Route and tell
the ORM to tell us if there is more than one. The Sequel docs claim that
using empty? on a Dataset object implicitly adds in a LIMIT 1
to the query that the ORM generates, so the code as written should only
ever send us one row at most.

For reference, this Pivotal Tracker story tracks this work, but it may not be accessible for folks who aren't a VMWare employee: https://www.pivotaltracker.com/story/show/181734583

Co-authored-by: Alex Rocha [email protected]
Co-authored-by: Merric de Launey [email protected]
Co-authored-by: Kenneth Lakin [email protected]

==============================================

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:
    See the description above

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

This commit adds 'POST /v3/routes/:guid/relationships/shared_spaces'.
The endpoint's behavior currently matches service instance sharing behavior.
Route sharing does not handle isolation segments.

This endpoint is currently experimental.

This commit also makes minor changes to the error message transmitted
when you fail to share a Service Instance. The existing message didn't
tell you what instance failed to share, or the space that caused the
failure,  which are both nice things for an operator to know.

Here is a note about some implementation detail for future programmers:

`Route.shared?` does not make use of the class's `shared_spaces`
member variable. This is because `shared_spaces` is an `Array`, which
indicates that -behind the scenes- Sequel will first fetch all of the
data for all of the matching objects in the association, wrap that in
an array, and then hand it to us. Given that we only want to know if
there are more than zero items in that array, this could be a huge
waste on Foundations that have a route shared between many spaces.

So, we explicitly search for `Spaces` associated with the `Route` and tell
the ORM to tell us if there is more than one. The Sequel docs claim that
using `empty?` on a `Dataset` object implicitly adds in a `LIMIT 1`
to the query that the ORM generates, so the code as written should only
ever send us one row at most.

[#181757727]

Co-authored-by: Alex Rocha <[email protected]>
Co-authored-by: Merric de Launey <[email protected]>
Co-authored-by: Kenneth Lakin <[email protected]>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 13, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: xandroc / name: Alex (8934f06)

@MerricdeLauney MerricdeLauney merged commit 6072ed6 into main May 20, 2022
@moleske moleske deleted the share-routes branch May 20, 2022 20:28
philippthun added a commit to sap-contributions/cloud_controller_ng that referenced this pull request Jul 11, 2022
There are already several PRs (see below [1]) that change dataset.any?
to !dataset.empty? or otherwise mention the difference between these two
statements. Whereas the first one fetches all data from the database to
then check if the resulting array contains at least one element, the
second statement does a 'select 1 as one from table limit 1' query that
has a far better performance.

Fortunately there is a Sequel extension (see [2]) that 'fixes' this
misleading behavior; so instead of searching for more places where we
could change the implementation, using dataset.any? should now be
equivalent to !dataset.empty? and everyone can choose freely which one
to use.

[1]
cloudfoundry#2117
cloudfoundry#2533
cloudfoundry#2803
cloudfoundry#2855

[2] https://sequel.jeremyevans.net/rdoc-plugins/files/lib/sequel/extensions/any_not_empty_rb.html
will-gant pushed a commit to sap-contributions/cloud_controller_ng that referenced this pull request Dec 16, 2022
There are already several PRs (see below [1]) that change dataset.any?
to !dataset.empty? or otherwise mention the difference between these two
statements. Whereas the first one fetches all data from the database to
then check if the resulting array contains at least one element, the
second statement does a 'select 1 as one from table limit 1' query that
has a far better performance.

Fortunately there is a Sequel extension (see [2]) that 'fixes' this
misleading behavior; so instead of searching for more places where we
could change the implementation, using dataset.any? should now be
equivalent to !dataset.empty? and everyone can choose freely which one
to use.

[1]
cloudfoundry#2117
cloudfoundry#2533
cloudfoundry#2803
cloudfoundry#2855

[2] https://sequel.jeremyevans.net/rdoc-plugins/files/lib/sequel/extensions/any_not_empty_rb.html
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.

4 participants