theCrag / website

theCrag.com: Add your voice and help guide the development of the world's largest collaborative rock climbing & bouldering platform
https://www.thecrag.com/
111 stars 8 forks source link

API: facet ascent search #2205

Open nicHoch opened 8 years ago

nicHoch commented 8 years ago

To use this endpoint also for logbook updates in the APP we need an "since" parameter (lastUpdated). If we also support the sortby: lastUpdate/since we could avoid to much logic with api pagination. Than the next bunch of ascents for a logbook will just show up with the next refresh.

Does the inflate property include inflated items (routes) for the current page or for the entire dataset?

scd commented 8 years ago

Does the inflate property include inflated items (routes) for the current page or for the entire dataset?

Good question. It is the current page. I only inflate if the route is on the page. Inflating the whole dataset will be quite expensive. The size of the database means we are moving to infinite queries anyway, in which case the whole data set is not a valid concept. If you don't have to return or even count the whole dataset then mysql can do some optimisation when a full page of results has been returned. There could be 700k ascents in a dataset. In 5 years, it may be 10M ascents.

This means there are potentially duplicate inflate between page views. I would suggest the default page size are too small for the app. If we want to get a persons full logbook then maybe we should be getting 1000 ascents per page. This would make the duplicate inflates between queries less of a problem. Use the 'perPage=1000' parameter.

add since parameter

done in repo

add sortby: since

done in repo

in the description it is not clear what date formate is expected for: after

2014-12-19

Updated in doco.

if i use logged-after the endpoint crashes

It worked for me. Could it be because of the format. I think the end point is not very informative if you have the wrong arguments. I am not happy about this so maybe I have to do some more work here.

the second example link in the new description is not working

Updated. It was missing the facet bit of the url stub

@brendanheywood could you please pull and update dev for nicky.

brendanheywood commented 8 years ago

deployed

I will cause a lot of issues by attempting to ignore pagination, or avoiding logic around pagination. Scale is important and we need to get it right from the start. eg there are heaps of climbers who have more than 1000 ascents, just making the page size bigger is the wrong answer, although tuning it's size is appropriate. Optimizations like handling diffs using lastUpdated is awesome but shouldn't be used by itself, you still need to handle the normal case of grabbing everything you need, over many pages, and ideally this should be pretty low level code which is used in turn by everything from ascents to routes to users or whatever.

scd commented 8 years ago

Are you saying we should also have a flag which says to do 'no_pagination' in the facet search?

Pagination is not in the bulk index calls for the very reason you alluded to, so I sort of agree for some cases in the facet search. In particular user logbook.

I think I would like to control when no_pagination is allowed. For example if the user is set then I am happy to allow no pagination.

@nicHoch thoughts?

brendanheywood commented 8 years ago

No - I'm saying the client should have only very limited control over pagination, and should correctly handle multiple pages at a deep level. Ideally the client's suggestion of a page size could not be honoured by the api, and the api returns what it thinks best and the client needs to be able to gracefully handle it. Turning off pagination sounds like a really terrible idea and I wouldn't want it at all, ever

nicHoch commented 8 years ago

No it was not my intention to skip pagination at all just to see it from an other perspective. If we order after lastUpdated and store in the app the latest item (node) update time, then the app does not have to deal with real pagination (knowing i am on page 2 of 5 and hopefulle 3 to come ...). The app just recognizes (by the returned perPage number and returned data items): there is more. Now the app can decide to go into a next update cycle or not (later).

I also like the idea that the user given perPage parameter can be lowered from the API on demand (for load balancing ...).

nicHoch commented 8 years ago

in the lastUpdate property sometimes "null" is returned. Is that an error or by intention? A fresh inserted node has already the updateTime=createTime or is it null until the first real update is done?

scd commented 8 years ago

LastUpdate is only set after first change to the record. For purely MySQL statement reasons this is not ideal and I plan to change the logic.

Do you want me to set it for the API.

nicHoch commented 8 years ago

Yes would be nice if you can handle that on your side. Also important is the correct sort order in connection with "since". Order by with null values sometime behave a bit unexpected ...

Nicky

scd commented 8 years ago

That should be ok as I have done what you suggested behind the scenes. So unless there is a bug it should be ok.

brendanheywood commented 8 years ago

How are we handling the sorting and pagination when a large batch of updates all have the same last update and the batch size is larger than the page size?

nicHoch commented 8 years ago

the normal way is pagination. But if something is interrupted i do not store: "i was on page 3 of 5 and so have to go on with 4" - but i save "last update" and start a new update process later.

scd commented 8 years ago

I think there is a secondary sort by id to keep it consistent between pages On May 26, 2016 00:53, "Nicky Hochmuth" notifications@github.com wrote:

the normal way is pagination. But if something is interrupted i do not store: "i was on page 3 of 5 and so have to go on with 4" - but i save "last update" and start a new update process later.

— You are receiving this because you were assigned. Reply to this email directly or view it on GitHub https://github.com/theCrag/website/issues/2205#issuecomment-221601391

nicHoch commented 8 years ago

looks like the heightfield is always null