Page MenuHomeSoftware Heritage
Differential D4866

Populate origin_visit_stats table out of the origin_visit_status topic
ClosedPublic
Actions

Authored by ardumont on Jan 14 2021, 5:46 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2967: Write journal client subcribed to origin_visit_status topics
Commits
rDSCH608aa2075619: Populate origin_visit_stats table out of the origin_visit_status topic
Summary

The snapshot is used to determine the "eventful/uneventful" nature of the origin visit
status.

When no snapshot is provided, the visit is considered as failed so the last_failed
column is updated.

As there is no time guarantee when reading message from the topic, the code tries to
keep the data in the most timely ordered as possible. Only most recent information is
kept.

TODO: @vlorentz mentioned in D4852 that the api origin_visit_stats_upsert should
be a batch api. That concern shall be addressed in D4868.

Related to T2967

Test Plan

tox

Diff Detail

Event Timeline

ardumont created this revision.Jan 14 2021, 5:46 PM
ardumont mentioned this in D4852: Start journal client to populate the origin_visit_stats model.Jan 14 2021, 5:47 PM
ardumont mentioned this in D4853: visit_stats: Keep a reference to the last snapshot.
swh-public-ci added a comment.Jan 14 2021, 5:48 PM

Build is green

Patch application report for D4866 (id=17235)

Rebasing onto ca45d40f2a...

Current branch diff-target is up to date.
Changes applied before test
commit 7f4d22a42213378b1e5e85e921c61bfa62be2868
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jan 13 13:03:28 2021 +0100

    Populate origin_visit_stats table out of the origin_visit_status topic
    
    The snapshot is used to determine the "eventful/uneventful" nature of the origin visit
    status.
    
    When no snapshot is provided, the visit is considered as failed so the last_failed
    column is updated.
    
    As there is no time guarantee when reading message from the topic, the code tries to
    keep the data in the most timely ordered as possible. Only most recent information is
    kept.
    
    Related to T2967

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

Harbormaster completed remote builds in B18379: Diff 17235.Jan 14 2021, 5:48 PM
ardumont requested review of this revision.Jan 14 2021, 5:48 PM
ardumont mentioned this in D4859: visit_stats: Update references when per event date is more recent.Jan 14 2021, 5:49 PM
ardumont edited the summary of this revision. (Show Details)
ardumont added a subscriber: vlorentz.
ardumont edited the summary of this revision. (Show Details)Jan 14 2021, 5:56 PM
ardumont added a child revision: D4868: backend: Make origin_visit_stats_upsert a batch api.Jan 15 2021, 8:53 AM
ardumont edited the summary of this revision. (Show Details)Jan 15 2021, 8:57 AM
vlorentz requested changes to this revision.Jan 15 2021, 11:22 AM
vlorentz added inline comments.
sql/updates/19.sql
21
swh/scheduler/backend.py
805

isn't this line redundant?

swh/scheduler/journal_client.py
29–31

Why not swh.model.model objects?

39
46–85

Make last_notfound/last_failed/... local variables, and build OriginVisitStats with explicit arguments at the end, so that mypy can type-check them.

swh/scheduler/tests/test_journal_client.py
26–28

Could you rename them to DATE1, DATE2, and DATE3?

31–32

use the Python, Luke

swh/scheduler/tests/test_scheduler.py
865–867
This revision now requires changes to proceed.Jan 15 2021, 11:22 AM
tenma added a subscriber: tenma.Jan 15 2021, 12:08 PM

incremental review

swh/scheduler/backend.py
805

It seems the conditional is bogus.
Is it ovi.last_eventful > excluded.last_eventful?
But even then there would be redundancy in doing 3 cases on this comparison.

swh/scheduler/journal_client.py
20
22

may seem useless after a raise, but using else/elif clauses may make mypy happy AND be less redundant

ardumont added inline comments.Jan 15 2021, 12:26 PM
swh/scheduler/backend.py
805

as mentioned in the previous diff, that could be collapsed, yes.
It was clearer as an incremental step.

and lol at the bogus conditional, tenma, you are right.

swh/scheduler/journal_client.py
22

it's totally for mypy here...

29–31

Are you implying we can receive model objects here?

It'd be neater.

46–85

next diff about batch upsert modifies this part so i won't touch it yet.

tenma added inline comments.Jan 15 2021, 12:29 PM
swh/scheduler/tests/test_journal_client.py
44
ardumont updated this revision to Diff 17254.Jan 15 2021, 12:45 PM

Adapt according to review
(again, minus the model object remark)

swh-public-ci added a comment.Jan 15 2021, 12:47 PM

Build is green

Patch application report for D4866 (id=17254)

Rebasing onto ca45d40f2a...

Current branch diff-target is up to date.
Changes applied before test
commit a916b4a8d3fbca565a9ec5abead5bdeb0e346845
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jan 13 13:03:28 2021 +0100

    Populate origin_visit_stats table out of the origin_visit_status topic
    
    The snapshot is used to determine the "eventful/uneventful" nature of the origin visit
    status.
    
    When no snapshot is provided, the visit is considered as failed so the last_failed
    column is updated.
    
    As there is no time guarantee when reading message from the topic, the code tries to
    keep the data in the most timely ordered as possible. Only most recent information is
    kept.
    
    Related to T2967

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

Harbormaster completed remote builds in B18395: Diff 17254.Jan 15 2021, 12:47 PM
vlorentz accepted this revision.Jan 15 2021, 2:17 PM
vlorentz added inline comments.
swh/scheduler/journal_client.py
29–31

my bad, we can't

swh/scheduler/tests/test_scheduler.py
865–867

forgot this one

This revision is now accepted and ready to land.Jan 15 2021, 2:17 PM
ardumont added inline comments.Jan 15 2021, 2:27 PM
swh/scheduler/tests/test_scheduler.py
865–867

right.

ardumont updated this revision to Diff 17269.Jan 15 2021, 2:28 PM

Change missing assertion change

swh-public-ci added a comment.Jan 15 2021, 2:31 PM

Build is green

Patch application report for D4866 (id=17269)

Rebasing onto ca45d40f2a...

Current branch diff-target is up to date.
Changes applied before test
commit a916b4a8d3fbca565a9ec5abead5bdeb0e346845
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jan 13 13:03:28 2021 +0100

    Populate origin_visit_stats table out of the origin_visit_status topic
    
    The snapshot is used to determine the "eventful/uneventful" nature of the origin visit
    status.
    
    When no snapshot is provided, the visit is considered as failed so the last_failed
    column is updated.
    
    As there is no time guarantee when reading message from the topic, the code tries to
    keep the data in the most timely ordered as possible. Only most recent information is
    kept.
    
    Related to T2967

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

Harbormaster completed remote builds in B18409: Diff 17269.Jan 15 2021, 2:31 PM
ardumont updated this revision to Diff 17272.Jan 15 2021, 2:34 PM

Actually fix the missing assertion change to a < b < c

swh-public-ci added a comment.Jan 15 2021, 2:39 PM

Build is green

Patch application report for D4866 (id=17272)

Rebasing onto ca45d40f2a...

Current branch diff-target is up to date.
Changes applied before test
commit 608aa2075619bbc9f5c7dab56d1c37a560f13e64
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jan 13 13:03:28 2021 +0100

    Populate origin_visit_stats table out of the origin_visit_status topic
    
    The snapshot is used to determine the "eventful/uneventful" nature of the origin visit
    status.
    
    When no snapshot is provided, the visit is considered as failed so the last_failed
    column is updated.
    
    As there is no time guarantee when reading message from the topic, the code tries to
    keep the data in the most timely ordered as possible. Only most recent information is
    kept.
    
    Related to T2967

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

Harbormaster completed remote builds in B18412: Diff 17272.Jan 15 2021, 2:39 PM
Closed by commit rDSCH608aa2075619: Populate origin_visit_stats table out of the origin_visit_status topic (authored by ardumont). · Explain WhyJan 15 2021, 2:45 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDSCH608aa2075619: Populate origin_visit_stats table out of the origin_visit_status topic.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
sql/
updates/
4 lines--
swh/
scheduler/
46 lines90%
Loading...
89 lines100%
Loading...
10 lines98%
Loading...
sql/
6 lines--
tests/
354 lines100%
Loading...
85 lines99%
Loading...

Diff 17254

View Options

sql/updates/19.sql

Loading...
View Options

swh/scheduler/backend.py

Loading...
View Options

swh/scheduler/journal_client.py

Loading...
View Options

swh/scheduler/model.py

Loading...
View Options

swh/scheduler/sql/30-schema.sql

Loading...
View Options

swh/scheduler/tests/test_journal_client.py

Loading...
View Options

swh/scheduler/tests/test_scheduler.py

Loading...
About · Developer information · Legal