Page MenuHomeSoftware Heritage
Differential D4873

journal_client: Improve stats detection
ClosedPublic
Actions

Authored by ardumont on Jan 15 2021, 4:57 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Maniphest Tasks
T2967: Write journal client subcribed to origin_visit_status topics
Commits
rDSCH58ca79622db0: journal_client: Improve stats detection
Summary

This adds an integration test which permutes inputs to ensure out of order renders the
same result.

This also improves the current algorithm which revealed some hit-and-miss cases:

  • Initialization of the first visit detection (through the "last_snapshot" absence field,

the previous implementation check could fail otherwise).

  • out of order policy (ignore old event) in case of supposedly "eventful" event was done too early

which ignored too much messages (those new test cases failed in some permutations).

  • This is now specifically checked in case of referenced snapshots which led to cases of possibly

changing eventful event into uneventful one. For example, the case of an anterior eventful event is
caught which means that the current most-up-to-date eventful event is actually an uneventful one).

  • ...

Related to T2967

Diff Detail

Repository
rDSCH Scheduling utilities
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18459
Build 28541: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 28540: arc lint + arc unit

Event Timeline

ardumont created this revision.Jan 15 2021, 4:57 PM
swh-public-ci added a comment.Jan 15 2021, 4:59 PM

Build is green

Patch application report for D4873 (id=17278)

Rebasing onto a5fb291703...

Current branch diff-target is up to date.
Changes applied before test
commit 2733c357a52c47016733352ec5418360363a98ff
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 15 15:49:41 2021 +0100

    journal_client: Improve stats detection
    
    This adds an integration test which permutes input to ensure out of order renders the
    same result.
    
    This also improves the current algorithm which revealed some hit-and-miss cases.
    
    Related to T2967

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

Harbormaster completed remote builds in B18418: Diff 17278.Jan 15 2021, 4:59 PM
ardumont requested review of this revision.Jan 15 2021, 4:59 PM
ardumont edited the summary of this revision. (Show Details)Jan 18 2021, 9:31 AM
vlorentz added a subscriber: vlorentz.Jan 18 2021, 12:05 PM

Can you be more explicit about what the improvements are, in the commit msg / diff description?

ardumont edited the summary of this revision. (Show Details)Jan 18 2021, 12:33 PM

Can you be more explicit about what the improvements are, in the commit msg / diff description?

Description updated.
I'll amend the commit message when we are fine with such a description ;)

ardumont added a child revision: D4876: scheduler.cli.journal: Add `swh scheduler journal visit-stats` cli.Jan 18 2021, 3:36 PM
ardumont added inline comments.Jan 18 2021, 4:47 PM
swh/scheduler/journal_client.py
88 ↗(On Diff #17278)

this was too dismissive a filter so this conditional was moved in the subsequent conditional with different behavior changes as explained in the diff description.

swh/scheduler/tests/test_journal_client.py
310 ↗(On Diff #17278)

Those tests are not real changes, they were deemed clearer inlined.

olasd accepted this revision.Jan 18 2021, 5:58 PM
olasd added a subscriber: olasd.

The new logic is somewhat elaborate, but it's well documented and seems to be properly tested, so this looks fine to me.

For now there's nothing that I see that would prevent this logic from being implemented in SQL to reduce the amount of database ping-pong, when we're done refining it. Alternatively, or as a stop-gap, we could consider retrieving the "current recorded status" in a single batch for all the messages being processed now (that is, collapsing all the scheduler.origin_visit_stats_get calls into a single call to a new backend endpoint).

swh/scheduler/journal_client.py
87 ↗(On Diff #17278)

This is bikeshed-ish, but I think the logic would be even easier to follow with:

  • date renamed to latest_recorded_visit(_date?)
  • msg_dict["date"] given a more explicit name like current_status_date
101–104 ↗(On Diff #17278)

Good catch!

This revision is now accepted and ready to land.Jan 18 2021, 5:58 PM
ardumont updated this revision to Diff 17320.Jan 19 2021, 9:53 AM

Adapt according to review, rename variables to improve readability

swh-public-ci added a comment.Jan 19 2021, 9:55 AM

Build is green

Patch application report for D4873 (id=17320)

Could not rebase; Attempt merge onto d3afd144af...

Merge made by the 'recursive' strategy.
 mypy.ini                                   |   3 +
 requirements-swh.txt                       |   1 +
 swh/scheduler/cli/journal.py               |  62 +++++++++++++
 swh/scheduler/journal_client.py            |  38 ++++++--
 swh/scheduler/tests/test_cli_journal.py    | 115 ++++++++++++++++++++++++
 swh/scheduler/tests/test_journal_client.py | 139 +++++++++++++++++++++++++++--
 6 files changed, 343 insertions(+), 15 deletions(-)
 create mode 100644 swh/scheduler/cli/journal.py
 create mode 100644 swh/scheduler/tests/test_cli_journal.py
Changes applied before test
commit ca8be2930a95f7bff9fc41d00370858987e5ad02
Merge: d3afd14 9a9db55
Author: Jenkins user <jenkins@localhost>
Date:   Tue Jan 19 08:53:44 2021 +0000

    Merge branch 'diff-target' into HEAD

commit 9a9db555a03e10305c183591e66a6dac5e760a33
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jan 18 14:46:57 2021 +0100

    scheduler.cli.journal: Add `swh scheduler journal visit-stats` cli
    
    This adds the cli entrypoint to actually process origin_visit_status topics and write to
    the origin_visit_stats db table.
    
    Related to T2967

commit dea576ef295210f9d772ff5fda8d44e5202fb5af
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 15 15:49:41 2021 +0100

    journal_client: Improve stats detection
    
    This adds an integration test which permutes input to ensure out of order renders the
    same result.
    
    This also improves the current algorithm which revealed some hit-and-miss cases.
    
    Related to T2967

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

Harbormaster completed remote builds in B18459: Diff 17320.Jan 19 2021, 9:55 AM
ardumont updated this revision to Diff 17321.Jan 19 2021, 10:02 AM

Use the right commit range for the diff update with the previous description

swh-public-ci added a comment.Jan 19 2021, 10:04 AM

Build is green

Patch application report for D4873 (id=17321)

Rebasing onto d3afd144af...

First, rewinding head to replay your work on top of it...
Applying: journal_client: Improve stats detection
Changes applied before test
commit 7b3a64e68e79f5165ceccb96679e07413ebb4d19
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 15 15:49:41 2021 +0100

    journal_client: Improve stats detection
    
    This adds an integration test which permutes input to ensure out of order renders the
    same result.
    
    This also improves the current algorithm which revealed some hit-and-miss cases.
    
    Related to T2967

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

Harbormaster completed remote builds in B18460: Diff 17321.Jan 19 2021, 10:04 AM
ardumont updated this revision to Diff 17322.Jan 19 2021, 10:16 AM

Rebase

ardumont updated this revision to Diff 17323.Jan 19 2021, 10:17 AM

Rework commit message as promised (I almost forgot ¯\_(ツ)_/¯)

swh-public-ci added a comment.Jan 19 2021, 10:18 AM

Build is green

Patch application report for D4873 (id=17322)

Rebasing onto d3afd144af...

Current branch diff-target is up to date.
Changes applied before test
commit c4f12d097ce23ef4cb748cf5412bf5f3d808a58d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 15 15:49:41 2021 +0100

    journal_client: Improve stats detection
    
    This adds an integration test which permutes input to ensure out of order renders the
    same result.
    
    This also improves the current algorithm which revealed some hit-and-miss cases.
    
    Related to T2967

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

Harbormaster completed remote builds in B18461: Diff 17322.Jan 19 2021, 10:18 AM
swh-public-ci added a comment.Jan 19 2021, 10:20 AM

Build is green

Patch application report for D4873 (id=17323)

Rebasing onto d3afd144af...

Current branch diff-target is up to date.
Changes applied before test
commit 58ca79622db077d9a2a897b4671a6013bf70f97d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 15 15:49:41 2021 +0100

    journal_client: Improve stats detection
    
    This adds an integration test which permutes input to ensure out of order renders the
    same result.
    
    This also improves the current algorithm which revealed some hit-and-miss cases:
    
    - Initialization of the first visit detection (through the "last_snapshot" absence
      field, the previous implementation check could fail otherwise).
    
    - out of order policy (ignore old event) in case of supposedly "eventful" event was done
      too early which ignored too much messages (those new test cases failed in some
      permutations). This is now specifically checked in case of referenced snapshots which
      led to cases of possibly changing eventful event into uneventful one. For example, the
      case of an anterior eventful event is caught which means that the current
      most-up-to-date eventful event is actually an uneventful one).
    
    ...
    
    Related to T2967

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

Harbormaster completed remote builds in B18462: Diff 17323.Jan 19 2021, 10:20 AM
Closed by commit rDSCH58ca79622db0: journal_client: Improve stats detection (authored by ardumont). · Explain WhyJan 19 2021, 10:22 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDSCH58ca79622db0: journal_client: Improve stats detection.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
3 lines--
1 line--
swh/
scheduler/
cli/
62 lines92%
Loading...
tests/
115 lines100%
Loading...

Diff 17320

View Options

mypy.ini

Loading...
View Options

requirements-swh.txt

Loading...
View Options

swh/scheduler/cli/journal.py

Loading...
View Options

swh/scheduler/tests/test_cli_journal.py

Loading...
About · Developer information · Legal