Details

Reviewers

Group Reviewers

Commits

rDCIDXdb2a631da27f: proper pagination for IndexerStorage.origin_intrinsic_metadata_search_by_produc…

Summary

replace the 'start' argument by a 'page_token' which is now expected to be an (not
so) opaque token used for pagination.

The 'end' argument is dropped.

Adapt tests and the list_origins_by_producer() helper function (in cli.py)
accordingly.

Diff Detail

Repository

rDCIDX Metadata indexer

Branch

pytest

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 8903
Build 12999: tox-on-jenkins	Jenkins
Build 12998: arc lint + arc unit

Event Timeline

douardda created this revision.Nov 5 2019, 4:08 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptNov 5 2019, 4:08 PM

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

Harbormaster completed remote builds in B8825: Diff 7658.Nov 5 2019, 4:11 PM

I would rather we move to opaque tokens for all pagination stuff.

Using last this way will not work if we move the idx-storage to a non-postgresql storage, and I don't want to go through this again.

swh/indexer/cli.py
157	lol I missed this

This revision now requires changes to proceed.Nov 6 2019, 1:58 PM

douardda added a child revision: D2228: tests: migrate indexer storage tests to pytest.Nov 6 2019, 5:49 PM

In D2222#52362, @vlorentz wrote:

I would rather we move to opaque tokens for all pagination stuff.

That's why I put a 'Note' in the Diff's description ;-)

Using last this way will not work if we move the idx-storage to a non-postgresql storage, and I don't want to go through this again.

I know. Let's have this "specify pagination" meeting ASAP.

In D2222#52502, @douardda wrote:

In D2222#52362, @vlorentz wrote:

I would rather we move to opaque tokens for all pagination stuff.

That's why I put a 'Note' in the Diff's description ;-)

Well not exactly exactly, but meh.

use 'page_token' as (opaque) pagination argument

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

Harbormaster completed remote builds in B8870: Diff 7701.Nov 7 2019, 4:22 PM

vlorentz requested changes to this revision.Nov 7 2019, 5:19 PM

vlorentz added inline comments.

swh/indexer/cli.py
152–153	I don't think this condition is needed.
swh/indexer/tests/storage/test_storage.py
1558–1587	Either test the value of `next_page_token`, or use its value for the next call. I prefer the latter because it actually treats the token as opaque.

This revision now requires changes to proceed.Nov 7 2019, 5:19 PM

douardda edited the summary of this revision. (Show Details)Nov 7 2019, 5:31 PM

douardda added inline comments.

swh/indexer/cli.py
152–153	yes it is
swh/indexer/tests/storage/test_storage.py
1558–1587	That's a refactoring to include in another 'make opaque token really opaque' diff.

vlorentz added inline comments.Nov 7 2019, 6:25 PM

swh/indexer/cli.py
152–153	Why? If there are no origin, then `next_page_token` is None. (and was None at the previous iteration, if there are any results)
swh/indexer/tests/storage/test_storage.py
1558–1587	Then you should test its value