Page MenuHomeSoftware Heritage
Differential D6249

Allow filtering extids per extid_version/extid_type when reading
ClosedPublic
Actions

Authored by ardumont on Sep 13 2021, 5:15 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T3567: storage: Allow extid reading with filter on extid version
Commits
rDSTOa9fde72c4816: Allow filtering extids per extid_version/extid_type when reading
Summary

This impacts both the extid_get_from_extid and extid_get_from_target endpoints.

Whe extid_version/extid_type are not provided, this keeps the existing behavior of
returning all extids matching.

Related to T3567

Test Plan

tox happy:

1087 passed, 37 skipped, 3 xfailed, 9 warnings in 189.22s (0:03:09)

Diff Detail

Repository
rDSTO Storage manager
Branch
filter-extids
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23626
Build 36870: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 36869: arc lint + arc unit

Event Timeline

ardumont created this revision.Sep 13 2021, 5:15 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 13 2021, 5:15 PM
swh-public-ci added a comment.Sep 13 2021, 5:22 PM

Build is green

Patch application report for D6249 (id=22628)

Rebasing onto 8e94afaa0e...

Current branch diff-target is up to date.
Changes applied before test
commit 503cdbac673603e8d3b543f60c10300a385d9409
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    If the version is not provided, this keeps the existing behavior of returning all extids
    matching.
    
    Related to T3567

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

Harbormaster completed remote builds in B23562: Diff 22628.Sep 13 2021, 5:22 PM
ardumont requested review of this revision.Sep 13 2021, 5:22 PM
ardumont planned changes to this revision.Sep 13 2021, 5:46 PM

cassandra implementation for extid_get_from_target needs to be changed to actually allow filtering on both extid_type and extid_version.

ardumont mentioned this in D6240: Use extids to filter out already seen revisions across hg origins.Sep 14 2021, 1:42 PM
ardumont updated this revision to Diff 22666.Sep 14 2021, 6:47 PM
ardumont added subscribers: vlorentz, olasd.

Add extid_get_from_target to filter correctly on extid_type and extid_version.

As far as I understood @vlorentz and @olasd, the index part both in cassandra and
postgres needs some update. It's not done yet.

The cassandra part is fuzzy to me though.

ardumont retitled this revision from Allow filtering extids per extid_version when reading to Allow filtering extids per extid_version/extid_type when reading.Sep 14 2021, 6:50 PM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Sep 14 2021, 6:54 PM

Build has FAILED

Patch application report for D6249 (id=22666)

Rebasing onto 589d20ed64...

First, rewinding head to replay your work on top of it...
Applying: Allow filtering extids per extid_version/extid_type when reading
Changes applied before test
commit e1a46bf93d939b834863b7d263ff0efc6ff4495d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version/extid_type when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    Whe extid_version/extid_type are not provided, this keeps the existing behavior of
    returning all extids matching.
    
    Related to T3567

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

Harbormaster failed remote builds in B23600: Diff 22666!Sep 14 2021, 6:54 PM
ardumont updated this revision to Diff 22667.Sep 14 2021, 7:10 PM

Fix tests hopefully

swh-public-ci added a comment.Sep 14 2021, 7:17 PM

Build has FAILED

Patch application report for D6249 (id=22667)

Rebasing onto 589d20ed64...

First, rewinding head to replay your work on top of it...
Applying: Allow filtering extids per extid_version/extid_type when reading
Changes applied before test
commit 6faab80522982ce96b2cdedd774866fbb7dab62b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version/extid_type when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    Whe extid_version/extid_type are not provided, this keeps the existing behavior of
    returning all extids matching.
    
    Related to T3567

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

Harbormaster failed remote builds in B23601: Diff 22667!Sep 14 2021, 7:17 PM
ardumont updated this revision to Diff 22670.Sep 15 2021, 10:03 AM

Forgot to catch the RemoteException case for the edge cases of the endpoint adaptation

swh-public-ci added a comment.Sep 15 2021, 10:10 AM

Build is green

Patch application report for D6249 (id=22670)

Rebasing onto 589d20ed64...

First, rewinding head to replay your work on top of it...
Applying: Allow filtering extids per extid_version/extid_type when reading
Changes applied before test
commit a81a62bfa5fd19ff63dbb3573d64bbf617e53eb2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version/extid_type when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    Whe extid_version/extid_type are not provided, this keeps the existing behavior of
    returning all extids matching.
    
    Related to T3567

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

Harbormaster completed remote builds in B23603: Diff 22670.Sep 15 2021, 10:10 AM
ardumont added a child revision: D6263: wip: Optimization for cassandra extid reading to avoid an extra index.Sep 15 2021, 11:48 AM
ardumont updated this revision to Diff 22693.Sep 15 2021, 2:25 PM

Drop the extra index and use the optimization started in D6263 (it works now)

Close D6263

ardumont mentioned this in D6263: wip: Optimization for cassandra extid reading to avoid an extra index.Sep 15 2021, 2:26 PM
swh-public-ci added a comment.Sep 15 2021, 2:46 PM

Build was aborted

Patch application report for D6249 (id=22693)

Rebasing onto 589d20ed64...

First, rewinding head to replay your work on top of it...
Applying: Allow filtering extids per extid_version/extid_type when reading
Changes applied before test
commit 9e0600980c24cb63dd14a427279668cd59cbe72b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version/extid_type when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    Whe extid_version/extid_type are not provided, this keeps the existing behavior of
    returning all extids matching.
    
    Related to T3567

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

Harbormaster failed remote builds in B23626: Diff 22693!Sep 15 2021, 2:46 PM
vlorentz requested changes to this revision.Sep 15 2021, 4:03 PM
vlorentz added inline comments.
swh/storage/cassandra/cql.py
1338–1340

actually it does not, so the error is legitimate

You can rephrase the comment like this:

Rows are partitioned by token(extid_type, extid), then ordered (aka. "clustered") by (extid_type, extid, extid_version, ...).
This means that, without knowing the exact extid_type and extid, we need to scan the whole partition; which should be reasonably small.
We can change the schema later if this becomes an issue

swh/storage/in_memory.py
729–755

what about splitting this into two functions (one for each case), to make it more readable?

This revision now requires changes to proceed.Sep 15 2021, 4:03 PM
ardumont marked 2 inline comments as done.Sep 15 2021, 5:15 PM
ardumont added inline comments.
swh/storage/cassandra/cql.py
1338–1340

Thanks that's way better.

swh/storage/in_memory.py
729–755

sure

ardumont updated this revision to Diff 22718.Sep 15 2021, 5:17 PM
ardumont marked 2 inline comments as done.

Adapt according to suggestions.

ardumont edited the test plan for this revision. (Show Details)Sep 15 2021, 5:17 PM
vlorentz accepted this revision.Sep 15 2021, 5:23 PM
This revision is now accepted and ready to land.Sep 15 2021, 5:23 PM
swh-public-ci added a comment.Sep 15 2021, 5:24 PM

Build is green

Patch application report for D6249 (id=22718)

Rebasing onto 589d20ed64...

First, rewinding head to replay your work on top of it...
Applying: Allow filtering extids per extid_version/extid_type when reading
Changes applied before test
commit ba28141eae5fc76553cf4fdec51d1c1f7b72e6be
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version/extid_type when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    Whe extid_version/extid_type are not provided, this keeps the existing behavior of
    returning all extids matching.
    
    Related to T3567

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

Harbormaster completed remote builds in B23651: Diff 22718.Sep 15 2021, 5:24 PM
olasd added inline comments.Sep 15 2021, 5:26 PM
swh/storage/postgresql/db.py
897–905

Please don't mix f-strings and %-formatting

ardumont updated this revision to Diff 22720.Sep 15 2021, 5:31 PM

Don't mix f-string and %-formatted string. I used the f-string instead, clearer to read.

swh/storage/postgresql/db.py
897–905

ack

ardumont updated this revision to Diff 22721.Sep 15 2021, 5:35 PM

Don't mix f-string and %-format strings (i missed one)

swh-public-ci added a comment.Sep 15 2021, 5:42 PM

Build is green

Patch application report for D6249 (id=22721)

Rebasing onto 589d20ed64...

First, rewinding head to replay your work on top of it...
Applying: Allow filtering extids per extid_version/extid_type when reading
Changes applied before test
commit 3c6589d2f0149a7187807c2e583d3cbbe3acb894
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version/extid_type when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    Whe extid_version/extid_type are not provided, this keeps the existing behavior of
    returning all extids matching.
    
    Related to T3567

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

Harbormaster completed remote builds in B23654: Diff 22721.Sep 15 2021, 5:43 PM
swh-public-ci added a comment.Sep 15 2021, 5:52 PM

Build was aborted

Patch application report for D6249 (id=22720)

Rebasing onto 589d20ed64...

First, rewinding head to replay your work on top of it...
Applying: Allow filtering extids per extid_version/extid_type when reading
Changes applied before test
commit 25f3b5e91431682965058f65f4b4ec4042be58f1
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version/extid_type when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    Whe extid_version/extid_type are not provided, this keeps the existing behavior of
    returning all extids matching.
    
    Related to T3567

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

Harbormaster failed remote builds in B23653: Diff 22720!Sep 15 2021, 5:52 PM
ardumont updated this revision to Diff 22724.Sep 15 2021, 6:35 PM

Rebase

swh-public-ci added a comment.Sep 15 2021, 6:42 PM

Build is green

Patch application report for D6249 (id=22724)

Rebasing onto 589d20ed64...

Current branch diff-target is up to date.
Changes applied before test
commit a9fde72c48162a6710add58355f2802f88c907f0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 13 14:25:34 2021 +0200

    Allow filtering extids per extid_version/extid_type when reading
    
    This impacts both the `extid_get_from_extid` and `extid_get_from_target` endpoints.
    
    Whe extid_version/extid_type are not provided, this keeps the existing behavior of
    returning all extids matching.
    
    Related to T3567

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

Harbormaster completed remote builds in B23657: Diff 22724.Sep 15 2021, 6:42 PM
Closed by commit rDSTOa9fde72c4816: Allow filtering extids per extid_version/extid_type when reading (authored by ardumont). · Explain WhySep 15 2021, 6:42 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDSTOa9fde72c4816: Allow filtering extids per extid_version/extid_type when reading.

Revision Contents
Changeset List

PathSize
swh/
storage/
cassandra/
52 lines
36 lines
40 lines
20 lines
postgresql/
33 lines
32 lines
tests/
42 lines

Diff 22693

View Options

swh/storage/cassandra/cql.py

Loading...
View Options

swh/storage/cassandra/storage.py

Loading...
View Options

swh/storage/in_memory.py

Loading...
View Options

swh/storage/interface.py

Loading...
View Options

swh/storage/postgresql/db.py

Loading...
View Options

swh/storage/postgresql/storage.py

Loading...
View Options

swh/storage/tests/storage_tests.py

Loading...
About · Developer information · Legal