Details

Reviewers

Group Reviewers

Maniphest Tasks

T2869: web search: allow to filter by origin type

Commits

rDSEA29c634c69e37: Enable to filter searched origins by visit types
rDSEAd1b86802952b: test_search: Add test for intrinsic metadata update
rDSEA91612192bca2: test_cli: Fix deprecation warnings

Summary

That diff adds support to filter origins by visit types in swh-search.

The following changes have been made:

Add a new visit_types field to elasticsearch document for origin.

Add a new optional visit_types parameter to origin_search method in SearchInterface.

Implement visit types filtering in search backends.

Send origin visit types to elasticsearch when processing origin visits in journal client.

I have tested that I could populate a local elasticsearch instance with those new data
using the following command.

$ swh --log-level DEBUG search -C ~/.config/swh/search.yml journal-client objects -o origin_visit

I used the following configuration file:

search.yml

search:
  cls: elasticsearch
  hosts:
    - http://localhost:9200

journal:
  brokers:
    - kafka1.internal.softwareheritage.org
    - kafka2.internal.softwareheritage.org
    - kafka3.internal.softwareheritage.org
    - kafka4.internal.softwareheritage.org
    
  prefix: swh.journal.objects
  group_id: anlambert.search

Related to T2869

Diff Detail

Repository

rDSEA Archive search

Branch

origin-visit-type-search

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 19183
Build 29745: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 29744: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

anlambert added inline comments.Feb 11 2021, 2:13 PM

swh/search/tests/test_cli.py
22–25	I put that change in a separate commit.

Build is green

Patch application report for D5064 (id=18060)

Rebasing onto 47db624364...

Current branch diff-target is up to date.

Changes applied before test

commit 47d4600e5b0bed96a6f5027e15c5bd92324f5842
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:48:53 2021 +0100

    Enable to filter searched origins by visit type
    
    Add a new visit_type field to elasticsearch document for origin.
    
    Add a new optional visit_type parameter to origin_search method in
    SearchInterface.
    
    Implement visit type filtering in search backends.
    
    Send origin visit type to elasticsearch when processing origin visits
    in journal client.
    
    Related to T2869

commit b7837c4112d5d29c0908c354023702e0ecada0c3
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:59:38 2021 +0100

    test_cli: Fix deprecation warnings

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

Harbormaster completed remote builds in B19172: Diff 18060.Feb 11 2021, 2:14 PM

anlambert requested review of this revision.Feb 11 2021, 2:14 PM

anlambert added inline comments.Feb 11 2021, 2:28 PM

swh/search/interface.py
44	Maybe it will be better to allow the filtering on multiple visit types ?

vlorentz added inline comments.Feb 11 2021, 2:33 PM

swh/search/elasticsearch.py
103	should be `keyword`, it has a very small set of possible values and doesn't need full-text search.
swh/search/interface.py
44	I was about to suggest that too, if it's not too much trouble. (If it it, accept a `List[str]` and error on `len != 1`, so we can easily add it in the future)

It's missing support for origins with multiple visit types. It may be enough to just replace "visit_type": visit["type"], with "visit_type": [visit["type"]], in the journal client

In D5064#127696, @vlorentz wrote:

It's missing support for origins with multiple visit types. It may be enough to just replace "visit_type": visit["type"], with "visit_type": [visit["type"]], in the journal client

Indeed, this should work. TIL that in ES any field can contain zero or more values by default (https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html#types-array-handling).

swh/search/elasticsearch.py
103	ack
swh/search/interface.py
44	This seems pretty easy to implement so go for it.

vlorentz requested changes to this revision.Feb 11 2021, 5:37 PM

This revision now requires changes to proceed.Feb 11 2021, 5:37 PM

Handling multiple visit types was a little bit more subtle that I thought
but I managed to find a good solution I think.

Now a list of visit types for an origin will be maintained without duplicates
in elasticsearch.

When searching origins with multiple visit types, an origin will be returned if
it has any of the requested visit types.

anlambert retitled this revision from Enable to filter searched origins by visit type to Enable to filter searched origins by visit types.Feb 11 2021, 6:14 PM

anlambert edited the summary of this revision. (Show Details)

Build is green

Patch application report for D5064 (id=18075)

Rebasing onto 47db624364...

Current branch diff-target is up to date.

Changes applied before test

commit 391fb080311998a230ec53e465341c768a1e76e8
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:48:53 2021 +0100

    Enable to filter searched origins by visit types
    
    Add a new visit_types field to elasticsearch document for origin.
    
    Add a new optional visit_types parameter to origin_search method in
    SearchInterface.
    
    Implement visit types filtering in search backends, an origin wil be
    returned if it has any of the requested visit types.
    
    Send origin visit types to elasticsearch when processing origin visits
    in journal client.
    
    Related to T2869

commit b7837c4112d5d29c0908c354023702e0ecada0c3
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:59:38 2021 +0100

    test_cli: Fix deprecation warnings

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

Harbormaster completed remote builds in B19183: Diff 18075.Feb 11 2021, 6:15 PM

Remove not needed line in painless script code.

Build is green

Patch application report for D5064 (id=18076)

Rebasing onto 47db624364...

Current branch diff-target is up to date.

Changes applied before test

commit 0147ee7eeee8270bb549730bd70e968ba0568817
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:48:53 2021 +0100

    Enable to filter searched origins by visit types
    
    Add a new visit_types field to elasticsearch document for origin.
    
    Add a new optional visit_types parameter to origin_search method in
    SearchInterface.
    
    Implement visit types filtering in search backends, an origin wil be
    returned if it has any of the requested visit types.
    
    Send origin visit types to elasticsearch when processing origin visits
    in journal client.
    
    Related to T2869

commit b7837c4112d5d29c0908c354023702e0ecada0c3
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:59:38 2021 +0100

    test_cli: Fix deprecation warnings

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

Harbormaster completed remote builds in B19184: Diff 18076.Feb 11 2021, 6:17 PM

It's lame that we need to rewrite the handling of has_visits just so that we can update visit_types, but I guess we don't have a choice :(

swh/search/elasticsearch.py
134–138	IMO that's easier to read
swh/search/tests/test_search.py
130–137	The function name should mention updates (as it's what it's really testing); and there should be a separate test for updating the origins in a separate step (ie. `self.search.origin_update([{"url": "http://foobar.baz", "visit_types": ["git", "svn"]}])` + `self.search.origin_update([{"url": "http://foobar.baz", "visit_types": ["hg"]}])`)
148–168	What does it add to `test_origin_with_multiple_visit_types_search`?

This revision now requires changes to proceed.Feb 12 2021, 10:56 AM

It also misses a test where an origin is initially inserted with visit_types, then visit_types is added

anlambert added inline comments.Feb 12 2021, 11:41 AM

swh/search/elasticsearch.py
134–138	ack, been a a while since I did not write java-ish code.
swh/search/tests/test_search.py
130–137	ack
148–168	It tests combination of visit types when searching but I guess I could merge that test code in the one above.

In D5064#127855, @vlorentz wrote:

It also misses a test where an origin is initially inserted with visit_types, then visit_types is added

and perhaps also one with an origin without a visit types at all, because we will have the case in production where the origin_visit_status contains former visits without the type.

The case can also exists if an origin message is indexed before its visits or during the migration

Update: Address review comments.

Build is green

Patch application report for D5064 (id=18088)

Rebasing onto 47db624364...

Current branch diff-target is up to date.

Changes applied before test

commit efdcf8aca2178f549f1caa575026e816f8c1b609
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:48:53 2021 +0100

    Enable to filter searched origins by visit types
    
    Add a new visit_types field to elasticsearch document for origin.
    
    Add a new optional visit_types parameter to origin_search method in
    SearchInterface.
    
    Implement visit types filtering in search backends, an origin wil be
    returned if it has any of the requested visit types.
    
    Send origin visit types to elasticsearch when processing origin visits
    in journal client.
    
    Related to T2869

commit b7837c4112d5d29c0908c354023702e0ecada0c3
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:59:38 2021 +0100

    test_cli: Fix deprecation warnings

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

Harbormaster completed remote builds in B19196: Diff 18088.Feb 12 2021, 12:06 PM

In D5064#127886, @vsellier wrote:

In D5064#127855, @vlorentz wrote:

It also misses a test where an origin is initially inserted with visit_types, then visit_types is added

and perhaps also one with an origin without a visit types at all

Oh yes indeed, I meant "inserted WITHOUT visit_types" ;)

Intrinsic metadata update must also be handled by the painless script and tested.

vlorentz added a child revision: D5070: Add typing to origin_update's argument and origin_search's return.Feb 12 2021, 1:04 PM

If that's too much trouble, we could do it in two steps. One "painless" for the visit types, and one "doc_as_upsert" for everything else.

It nearly doubles the query time, but it would keep the code simple and less error prone

Update:

Simplify painless script for updating an origin document
Update tests
Add a new test for intrinsic metadata update in a separate commit

Build is green

Patch application report for D5064 (id=18091)

Rebasing onto 47db624364...

Current branch diff-target is up to date.

Changes applied before test

commit 132552f278a149f06ff30071a46fd4f26b88348d
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Fri Feb 12 15:19:14 2021 +0100

    test_search: Add test for intrinsic metadata update

commit 234f9943a29f3aa712a858859a7a9cd3fe12b650
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:48:53 2021 +0100

    Enable to filter searched origins by visit types
    
    Add a new visit_types field to elasticsearch document for origin.
    
    Add a new optional visit_types parameter to origin_search method in
    SearchInterface.
    
    Implement visit types filtering in search backends, an origin wil be
    returned if it has any of the requested visit types.
    
    Send origin visit types to elasticsearch when processing origin visits
    in journal client.
    
    Related to T2869

commit b7837c4112d5d29c0908c354023702e0ecada0c3
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:59:38 2021 +0100

    test_cli: Fix deprecation warnings

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

Harbormaster completed remote builds in B19199: Diff 18091.Feb 12 2021, 3:27 PM

Reorder commits

Build is green

Patch application report for D5064 (id=18092)

Rebasing onto 47db624364...

Current branch diff-target is up to date.

Changes applied before test

commit cc72df622f46fb8e07b9c3e1091f452b8d4b33fe
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:48:53 2021 +0100

    Enable to filter searched origins by visit types
    
    Add a new visit_types field to elasticsearch document for origin.
    
    Add a new optional visit_types parameter to origin_search method in
    SearchInterface.
    
    Implement visit types filtering in search backends, an origin wil be
    returned if it has any of the requested visit types.
    
    Send origin visit types to elasticsearch when processing origin visits
    in journal client.
    
    Related to T2869

commit ebd19ea7a298094c6b1d58381921eaca0ba98ccb
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Fri Feb 12 15:19:14 2021 +0100

    test_search: Add test for intrinsic metadata update

commit b7837c4112d5d29c0908c354023702e0ecada0c3
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:59:38 2021 +0100

    test_cli: Fix deprecation warnings

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

Harbormaster completed remote builds in B19200: Diff 18092.Feb 12 2021, 3:29 PM

In D5064#127911, @vlorentz wrote:

If that's too much trouble, we could do it in two steps. One "painless" for the visit types, and one "doc_as_upsert" for everything else.

It nearly doubles the query time, but it would keep the code simple and less error prone

I managed to update the painless script to avoid sending two queries when visit types need to be updated.

I added a new test updating intrinsic metadata in a separate commit to ensure no regression.

It is missing a test of calling origin_search with a superset of the visits an origin has (ie. what happens when we do origin_update({"url": "foo", "visit_types": ["git"]}) then origin_search(url_pattern="foo", visit_types=["git", "hg"]).

This raises a bigger issue about the semantics of that operation; I can't guess what ES does, but for the in-mem it would return nothing. So the current semantics would be "return origins that have all the given visit types". Is this why we want? If yes, it should be clearly documented in the docstring.

swh/search/elasticsearch.py
134–138	Could you add comments? It's not a bit hard to follow.
swh/search/in_memory.py
43–48	store a set in `document["visit_types"]`, it will make `origin_search` more readable
104–106	Looks like this is equivalent. (but still probably not what we want, see my comment about the semantics of `origin_search`)
swh/search/tests/test_search.py
159–172	I find this to be harder to read than the code being tested. Could you keep it simpler, by removing all the loops and writing the origin types explicitly every time?
165	IMO this is clearer
384–408	why this new test?

This revision now requires changes to proceed.Feb 15 2021, 10:24 AM

This raises a bigger issue about the semantics of that operation; I can't guess what ES does, but for the in-mem it would return nothing.
So the current semantics would be "return origins that have all the given visit types". Is this why we want? If yes, it should be clearly documented in the docstring.

ES will return an origin if it has any of the provided visit types (terms query). That's also the behavior I originally wanted to implement
in the memory backend but I made a mistake: I should have use a set intersection instead of a set difference.

Still some tests to update and I will push a new diff revision.

swh/search/tests/test_search.py
159–172	Ack, will simplify a bit
384–408	As I have modified the way document gets updated in elasticsearch, I added a new commit with that test to ensure no update regression for metadata.

In D5064#128946, @anlambert wrote:

ES will return an origin if it has any of the provided visit types (terms query). That's also the behavior I originally wanted to implement
in the memory backend

Cool!

Rebase and address latest @vlorentz comments.

Build is green

Patch application report for D5064 (id=18242)

Rebasing onto 589ef4d8c4...

Current branch diff-target is up to date.

Changes applied before test

commit bac5fd87f612647174beedbee5965b554466dd94
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:48:53 2021 +0100

    Enable to filter searched origins by visit types
    
    Add a new visit_types field to elasticsearch document for origin.
    
    Add a new optional visit_types parameter to origin_search method in
    SearchInterface.
    
    Implement visit types filtering in search backends, an origin wil be
    returned if it has any of the requested visit types.
    
    Send origin visit types to elasticsearch when processing origin visits
    in journal client.
    
    Related to T2869

commit d1b86802952bc35e795e7e3bb4f8866b7fd0c61c
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Fri Feb 12 15:19:14 2021 +0100

    test_search: Add test for intrinsic metadata update

commit 91612192bca214fb83c52b9c99f7a1beadcd1c91
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:59:38 2021 +0100

    test_cli: Fix deprecation warnings

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

Harbormaster completed remote builds in B19339: Diff 18242.Feb 18 2021, 1:58 PM

Looks good

Two more nitpicks:

swh/search/elasticsearch.py
130–136	for readability
swh/search/tests/test_search.py
153–166	could you split this into two functions? eg. `add_visit_type` and `check_visit_types`.

This revision now requires changes to proceed.Feb 18 2021, 2:10 PM

Handle nitpick comments.

Build is green

Patch application report for D5064 (id=18244)

Rebasing onto 589ef4d8c4...

Current branch diff-target is up to date.

Changes applied before test

commit 29c634c69e37539776e347c4edf9efaf121544ba
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:48:53 2021 +0100

    Enable to filter searched origins by visit types
    
    Add a new visit_types field to elasticsearch document for origin.
    
    Add a new optional visit_types parameter to origin_search method in
    SearchInterface.
    
    Implement visit types filtering in search backends, an origin wil be
    returned if it has any of the requested visit types.
    
    Send origin visit types to elasticsearch when processing origin visits
    in journal client.
    
    Related to T2869

commit d1b86802952bc35e795e7e3bb4f8866b7fd0c61c
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Fri Feb 12 15:19:14 2021 +0100

    test_search: Add test for intrinsic metadata update

commit 91612192bca214fb83c52b9c99f7a1beadcd1c91
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Feb 11 13:59:38 2021 +0100

    test_cli: Fix deprecation warnings

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

Harbormaster completed remote builds in B19341: Diff 18244.Feb 18 2021, 2:34 PM

great!

This revision is now accepted and ready to land.Feb 18 2021, 2:58 PM

Closed by commit rDSEA91612192bca2: test_cli: Fix deprecation warnings (authored by anlambert). · Explain WhyFeb 18 2021, 3:01 PM

This revision was automatically updated to reflect the committed changes.

anlambert added a commit: rDSEA91612192bca2: test_cli: Fix deprecation warnings.

anlambert added a commit: rDSEAd1b86802952b: test_search: Add test for intrinsic metadata update.

anlambert added a commit: rDSEA29c634c69e37: Enable to filter searched origins by visit types.

Enable to filter searched origins by visit types
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D5064 (id=18060)

Changes applied before test

Patch application report for D5064 (id=18075)

Changes applied before test

Patch application report for D5064 (id=18076)

Changes applied before test

Patch application report for D5064 (id=18088)

Changes applied before test

Patch application report for D5064 (id=18091)

Changes applied before test

Patch application report for D5064 (id=18092)

Changes applied before test

Patch application report for D5064 (id=18242)

Changes applied before test

Patch application report for D5064 (id=18244)

Changes applied before test

Revision Contents
Changeset List

Diff 18075

swh/search/elasticsearch.py

swh/search/in_memory.py

swh/search/interface.py

swh/search/journal_client.py

swh/search/tests/test_cli.py

swh/search/tests/test_journal_client.py

swh/search/tests/test_search.py

Enable to filter searched origins by visit typesClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D5064 (id=18060)

Changes applied before test

Patch application report for D5064 (id=18075)

Changes applied before test

Patch application report for D5064 (id=18076)

Changes applied before test

Patch application report for D5064 (id=18088)

Changes applied before test

Patch application report for D5064 (id=18091)

Changes applied before test

Patch application report for D5064 (id=18092)

Changes applied before test

Patch application report for D5064 (id=18242)

Changes applied before test

Patch application report for D5064 (id=18244)

Changes applied before test

Revision ContentsChangeset List

Diff 18075

swh/search/elasticsearch.py

swh/search/in_memory.py

swh/search/interface.py

swh/search/journal_client.py

swh/search/tests/test_cli.py

swh/search/tests/test_journal_client.py

swh/search/tests/test_search.py

Enable to filter searched origins by visit types
ClosedPublic
Actions

Revision Contents
Changeset List