Page MenuHomeSoftware Heritage
Differential D1337

core.loader: Adapt according to latest storage api change
ClosedPublic
Actions

Authored by ardumont on Apr 3 2019, 2:19 PM.

Details

Reviewers
olasd
vlorentz
Group Reviewers
Reviewers
Commits
rDLDBASE3c687f0b1aff: core.loader: Migrate to latest snapshot_add, origin_visit_update api
rDLDBASE0c5d99bb76cf: core.loader: Count only the effectively new objects ingested
rDLDBASE29e3573da8f7: core.loader: Adapt according to latest storage api change
Summary
  • core.loader: Count only the new objects ingested
  • core.loader: Migrate to latest snapshot_add, origin_visit_update api

Build will fail because storage not tagged yet (133 does not exist yet).

Depends on D1329
Depends on D1321

Test Plan

pytest
(tox won't work as swh-storage is not tagged yet)

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Apr 3 2019, 2:19 PM
ardumont edited the summary of this revision. (Show Details)Apr 3 2019, 2:19 PM
ardumont added a project: Core Loader.
ardumont added parent revisions: D1329: Make snapshot_add take an iterable of snapshot., D1231: SvnLoaderFromRemoteDump: Add debug output when svnrdump is running.
ardumont edited the summary of this revision. (Show Details)
ardumont added a parent revision: D1321: swh-storage: Install counter metrics.
swh-public-ci added a comment.Apr 3 2019, 2:20 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/97/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/97/console

Harbormaster failed remote builds in B5037: Diff 4270!Apr 3 2019, 2:20 PM
olasd added a comment.Apr 3 2019, 2:51 PM

I wonder if it wouldn't make sense to just turn the counter attribute into a dict compatible with the output of the storage functions...

vlorentz added inline comments.Apr 3 2019, 2:57 PM
swh/loader/core/loader.py
286

Why this change? I don't think it will play well with a non-remote storage, because they return iterators.

ardumont added inline comments.Apr 3 2019, 3:10 PM
swh/loader/core/loader.py
286

See D1321 which transformed the generator into a list to unify with other _add endpoints.
That simplifies the metrics extraction (counting length).

ardumont added a comment.Apr 3 2019, 3:10 PM
In D1337#28947, @olasd wrote:

I wonder if it wouldn't make sense to just turn the counter attribute into a dict compatible with the output of the storage functions...

Yes, i wonder as well, that's why i opened now that diff ;)
To discuss this.

ardumont updated this revision to Diff 4280.Apr 3 2019, 3:14 PM

snapshot_add now takes a list of snapshot as input

swh-public-ci added a comment.Apr 3 2019, 3:15 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/100/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/100/console

Harbormaster failed remote builds in B5047: Diff 4280!Apr 3 2019, 3:15 PM
ardumont edited the summary of this revision. (Show Details)Apr 3 2019, 3:25 PM
olasd added inline comments.Apr 3 2019, 3:42 PM
swh/loader/core/loader.py
487

That should be snapshot['id']

487

also needs an explicit keyword argument

ardumont updated this revision to Diff 4289.Apr 3 2019, 6:15 PM
ardumont marked 2 inline comments as done.

Fix api call to origin_visit_update api

swh-public-ci added a comment.Apr 3 2019, 6:16 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/101/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/101/console

Harbormaster failed remote builds in B5056: Diff 4289!Apr 3 2019, 6:16 PM
swh-public-ci added a comment.Apr 4 2019, 11:47 AM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/102/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/102/console

swh-public-ci added a comment.Apr 4 2019, 2:43 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/103/ for more details.

Harbormaster completed remote builds in B5056: Diff 4289.Apr 4 2019, 2:43 PM
olasd accepted this revision.Apr 4 2019, 5:23 PM

It feels a little weird to have different counters in the dict and in the logs, but this is after filtering out the existing objects so the difference should be minimal anyway.

In a further step, it'd be nicer to make the loader-core counters compatible with the parsing done in swh.storage, so we can get a breakdown of operations per loader without jumping through too many hoops

swh/loader/core/loader.py
431

I'm not sure this was intended?

This revision is now accepted and ready to land.Apr 4 2019, 5:23 PM
ardumont added inline comments.Apr 5 2019, 9:42 AM
swh/loader/core/loader.py
431

no, it was not ;)

Your 'weird' feeling about the counter discrepancy was shared and i started something.
But then i stopped myself because it's out of scope (first migration) and did not want to go there.

That's a left over!
Thanks for pointing it out.

ardumont updated this revision to Diff 4364.Apr 6 2019, 9:27 AM

Remove badly named counter

swh-public-ci added a comment.Apr 6 2019, 9:29 AM

Build is green
See https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/104/ for more details.

Harbormaster completed remote builds in B5167: Diff 4364.Apr 6 2019, 9:29 AM
ardumont added a comment.Apr 6 2019, 9:30 AM

In a further step, it'd be nicer to make the loader-core counters compatible with the parsing done in swh.storage, so we can get a breakdown of operations per loader without jumping through too many hoops

i have opened T1626 to track this by the way.

Cheers,

Closed by commit rDLDBASE29e3573da8f7: core.loader: Adapt according to latest storage api change (authored by ardumont). · Explain WhyApr 6 2019, 10:07 AM
This revision was automatically updated to reflect the committed changes.
ardumont mentioned this in rDLDBASE1912e96f2b8d: d/control: Bump dependency to latest storage.Apr 6 2019, 10:16 AM

Revision Contents
Changeset List

PathSize
2 lines
swh/
loader/
core/
2 lines

Diff 4365

View Options

requirements-swh.txt

Loading...
View Options

swh/loader/core/loader.py

Loading...
About · Developer information · Legal