Details

Reviewers

vlorentz

Group Reviewers

Reviewers

Maniphest Tasks

T645: Type swh-storage endpoints with swh.model objects

Commits

rDSTO5344a6ff9c40: storage*: type origin_visit_get_latest endpoint result

Summary

The endpoint returns an optional OriginVisit object instead of a dict:

def origin_visit_get_latest(...) -> Optional[OriginVisit]

It also fixes the in-memory storage implementation which filtered data too
early. It only filtered on the latest origin visit status associated to the
origin visit. So depending on filters, this could have been wrong. It was not
much of a problem as there is no longer any direct clients of this api (they
are using [1] now).

This will allow in a subsequent diff to simplify the function [1] (and reintroduce
the use of this api, albeit indirectly).

[1] swh.storage.algos.origin.origin_get_latest_origin_visit_status function.

Related to T645

Test Plan

tox

Diff Detail

Repository

rDSTO Storage manager

Branch

refactor-algos-origin

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 13971
Build 21441: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 21440: arc lint + arc unit

Event Timeline

ardumont created this revision.Jul 27 2020, 8:16 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptJul 27 2020, 8:16 AM

Build is green

Patch application report for D3620 (id=12730)

Rebasing onto 789972fd5f...

First, rewinding head to replay your work on top of it...
Applying: storage*: type origin_visit_get_latest endpoint result

Changes applied before test

commit 68a7ee55c75a61fea8bc1a8c93b21b3a6f18f61c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Jul 26 08:05:10 2020 +0200

    storage*: type origin_visit_get_latest endpoint result
    
    The endpoint returns an optional OriginVisit object instead of a dict:

def origin_visit_get_latest(...) -> Optional[OriginVisit]
```

Note that it also fixes the in-memory storage implementation which filtered
data too early.

Related to T645

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/560/ for more details.

Harbormaster completed remote builds in B13960: Diff 12730.Jul 27 2020, 8:23 AM

It also fixes the in-memory storage implementation which filtered data too early.

Could you add a test for that?

swh/storage/in_memory.py
931	`visit_statuses`
swh/storage/tests/test_storage.py
1650	why wasn't the rounding needed before?
1998–2010	You could switch the equality operands to make it more readable imo (same comment for other instances below)

This revision now requires changes to proceed.Jul 27 2020, 9:42 AM

Could you add a test for that?

I don't see yet how ;)

swh/storage/in_memory.py
931	It was too long and i was annoyed by it...
swh/storage/tests/test_storage.py
1650	No idea.
1998–2010	I did it that way because it matches most of what we do elsewhere (in other modules or even here i mean). `actual == expected`. Which i now found clearer (force of habits i guess).

In D3620#89000, @ardumont wrote:

Could you add a test for that?

I don't see yet how ;)

like this?

visit = origin_visit_add(origin)
st1 = OriginVisitStatus(origin, visit, date1, status="partial")
st2 = OriginVisitStatus(origin, visit, date2, status="full")
origin_visit_status_add(st1)
origin_visit_status_add(st2)

assert st2 == origin_visit_get_latest(allowed_statuses=["partial"])

swh/storage/in_memory.py
931	I get that, but a abbreviations harm readability
swh/storage/tests/test_storage.py
1998–2010	Then could you add temporary variables (eg. with names containing `actual` and `expected`) and compare them?

ardumont added inline comments.Jul 27 2020, 10:03 AM

swh/storage/tests/test_storage.py
1650	To be clear, that's been bothering me for a while... I have no idea why it's kinda not needed anywhere near we deal with date... Pondering whether we should not make the `swh.storage.utils.now` do that rounding itself... Also, I don't really know what that rounding in itself entails for the day we really switch to cass instead of pg... (in terms of data). Because as far as i understood this, when we'll read from cass, we'll lose that rounded data right? (That's my current thinking...)

vlorentz added inline comments.Jul 27 2020, 10:05 AM

swh/storage/tests/test_storage.py
1650	I have no idea why it's kinda not needed anywhere near we deal with date... It's only needed when doing equality comparisons. Because as far as i understood this, when we'll read from cass, we'll lose that rounded data right? Yes, but it is not useful data.

ardumont added inline comments.Jul 27 2020, 10:06 AM

swh/storage/tests/test_storage.py
1998–2010	That's indeed specifically how it's usually done. I thought the indirection was unneeded here as we do it a lot... And the original code did not bother when it came to compare with None values. So I thought, align everything on that pattern... The test is also too big to give too much lines... Note: The `actual` value is the one we read. The `expected` one is the one we build to check that actual one. So in that front, i though read and check in one line was enough...

vlorentz added inline comments.Jul 27 2020, 10:10 AM

swh/storage/tests/test_storage.py

1998–2010

Usually it is, but Black abuses parentheses here. Compare:

assert (
    swh_storage.origin_visit_status_get_latest(
        origin.url, ov1.visit, require_snapshot=True
    )
    == visit_status_with_snapshot
)

with:

actual_visit_status = swh_storage.origin_visit_status_get_latest(
    origin.url, ov1.visit, require_snapshot=True
)
assert actual_visit_status == visit_status_with_snapshot

ardumont added inline comments.Jul 27 2020, 10:11 AM

swh/storage/in_memory.py
931	oh don't worry, i'm gonna change it. You fine about the implementation otherwise? I tried to keep as much as possible the existing implementation pattern. I initially sorted the visit_statuses as well (order by date desc) and kept the list comprehension filterings as here. And then just returned the first visit_status of the list (since it should then be the max). I changed it because the sorted stanza is a bit hard to read (due to the annoying copy...).

ardumont added inline comments.Jul 27 2020, 10:15 AM

swh/storage/tests/test_storage.py
1650	Yes, but it is not useful data. How so? Isn't say, revision for example, concerned by this (fields `committer_date`, `date`)? If people wants to retrieve revision data and compute the hashes themselves, won't that impact them?
1998–2010	you convinced me, thanks ;)

vlorentz added inline comments.Jul 27 2020, 10:18 AM

swh/storage/in_memory.py
931	yes, it's fine.
931	Actually, why do we need deepcopies? Aren't they immutable objects now?
swh/storage/tests/test_storage.py
1650	These are stored in a TimestampWithTimezone, which preserves exact dates (by storing them as integers in the DB), including microseconds and negative UTC. For visits we don't need exact dates, so we use datetime.datetime (and store them as the native timestamp type of the DB)

ardumont added inline comments.Jul 27 2020, 10:47 AM

swh/storage/in_memory.py
931	afaiui, in-memory's storage implementation regarding its data are lists, aren't they?

ardumont added inline comments.Jul 27 2020, 10:51 AM

swh/storage/in_memory.py
931	self._origin_visits = {} self._origin_visit_statuses: Dict[Tuple[str, int], List[OriginVisitStatus]] = {}
swh/storage/tests/test_storage.py
1650	oh right, thank you ;)

ardumont added inline comments.Jul 27 2020, 10:53 AM

swh/storage/in_memory.py
931	but yes for origin_visit_statuses here, it's unneeded (left-over from the other implementation i told you about ;)

vlorentz added inline comments.Jul 27 2020, 10:57 AM

swh/storage/in_memory.py
931	It's also not needed for visits, because we don't modify the list nor return it

ardumont marked 4 inline comments as done.Jul 27 2020, 10:58 AM

ardumont added inline comments.

swh/storage/in_memory.py
931	we sort it, don't that impact it?

vlorentz added inline comments.Jul 27 2020, 11:00 AM

swh/storage/in_memory.py
931	You're confusing `sorted()` with `.sort()`. The former makes a shallow copy, the latter sorts in-place.

ardumont added inline comments.Jul 27 2020, 11:04 AM

swh/storage/in_memory.py
931	indeed, thanks!

Adapt according to discussions:

rename variable visit_sts to visit_statuses
Remove unneeded copy.deepcopy instructions
Use temporary variables to retrieve visit and visit_status to compare with expected data in tests
Add another test to add coverage

Build is green

Patch application report for D3620 (id=12737)

Rebasing onto 789972fd5f...

First, rewinding head to replay your work on top of it...
Applying: storage*: type origin_visit_get_latest endpoint result

Changes applied before test

commit e1caad7be2d04407f5b366afc0ff771872e0a3a4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Jul 26 08:05:10 2020 +0200

    storage*: type origin_visit_get_latest endpoint result
    
    The endpoint returns an optional OriginVisit object instead of a dict:

def origin_visit_get_latest(...) -> Optional[OriginVisit]
```

Note that it also fixes the in-memory storage implementation which filtered
data too early.

Related to T645

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/562/ for more details.

Harbormaster completed remote builds in B13967: Diff 12737.Jul 27 2020, 11:37 AM

Unify format on some missing part (origin-visit-status-add test)
rework commit message to unify with diff description

vlorentz accepted this revision.Jul 27 2020, 11:43 AM

This revision is now accepted and ready to land.Jul 27 2020, 11:43 AM

vlorentz added inline comments.Jul 27 2020, 11:46 AM

swh/storage/tests/test_storage.py
1650	Oh actually, the answer to my original question: it wasn't needed before because we only compared some fields, so date truncation didn't matter.

Build is green

Patch application report for D3620 (id=12738)

Rebasing onto 789972fd5f...

First, rewinding head to replay your work on top of it...
Applying: storage*: type origin_visit_get_latest endpoint result

Changes applied before test

commit 1f1d23f97364f850e1056f25f389b1f9fbc15e52
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Jul 26 08:05:10 2020 +0200

    storage*: type origin_visit_get_latest endpoint result
    
    The endpoint returns an optional OriginVisit object instead of a dict:

def origin_visit_get_latest(...) -> Optional[OriginVisit]
```

It also fixes the in-memory storage implementation which filtered data too
early. It only filtered on the latest origin visit status associated to the
origin visit. So depending on filters, this could have been wrong. It was not
much of a problem as there is no longer any direct clients of this api (they
are using [1] now).

[1] swh.storage.algos.origin.origin_get_latest_origin_visit_status function

Related to T645

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/563/ for more details.

Harbormaster completed remote builds in B13968: Diff 12738.Jul 27 2020, 11:48 AM

ardumont added inline comments.Jul 27 2020, 11:51 AM

swh/storage/tests/test_storage.py
1650	oh, yeah, cool then.

Rebase

Build is green

Patch application report for D3620 (id=12741)

Rebasing onto 789972fd5f...

Current branch diff-target is up to date.

Changes applied before test

commit 5344a6ff9c40290e670de9fd0d71c0b84185e4fa
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Jul 26 08:05:10 2020 +0200

    storage*: type origin_visit_get_latest endpoint result
    
    The endpoint returns an optional OriginVisit object instead of a dict:

def origin_visit_get_latest(...) -> Optional[OriginVisit]
```

It also fixes the in-memory storage implementation which filtered data too
early. It only filtered on the latest origin visit status associated to the
origin visit. So depending on filters, this could have been wrong. It was not
much of a problem as there is no longer any direct clients of this api (they
are using [1] now).

[1] swh.storage.algos.origin.origin_get_latest_origin_visit_status function

Related to T645

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/565/ for more details.

Harbormaster completed remote builds in B13971: Diff 12741.Jul 27 2020, 12:03 PM

Closed by commit rDSTO5344a6ff9c40: storage*: type origin_visit_get_latest endpoint result (authored by ardumont). · Explain WhyJul 27 2020, 12:13 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDSTO5344a6ff9c40: storage*: type origin_visit_get_latest endpoint result.

ardumont mentioned this in D3628: in_memory: fix tie-breaking when two visits have the same date..Jul 28 2020, 9:43 AM

storage*: type origin_visit_get_latest endpoint result
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D3620 (id=12730)

Changes applied before test

Patch application report for D3620 (id=12737)

Changes applied before test

Patch application report for D3620 (id=12738)

Changes applied before test

Patch application report for D3620 (id=12741)

Changes applied before test

Revision Contents
Changeset List

Diff 12741

swh/storage/cassandra/storage.py

swh/storage/in_memory.py

swh/storage/interface.py

swh/storage/storage.py

swh/storage/tests/algos/test_snapshot.py

swh/storage/tests/test_storage.py

storage*: type origin_visit_get_latest endpoint resultClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D3620 (id=12730)

Changes applied before test

Patch application report for D3620 (id=12737)

Changes applied before test

Patch application report for D3620 (id=12738)

Changes applied before test

Patch application report for D3620 (id=12741)

Changes applied before test

Revision ContentsChangeset List

Diff 12741

swh/storage/cassandra/storage.py

swh/storage/in_memory.py

swh/storage/interface.py

swh/storage/storage.py

swh/storage/tests/algos/test_snapshot.py

swh/storage/tests/test_storage.py

storage*: type origin_visit_get_latest endpoint result
ClosedPublic
Actions

Revision Contents
Changeset List