Page MenuHomeSoftware Heritage
Differential D3307

loader: Read snapshot out of the last origin visit status
ClosedPublic
Actions

Authored by ardumont on Jun 17 2020, 4:59 PM.

Details

Reviewers
None
Group Reviewers
Reviewers
Maniphest Tasks
T2310: Make origin visits immutable
Commits
rDLDSVN782690bd27d3: loader: Read snapshot out of the last origin visit status
Summary

Preparatory work to drop unused fields in OriginVisit.

Related to T2310

Test Plan

tox

Diff Detail

Event Timeline

ardumont created this revision.Jun 17 2020, 4:59 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJun 17 2020, 4:59 PM
swh-public-ci added a comment.Jun 17 2020, 5:01 PM

Build is green

Patch application report for D3307 (id=11713)

Rebasing onto 2f08b0dfb7...

Current branch diff-target is up to date.
Changes applied before test
commit 475e95b710ce83ee8c02ea0652ccf73f9038e997
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 17:00:03 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

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

Harbormaster completed remote builds in B12922: Diff 11713.Jun 17 2020, 5:01 PM
ardumont added inline comments.Jun 17 2020, 5:07 PM
swh/loader/svn/loader.py
206

rahhh, come on...
(apparently missing test for that part...)

swh/loader/svn/tests/test_loader.py
51

This is already defined in loader-core now... [1]
Unsure if we want to avoid duplication here...

[1] D3305

ardumont mentioned this in T2310: Make origin visits immutable.Jun 17 2020, 5:31 PM
ardumont updated this revision to Diff 11715.Jun 17 2020, 5:40 PM

Align assert_last_visit_ok helper function

swh-public-ci added a comment.Jun 17 2020, 5:41 PM

Build is green

Patch application report for D3307 (id=11715)

Rebasing onto 2f08b0dfb7...

Current branch diff-target is up to date.
Changes applied before test
commit 370f660e10b3f8106d855c2bdeacfc874f52c99f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 17:00:03 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

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

Harbormaster completed remote builds in B12924: Diff 11715.Jun 17 2020, 5:41 PM
ardumont updated this revision to Diff 11717.Jun 17 2020, 5:53 PM

test_loader: Load twice the same repository to try and increase coverage

swh-public-ci added a comment.Jun 17 2020, 5:55 PM

Build is green

Patch application report for D3307 (id=11717)

Rebasing onto 2f08b0dfb7...

Current branch diff-target is up to date.
Changes applied before test
commit 6f504132277f7eadcda44da0a058dcec15f24be2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 17:53:31 2020 +0200

    test_loader: Load twice the same repository

commit 370f660e10b3f8106d855c2bdeacfc874f52c99f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 17:00:03 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

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

Harbormaster completed remote builds in B12926: Diff 11717.Jun 17 2020, 5:55 PM
ardumont added inline comments.Jun 17 2020, 6:11 PM
swh/loader/svn/loader.py
206

The current test scaffolding prevent this from running there.
This needs more work.

ardumont planned changes to this revision.Jun 18 2020, 7:49 AM
ardumont mentioned this in D3313: algos.origin: Open origin-get-latest-visit-status function.Jun 18 2020, 1:45 PM
ardumont mentioned this in D3305: package.loader: Retrieve latest snapshot out of the latest visit status.Jun 19 2020, 3:40 PM
ardumont updated this revision to Diff 11771.Jun 19 2020, 7:18 PM

Drop duplication in helper test function declaration

Note: Tests will fail as a new release of loader-core is needed

swh-public-ci added a comment.Jun 19 2020, 7:19 PM

Build has FAILED

Patch application report for D3307 (id=11771)

Rebasing onto 2f08b0dfb7...

Current branch diff-target is up to date.
Changes applied before test
commit 91787b96bfc9735bf2619d9951252804e2f0560c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 17:00:03 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

Link to build: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/20/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/20/console

Harbormaster failed remote builds in B12981: Diff 11771!Jun 19 2020, 7:19 PM
ardumont added inline comments.Jun 22 2020, 10:03 AM
swh/loader/svn/loader.py
206

tbc, that's for another diff heh.

vlorentz added a subscriber: vlorentz.Jun 22 2020, 10:43 AM

Why set last_visit as an attribute? It's only needed once when getting the snapshot

ardumont added a comment.Jun 22 2020, 11:07 AM

Why set last_visit as an attribute? It's only needed once when getting the snapshot

Because right now (storage wise), as we just created a new visit, there is no
snapshot. Thus, when requiring the latest snapshot of the latest visit, we got
None...

I'm currently reflecting on that issue because this is a current
limitation(/regression?) of the storage api/algo modules.

As discussed on irc, there must be an iteration done somewhere, looking from
last visit up until we find what we are looking for (snapshot here). I'm unsure
yet as to what needs to change though...

api:

  • origin-visit-get-latest (which would now iterate other visit status?)
  • origin-visit-status-get-latest (currently taking the visit id parameter, if we remove it, we might be able to search for more data)

algo:

  • snapshot-get-latest which iterates other visit and then visit status up until it finds something worse?

The last entry sounds like the better location as we keep the api dealing with
their respective model objects. What do you think?

vlorentz added a comment.Jun 22 2020, 11:19 AM
In D3307#81392, @ardumont wrote:

Why set last_visit as an attribute? It's only needed once when getting the snapshot

Because right now (storage wise), as we just created a new visit, there is no
snapshot. Thus, when requiring the latest snapshot of the latest visit, we got
None...

We can get the previous visit + snapshot before creating the new visit

  • snapshot-get-latest which iterates other visit and then visit status up until it finds something worse?

worse than what?

The last entry sounds like the better location as we keep the api dealing with
their respective model objects. What do you think?

yes, all loaders need to do the same visit->status->snapshot thing, so it's good to have a function that does it

ardumont added a comment.Jun 22 2020, 11:23 AM

We can get the previous visit + snapshot before creating the new visit

yes, we could do that also...
I found it less clear than the current separation for some reason.

  • snapshot-get-latest which iterates other visit and then visit status up until it finds something worse?

worse than what?

my bad, i meant something "worth" ("what we are looking for")

The last entry sounds like the better location as we keep the api dealing with
their respective model objects. What do you think?

yes, all loaders need to do the same visit->status->snapshot thing, so it's good to have a function that does it

Yes, I'll do that then ;)

That way that code here could be simplified and look almost the same as before.

ardumont added a comment.Jun 22 2020, 3:01 PM

yes, all loaders need to do the same visit->status->snapshot thing, so it's good to have a function that does it

Yes, I'll do that then ;)
That way that code here could be simplified and look almost the same as before.

Done in D3330

ardumont updated this revision to Diff 11802.Jun 22 2020, 3:28 PM

Rewrite to use the latest swh.storage.algos.snapshot.snapshot_get_latest
implementation

Depends on D3330

swh-public-ci added a comment.Jun 22 2020, 3:29 PM

Build has FAILED

Patch application report for D3307 (id=11802)

Rebasing onto 600b86bbe9...

Current branch diff-target is up to date.
Changes applied before test
commit b117df6be252b8bc811fbee86c812e2b6cb5de3c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 15:16:28 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

Link to build: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/21/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/21/console

Harbormaster failed remote builds in B13015: Diff 11802!Jun 22 2020, 3:29 PM
ardumont edited the summary of this revision. (Show Details)Jun 22 2020, 3:30 PM
ardumont updated this revision to Diff 11803.Jun 22 2020, 3:31 PM

Drop no longer needed self.last_visit

Also tests won't work as storage 0.7.0 is not released yet (whose part needed
is D3330).

swh-public-ci added a comment.Jun 22 2020, 3:32 PM

Build has FAILED

Patch application report for D3307 (id=11803)

Rebasing onto 600b86bbe9...

Current branch diff-target is up to date.
Changes applied before test
commit 3337028ae9e93abf573b5332d3a6883411c1a122
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 15:16:28 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

Link to build: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/22/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/22/console

Harbormaster failed remote builds in B13016: Diff 11803!Jun 22 2020, 3:32 PM
swh-public-ci added a comment.Jun 22 2020, 4:15 PM

Build is green

Patch application report for D3307 (id=11803)

Rebasing onto 600b86bbe9...

Current branch diff-target is up to date.
Changes applied before test
commit 3337028ae9e93abf573b5332d3a6883411c1a122
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 15:16:28 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

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

Harbormaster completed remote builds in B13016: Diff 11803.Jun 22 2020, 4:15 PM
vlorentz added a comment.Jun 22 2020, 4:39 PM

lgtm, but could you add a test?

ardumont added a comment.Jun 22 2020, 4:42 PM

That will actually be difficult... i tried (you can look at the diff history here)

My conclusion is that the test scaffolding here needs to be reworked to allow it...
so i'm inclined towards it but not right now...

olasd added a subscriber: olasd.Jun 23 2020, 11:56 AM
olasd added inline comments.
swh/loader/svn/loader.py
215–216

Will that ever happen now?

ardumont added inline comments.Jun 23 2020, 12:12 PM
swh/loader/svn/loader.py
215–216

I actually have mistyped the method...
For previous_swh_revision, the type should be Optional[Union[bytes, dict]].

Yes, i think that can still happen... That parameter is coming from the constructor and i did not touched that part.

ardumont added inline comments.Jun 23 2020, 12:21 PM
swh/loader/svn/loader.py
215–216

https://forge.softwareheritage.org/source/swh-loader-svn/browse/master/swh/loader/svn/tests/test_loader.py$510-530

(Thus why i want to rework those among other things ;)

ardumont updated this revision to Diff 11829.Jun 23 2020, 12:24 PM

Fix mistyped method

swh-public-ci added a comment.Jun 23 2020, 12:25 PM

Build is green

Patch application report for D3307 (id=11829)

Rebasing onto 600b86bbe9...

Current branch diff-target is up to date.
Changes applied before test
commit 1e332e68755a5ecc06e2e8c739f1caba783fc74c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 15:16:28 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

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

Harbormaster completed remote builds in B13043: Diff 11829.Jun 23 2020, 12:25 PM
ardumont updated this revision to Diff 11832.Jun 23 2020, 2:26 PM

Rework docstring sentence phrasing

swh-public-ci added a comment.Jun 23 2020, 2:27 PM

Build is green

Patch application report for D3307 (id=11832)

Rebasing onto 600b86bbe9...

Current branch diff-target is up to date.
Changes applied before test
commit 4ee911a462619349bf089774926355b14e0997da
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 15:16:28 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

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

Harbormaster completed remote builds in B13047: Diff 11832.Jun 23 2020, 2:27 PM
ardumont updated this revision to Diff 11834.Jun 23 2020, 2:31 PM

Add missing returned type to method

swh-public-ci added a comment.Jun 23 2020, 2:32 PM

Build is green

Patch application report for D3307 (id=11834)

Rebasing onto 600b86bbe9...

Current branch diff-target is up to date.
Changes applied before test
commit 782690bd27d3b78cd039a66b48cba03c3ae5ab54
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 15:16:28 2020 +0200

    loader: Read snapshot out of the last origin visit status
    
    Related to T2310

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

Harbormaster completed remote builds in B13049: Diff 11834.Jun 23 2020, 2:32 PM
ardumont mentioned this in T2462: svn loader: Refactor tests using pytest fixtures.Jun 23 2020, 3:10 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 23 2020, 3:23 PM
Closed by commit rDLDSVN782690bd27d3: loader: Read snapshot out of the last origin visit status (authored by ardumont). · Explain Why
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDSVN782690bd27d3: loader: Read snapshot out of the last origin visit status.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
2 lines--
swh/
loader/
svn/
25 lines77%
Loading...
tests/
200 lines99%
Loading...

Diff 11715

View Options

requirements-swh.txt

Loading...
View Options

swh/loader/svn/loader.py

Loading...
View Options

swh/loader/svn/tests/test_loader.py

Loading...
About · Developer information · Legal