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

Clarify no pagination on static provider endpoints /trips and /status_changes #424

Merged

Conversation

hyperknot
Copy link
Contributor

@hyperknot hyperknot commented Jan 9, 2020

Explain pull request

It clarifies that the static provider endpoints (/trips and /status_changes) should not have server-side pagination.

Is this a breaking change

A breaking change would require consumers or implementors of the API to modify their code for it to continue to function (ex: renaming of a required field or the change in data type of an existing field). A non-breaking change would allow existing code to continue to function (ex: addition of an optional field or the creation of a new optional endpoint).

  • No, not breaking

Impacted Spec

Which spec(s) will this pull request impact?

  • provider

Additional context

As one of the contributors who proposed the original static-file backed provider endpoint idea for /trips and /status_changes endpoints, I believe it's important that there should be no server-side pagination on the short time intervals.

Longer explanation is copied from my original post here:


One more important point I'd like to raise is the removal of pagination from static-backed endpoints.

1. Pagination currently is the most "broken" part of the MDS specs. I work on MDS acquisition at Populus, we work with 10+ operators' MDS API, and pagination code is by far the most complex part of our MDS acquisition infrastructure. Most operators have slightly different behavior, and errors are not uncommon.

Some operators do not support pagination at all but require us to query data in hourly blocks (exactly like the new static endpoints), some do not add links to the last page, some require custom query parameters and the list goes on.

2. Since on the static endpoints, the time-period is fixed, I went ahead and analyzed all our historical data.

=> The average size for an hourly block of trips data, gzipped is 100 kB.
=> The maximum, extreme value for an hourly block of trips data is 7 MB, but this is very rare.

Since we are talking about servers in datacenters communicating with each other, even sending 7 MB in a single HTTPS request should be a non-issue today. Status changes are tiny in comparison, there is no point in analyzing them.

=> As a conclusion, I recommend removing pagination from the static-backed endpoints (provider /trips and /status_changes).

@hyperknot hyperknot requested a review from a team as a code owner January 9, 2020 03:12
@hyperknot hyperknot requested a review from a team January 9, 2020 03:12
@claassistantio
Copy link

claassistantio commented Jan 9, 2020

CLA assistant check
All committers have signed the CLA.

@sarob sarob added this to the Future milestone Jan 9, 2020
@sarob sarob added enhancement New feature or request Provider Specific to the Provider API labels Jan 10, 2020
@jiffyclub
Copy link
Contributor

jiffyclub commented Jan 22, 2020

Looking at the provider spec I don't see anything that requires the trips and status changes endpoints to be implemented via static files, they could choose to implement it as an API endpoint that queries a database.

In fact I'm little unclear on how we imagine operators will implement this with static files. Do these endpoints support redirect responses?

All that said, we could still disallow pagination in the trips and status changes endpoints, but I'd phrase it as:

The /trips and /status_changes APIs should not use pagination. If providers choose to use pagination for the /events endpoint the pagination must comply with the JSON API specification.

@thekaveman thekaveman modified the milestones: Future, 0.4.1 Jan 22, 2020
@thekaveman thekaveman added documentation documentation change can be for code and/or markdown pages and removed enhancement New feature or request labels Jan 29, 2020
@jiffyclub
Copy link
Contributor

I'll update this PR with my language from above.

@hyperknot
Copy link
Contributor Author

Thanks, it is much cleaner this way!

Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

The language change looks great! As we've requested in other PRs, please do not include additional cleanup type fixes elsewhere in the document that aren't related to the specific change you are proposing. It makes tracing history and review much more complicated (in general, probably not in this specific case). We are more than happy to accept simple self-contained cleanup PRs for things like spacing etc.

@hyperknot
Copy link
Contributor Author

@thekaveman you mean the end-of-line empty space removal in @jiffyclub's commit? It was probably done by his text editor I guess.

@thekaveman
Copy link
Collaborator

@thekaveman you mean the end-of-line empty space removal in @jiffyclub's commit?

In this case, yes that is what I am referring to.

@hyperknot
Copy link
Contributor Author

PR updated with removed formatting.

Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

May I request one final change:

-- The `/trips` and `/status_changes` APIs should not use pagination.
++ The `/trips` and `/status_changes` APIs must not use pagination.

This aligns with our usage of "must" to imply "required for valid implementation".

@hyperknot
Copy link
Contributor Author

May I request one final change

Updated

@jiffyclub
Copy link
Contributor

Yes, sorry, my editor automatically trims trailing white space.

Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Thank you!

@thekaveman thekaveman merged commit 0e18b1a into openmobilityfoundation:dev Feb 19, 2020
@hyperknot hyperknot deleted the no-static-pagination branch July 8, 2020 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation change can be for code and/or markdown pages Provider Specific to the Provider API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants