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

DRS bundle contents pagination #367

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

jb-adams
Copy link
Member

Issues: #366 , #325

Built Docs: link

This PR aims to specify pagination of the contents array for a single DRS Object. This is done by adding a page request query parameter, and a Link response header to inform the client of the traversal links to various pages.

Open Questions:

  • A DRS Object has multiple array-type attributes. In addition to contents there is also checksums, access_methods, aliases. Do we foresee any of these attributes requiring pagination as well? If yes, we would likely need to rename the new parameter and header to avoid collision downstream (e.g. Link to ContentsLink)
  • Does this introduction constitute a breaking change, since pagination is introduced on an existing endpoint that didn't have it previously? If so, we should think about how to get around this (such as another new query parameter that, when absent, causes the server to return the default, non-paginated response)

@jb-adams jb-adams force-pushed the feature/issue-366-bundle-contents-pagination branch from 5fb708e to c87b13a Compare January 4, 2022 17:30
@jb-adams
Copy link
Member Author

jb-adams commented Jan 4, 2022

Additional notes/questions:

  • for the POST /objects/{object_id} endpoint, page is still currently listed as a query string param. This is to keep it aligned with the links returned in the Link header
  • if we decide to rename Link to ContentsLink (or something similar) to make it explicit that it's for contents pagination, we should also rename the query string parameter from page to contents_page (for example).

@ianfore
Copy link

ianfore commented Jan 19, 2022

See also #377

@jb-adams
Copy link
Member Author

Following the Jan 10th Cloud WS call, I moved the paginated contents array function to 2 new endpoints:

This was done to address the original awkwardness of having pagination for an attribute within the returned DrsObject. Now, the pagination affects the top-level array of the response. @patmagee @aniewielska

We may need to back up the conversation a bit though, as @dglazer and @ianfore propose in #377 dropping bundles, and hence contents pagination, altogether. I'd prefer to arrive at an answer on whether bundles should be scrapped before doing more work in the branch.

@MichaelLukowski it would be really helpful to get your feedback on the U Chicago bundle use case to inform the conversation.

@briandoconnor

@MichaelLukowski
Copy link
Collaborator

@jb-adams in our experience of implementing bundles we have found that they grow to be too large and pagination becomes difficult to implement. We are in favor of dropping bundles and moving forward with the batch approach. As discussed in the FASP hackathon U Chicago is working on a "packaging" approach that doesn't have to be standardized but could be used as a "best-practice"

@jb-adams
Copy link
Member Author

Thanks for the feedback @MichaelLukowski ! Can you also relay your preference to drop bundles in thread #377 ? That's the PR that would remove bundles from the spec

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.

3 participants