Changeset View
Standalone View
swh/storage/cassandra/storage.py
| Show First 20 Lines • Show All 856 Lines • ▼ Show 20 Lines | ) -> PagedResult[OriginVisit]: | ||||
| assert len(visits) <= extra_limit | assert len(visits) <= extra_limit | ||||
| if len(visits) == extra_limit: | if len(visits) == extra_limit: | ||||
| visits = visits[:limit] | visits = visits[:limit] | ||||
| next_page_token = str(visits[-1].visit) | next_page_token = str(visits[-1].visit) | ||||
| return PagedResult(results=visits, next_page_token=next_page_token) | return PagedResult(results=visits, next_page_token=next_page_token) | ||||
| def origin_visit_status_get( | |||||
| self, | |||||
| origin: str, | |||||
| visit: int, | |||||
| page_token: Optional[str] = None, | |||||
| order: ListOrder = ListOrder.ASC, | |||||
| limit: int = 10, | |||||
| ) -> PagedResult[OriginVisitStatus]: | |||||
| if not isinstance(order, ListOrder): | |||||
| raise StorageArgumentException("order must be a ListOrder value") | |||||
| if page_token and not isinstance(page_token, str): | |||||
douardda: do we really need those? I mean, these are type annotated, so it seems unnecessary to me to add… | |||||
Done Inline Actionsexcept until recently it was moot... But that reassures me a bit to have those ;) ardumont: except until recently it was moot...
D3635 fixed it...
But that reassures me a bit to have… | |||||
Done Inline Actionsbut it adds code and (at least) "visual complexity", so I'd rather say "no", but I won't fight too hard, it's not a big deal, really. douardda: but it adds code and (at least) "visual complexity", so I'd rather say "no", but I won't fight… | |||||
| raise StorageArgumentException("page_token must be a string.") | |||||
| next_page_token = None | |||||
| if page_token is None: | |||||
| date_from = None | |||||
| else: | |||||
| date_from = datetime.datetime.fromisoformat(page_token) | |||||
| visit_statuses: List[OriginVisitStatus] = [] | |||||
| extra_limit = limit + 1 | |||||
Done Inline Actionsa small comment on the reason for this +1 could be handy (aka get one more result to handle the next_page logic). Also not sure this extra_limit variable is really helpful, as well as the assertion below seems unnecessary to me (add unit tests for _cql_runner.origin_visit_status_get_range is needed, then trust them). douardda: a small comment on the reason for this +1 could be handy (aka get one more result to handle the… | |||||
Done Inline Actionsi use the extra-limit variable to avoid having + 1 all over the places which i found annoying to read and prone to error. ardumont: i use the extra-limit variable to avoid having + 1 all over the places which i found annoying… | |||||
Done Inline ActionsYou can avoid multiple +1 without extra_limit: rows = self._cql_runner.origin_visit_status_get_range(
origin, visit, date_from, limit+1, order
)
...
if len(visit_statuses) > limit:
...vlorentz: You can avoid multiple `+1` without `extra_limit`:
```
rows = self._cql_runner. | |||||
| rows = self._cql_runner.origin_visit_status_get_range( | |||||
| origin, visit, date_from, extra_limit, order | |||||
| ) | |||||
| for row in rows: | |||||
| visit_statuses.append(converters.row_to_visit_status(row)) | |||||
| assert len(visit_statuses) <= extra_limit | |||||
| if len(visit_statuses) == extra_limit: | |||||
| visit_statuses = visit_statuses[:limit] | |||||
Done Inline Actionsthis is wrong, the next_page_token should be before the visit_statuses above. douardda: this is wrong, the next_page_token should be before the visit_statuses above. | |||||
Done Inline Actionstests disagrees there. ardumont: tests disagrees there. | |||||
Done Inline Actionsthen either tests are wrong or the limit+1 is useless, since you never use the extra returned value... douardda: then either tests are wrong or the limit+1 is useless, since you never use the extra returned… | |||||
Done Inline ActionsI'm using it but not for its value for its presence. ardumont: I'm using it but not for its value for its presence. | |||||
Done Inline Actionsbut like i said, i'll try your version which seems simpler to read and to the point alright ;) ardumont: but like i said, i'll try your version which seems simpler to read and to the point alright ;) | |||||
Done Inline Actionsok then this MUST be documented somehow because it really feels wrong for the fellow dev reading this piece of code. BTW, do we really need to gather this extra result from the base? why not just add the next link if len(rows) == limit (according we do not use limit+1 when calling it). Worst case, we do an extra call with no results if the total number of visits is a multiple of limit. douardda: ok then this MUST be documented somehow because it really feels wrong for the fellow dev… | |||||
Done Inline ActionsIt's not just the test that disagrees. next_page_token is the date of the last returned visit, and the next call will return all visits with a date strictly greater than the one in the token vlorentz: It's not just the test that disagrees. `next_page_token` is the date of the last returned visit… | |||||
| next_page_token = str(visit_statuses[-1].date) | |||||
| return PagedResult(results=visit_statuses, next_page_token=next_page_token) | |||||
Done Inline Actionslong story short, here is my proposal for this part of the code: def origin_visit_status_get([...]):
next_page_token = None
date_from = None
if page_token is not None:
date_from = datetime.datetime.fromisoformat(page_token)
rows = self._cql_runner.origin_visit_status_get_range(
origin, visit, date_from, limit+1, order
)
visit_statuses = [converters.row_to_visit_status(row) for row in rows[:limit]]
if len(rows) > limit:
next_page_token = str(converters.row_to_visit_status(rows[limit+1].date)
return PagedResult(results=visit_statuses, next_page_token=next_page_token)douardda: long story short, here is my proposal for this part of the code:
```
def… | |||||
Done Inline Actionslet's see if that works ;) ardumont: let's see if that works ;) | |||||
Done Inline ActionsNote: this applies to other storage implementations too, i guess. douardda: Note: this applies to other storage implementations too, i guess. | |||||
Done Inline Actionsif you do that, then you need to change the inequalities in the backend to be non-strict inequalities vlorentz: if you do that, then you need to change the inequalities in the backend to be non-strict… | |||||
Done Inline Actionsoh and your code is incorrect david, because you're returning the limit first results, and use the limit+2's date as token vlorentz: oh and your code is incorrect david, because you're returning the `limit` first results, and… | |||||
Done Inline Actionsyes it's indeed incorrect since I did not get the behavior of _cql_runner.origin_visit_status_get_range() w.r.t the date_from argument. ( I mean calling a function with a date_from argument, I expect to have results starting from this date by default). But as said above, I'm not sure I see the real advantage of gathering limit+1 results from the base (just add the next_page_token iff len(rows) == limit ) douardda: yes it's indeed incorrect since I did not get the behavior of _cql_runner. | |||||
Done Inline ActionsIt does not work for the reasons val exposed.
I think the tests are fine, spent quite some time on it (either here and for origin-visit-get endpoint). In the end, the improvments i can do here is:
Note: ardumont: It does not work for the reasons val exposed.
> either the tests are wrong...
I think the… | |||||
Done Inline Actions
ah yes, that's reasonable.
That saves one call to the api from the user perspective? without the extra limit, i gather, the user needs to make one more call with possibly nothing to retrieve. ardumont: > ( I mean calling a function with a date_from argument, I expect to have results starting… | |||||
Done Inline Actions
I'll check how hard it is to invert the strictness in the backend... ardumont: > ah yes, that's reasonable.
I'll check how hard it is to invert the strictness in the backend. | |||||
Done Inline Actions
and David's argument is that it's not worth the extra complexity + fetching an extra record. Personally I don't care either way. vlorentz: > That saves one call to the api from the user perspective?
and David's argument is that it's… | |||||
Done Inline ActionsYes, but I'm more for the "extra complexity" argument, as you would have guessed ;-) But I don't really care either as long as the code is easy to read and understand (i.e. with comments for traps like the date_from argument) douardda: Yes, but I'm more for the "extra complexity" argument, as you would have guessed ;-) But I… | |||||
Done Inline Actions
I successfully inverted the strictness. (And note that i did not realize it was a trap, otherwise, i would have done something about it...) Though that means we need to exclude the last result from the fetch. We use its date as value for the next-page-token (so it is used!). i'll check the extra comments you made to adapt according before pushing an update. Note: I can remove the extra-limit though and just use limit + 1 etc... ardumont: > (i.e. with comments for traps like the date_from argument)
I successfully inverted the… | |||||
| def origin_visit_find_by_date( | def origin_visit_find_by_date( | ||||
| self, origin: str, visit_date: datetime.datetime | self, origin: str, visit_date: datetime.datetime | ||||
| ) -> Optional[OriginVisit]: | ) -> Optional[OriginVisit]: | ||||
| # Iterator over all the visits of the origin | # Iterator over all the visits of the origin | ||||
| # This should be ok for now, as there aren't too many visits | # This should be ok for now, as there aren't too many visits | ||||
| # per origin. | # per origin. | ||||
| rows = list(self._cql_runner.origin_visit_get_all(origin)) | rows = list(self._cql_runner.origin_visit_get_all(origin)) | ||||
| ▲ Show 20 Lines • Show All 296 Lines • Show Last 20 Lines | |||||
do we really need those? I mean, these are type annotated, so it seems unnecessary to me to add execution time type validation.