Details

Reviewers

Group Reviewers

Maniphest Tasks

T2734: Investigate timeout issues when querying snapshot data

Commits

rDWAPPSc469412a897a: common/archive: Avoid db timeouts in lookup_snapshot_sizes
rDWAPPS3c59879e2fe6: browse/snapshot_context: Put processed snapshot sizes in cache

Summary

When querying all branch aliases in a snapshot, the underlying database query can
timeout as it is not properly indexable. That diff intends to mitigate that issue.

A first commit moves the branches filtering by target type client side to avoid
sending the costly request that might timeout.
Note that even it seems that all branches of a snapshot are iterated, most of
swh snapshots contains a single branch alias named HEAD that will be iterated
first. Branches iteration will stop once all aliases processed so in that case
the iteration will stop quickly.

A first commit removes aliases resolving in the lookup_snapshot_sizes function.
Branch aliases will be resolved only when required for display from now on.

A second commit puts processed snapshot sizes in cache to avoid sending the same
set of database queries each time a page is loaded when browsing the archive
in the context of a specific snapshot.

Closes T2734

Depends on D4369

Diff Detail

Repository

rDWAPPS Web applications

Branch

lookup-snapshot-sizes-optimization

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 16847
Build 25985: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 25984: arc lint + arc unit

Event Timeline

anlambert created this revision.Oct 26 2020, 6:43 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptOct 26 2020, 6:43 PM

Build is green

Patch application report for D4356 (id=15405)

Rebasing onto d1643344e5...

Current branch diff-target is up to date.

Changes applied before test

commit 6118c3723f833f381661aa2f99a64498510812f0
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:34:46 2020 +0100

    browse/snapshot_context: Put processed snapshot sizes in cache
    
    Put processed snapshot sizes in cache to avoid sending the same set of
    database queries each time a page is loaded when browsing the archive
    in the context of a specific snapshot.
    
    Closes T2734

commit a0fa085c27f3c8b680764ad490ffb645c5ab6332
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:26:20 2020 +0100

    common/archive: Avoid db timeouts in lookup_snapshot_sizes
    
    When querying all branch aliases in a snapshot, the underlying database query can
    timeout as it is not properly indexable.
    
    This commits moves the branches filtering by target type client side to avoid
    sending the costly request that might timeout.
    
    Related to T2734

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

Harbormaster completed remote builds in B16634: Diff 15405.Oct 26 2020, 6:51 PM

Wouldn't it make sense to have this in swh.storage.algos instead?

Update: Simplify some code and use default branches_count parameter value in lookup_snapshot

Build is green

Patch application report for D4356 (id=15419)

Rebasing onto d1643344e5...

Current branch diff-target is up to date.

Changes applied before test

commit 15654ede55837df06a8f0c76f72c7e92104b27cf
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:34:46 2020 +0100

    browse/snapshot_context: Put processed snapshot sizes in cache
    
    Put processed snapshot sizes in cache to avoid sending the same set of
    database queries each time a page is loaded when browsing the archive
    in the context of a specific snapshot.
    
    Closes T2734

commit d5b8d76e161cae321737ec080eb33a933d8094e1
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:26:20 2020 +0100

    common/archive: Avoid db timeouts in lookup_snapshot_sizes
    
    When querying all branch aliases in a snapshot, the underlying database query can
    timeout as it is not properly indexable.
    
    This commits moves the branches filtering by target type client side to avoid
    sending the costly request that might timeout.
    
    Related to T2734

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

Harbormaster completed remote builds in B16646: Diff 15419.Oct 27 2020, 11:22 AM

In D4356#108458, @vlorentz wrote:

Wouldn't it make sense to have this in swh.storage.algos instead?

Indeed we could add a snapshot_resolve_aliases function in storage.algos. Plus the code in that diff only handles one level of aliases so it should be improved.

As pointed by @vlorentz, the logic to resolve snapshot branch aliases could be moved in swh.storage.algos.snapshot.

Update:

rebase

move snapshot aliases resolving code in swh.storage.algos.snapshot

add more tests

Jenkins build will fail until D4369 gets landed and swh-storage gets a new tag.

anlambert edited the summary of this revision. (Show Details)Oct 27 2020, 5:22 PM

anlambert added a parent revision: D4369: algos/snapshot: Add function to resolve branch alias to real target.

Build has FAILED

Patch application report for D4356 (id=15456)

Rebasing onto 3ffeb396fa...

Current branch diff-target is up to date.

Changes applied before test

commit f172f3cfc42c64858c529a54df54a9b29e146bc9
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:34:46 2020 +0100

    browse/snapshot_context: Put processed snapshot sizes in cache
    
    Put processed snapshot sizes in cache to avoid sending the same set of
    database queries each time a page is loaded when browsing the archive
    in the context of a specific snapshot.
    
    Closes T2734

commit 806ea48be48437db7e480f92e1cbd7d42f48d10b
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:26:20 2020 +0100

    common/archive: Avoid db timeouts in lookup_snapshot_sizes
    
    When querying all branch aliases in a snapshot, the underlying database query can
    timeout as it is not properly indexable.
    
    This commits moves the branches filtering by target type client side to avoid
    sending the costly request that might timeout.
    
    Related to T2734

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

Harbormaster failed remote builds in B16684: Diff 15456!Oct 27 2020, 5:23 PM

anlambert mentioned this in D4369: algos/snapshot: Add function to resolve branch alias to real target.Nov 2 2020, 4:40 PM

anlambert planned changes to this revision.Nov 2 2020, 6:30 PM

Update:

Do not resolve snapshot aliases in swh.web.common.archive.lookup_snapshot_sizes anymore

Resolve a snapshot alias only when required for display

Update tests as revision / release counters have changed since aliases are no more taken into account

Build is green

Patch application report for D4356 (id=15620)

Rebasing onto 527e7b0a87...

Current branch diff-target is up to date.

Changes applied before test

commit 1d459bef95ab5e59fec7b4236ca21bd7175cac15
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:34:46 2020 +0100

    browse/snapshot_context: Put processed snapshot sizes in cache
    
    Put processed snapshot sizes in cache to avoid sending the same set of
    database queries each time a page is loaded when browsing the archive
    in the context of a specific snapshot.
    
    Closes T2734

commit afbce45f99aba5a7bc09a540d3f5485bb34db214
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:26:20 2020 +0100

    common/archive: Avoid db timeouts in lookup_snapshot_sizes
    
    When querying all branch aliases in a snapshot, the underlying database
    query can timeout as it is not properly indexable.
    
    That commit stops to try resolving aliases when getting snapshot sizes
    and only resolve an alias when required for display.
    
    Related to T2734

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

Harbormaster completed remote builds in B16843: Diff 15620.Nov 3 2020, 6:42 PM

douardda added a subscriber: douardda.Nov 4 2020, 10:10 AM

douardda added inline comments.

swh/web/common/archive.py
1007–1008	minor point: could use a `snapshot_sizes.set_default(target_type, 0)` I guess here, or initialize the dict beforehand: snapshot_sizes = dict.from_keys(("alias", "release", "revision"), 0) snapshot_sizes.update(storage.snapshot_count_branches(snapshot_id_bin)) (not sure it really more readable however :-)
1127	are we 100% sure that `result != None` necessarly means `len(result)>=1`?

anlambert edited the summary of this revision. (Show Details)Nov 4 2020, 1:53 PM

anlambert added inline comments.Nov 4 2020, 1:58 PM

swh/web/common/archive.py
1007–1008	Ack, will update
1127	yes, `snapshot_resolve_alias` returns a tuple with two members in it when the snapshot is valid. This has been properly tested in rDSTO8b1815572c695f7c9751b353f51d507e3fe76e0a.

Update: Address @douardda comments

Build is green

Patch application report for D4356 (id=15624)

Rebasing onto 527e7b0a87...

Current branch diff-target is up to date.

Changes applied before test

commit 25d33a00a0171fab288a6a366b66bf134ca4eba4
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:34:46 2020 +0100

    browse/snapshot_context: Put processed snapshot sizes in cache
    
    Put processed snapshot sizes in cache to avoid sending the same set of
    database queries each time a page is loaded when browsing the archive
    in the context of a specific snapshot.
    
    Closes T2734

commit 907c4760d11224bfb1ddffcabc054b1eb161a197
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:26:20 2020 +0100

    common/archive: Avoid db timeouts in lookup_snapshot_sizes
    
    When querying all branch aliases in a snapshot, the underlying database
    query can timeout as it is not properly indexable.
    
    That commit stops to try resolving aliases when getting snapshot sizes
    and only resolve an alias when required for display.
    
    Related to T2734

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

Harbormaster completed remote builds in B16847: Diff 15624.Nov 4 2020, 2:19 PM

anlambert added a child revision: D4409: browse: Add dedicated icon for snapshot branch alias.Nov 4 2020, 3:48 PM

vlorentz added inline comments.Nov 5 2020, 10:55 AM

swh/web/common/archive.py
1122–1131	I think it's unclear that `result` is a tuple, not a list. What about unpacking instead? eg. `(aliases, target_branch) = result`?

It remains unclear to me how this diff addresses the original timeout problem. I see the beneficial effect of having a lazy loading mechanism and cache to reduce the load, but not to prevent the timeouts to occur in the first place (thus failing to fill the cache).

In D4356#110030, @douardda wrote:

It remains unclear to me how this diff addresses the original timeout problem. I see the beneficial effect of having a lazy loading mechanism and cache to reduce the load, but not to prevent the timeouts to occur in the first place (thus failing to fill the cache).

The diff is a mitigation and only avoids to send the expensive PGSQL query filtering the alias branches in a snapshot.

But you are right, we can still have timeout issues when dealing with a large snapshot, see below with https://github/com/v8/v8 origin (200k branches)

INFO:werkzeug:127.0.0.1 - - [05/Nov/2020 12:02:06] "POST /origin/visit_status/get_latest HTTP/1.1" 200 -
b"           SELECT snapshot_id, name, target, target_type\n           FROM swh_snapshot_get_by_id('\\x8891607122ad552e8c2f4a55cc5b3925f8f6f01c'::bytea, '\\x'::bytea, 1001, NULL :: snapshot_target[])\n        "
DEBUG:swh.core.statsd:Error submitting statsd packet. Dropping the packet and closing the socket.
ERROR:root:canceling statement due to statement timeout
Traceback (most recent call last):
  File "/home/anlambert/.virtualenvs/swh/lib/python3.7/site-packages/flask/app.py", line 1950, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/anlambert/.virtualenvs/swh/lib/python3.7/site-packages/flask/app.py", line 1936, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "<decorator-gen-68>", line 2, in snapshot_get_branches
  File "/home/anlambert/swh/swh-environment/swh-core/swh/core/api/negotiation.py", line 147, in _negotiate
    return f.negotiator(*args, **kwargs)
  File "/home/anlambert/swh/swh-environment/swh-core/swh/core/api/negotiation.py", line 81, in __call__
    result = self.func(*args, **kwargs)
  File "/home/anlambert/swh/swh-environment/swh-core/swh/core/api/__init__.py", line 456, in _f
    return obj_meth(**kw)
  File "/home/anlambert/swh/swh-environment/swh-storage/swh/storage/metrics.py", line 24, in d
    return f(*a, **kw)
  File "/home/anlambert/swh/swh-environment/swh-core/swh/core/db/common.py", line 62, in _meth
    return meth(self, *args, db=db, cur=cur, **kwargs)
  File "/home/anlambert/swh/swh-environment/swh-storage/swh/storage/postgresql/storage.py", line 785, in snapshot_get_branches
    cur=cur,
  File "/home/anlambert/swh/swh-environment/swh-storage/swh/storage/postgresql/db.py", line 273, in snapshot_get_by_id
    cur.execute(query, (snapshot_id, branches_from, branches_count, target_types))
psycopg2.errors.QueryCanceled: canceling statement due to statement timeout

Nevertheless, next queries will not timeout as cache will be filled.

douardda added inline comments.Nov 5 2020, 2:10 PM

swh/web/common/archive.py
1122–1131	I agree this would be better/clearer

Update: Rebase and synchronize with updated snapshot_resolve_alias signature.

Build is green

Patch application report for D4356 (id=15657)

Rebasing onto 8daf7a8fda...

Current branch diff-target is up to date.

Changes applied before test

commit 3c59879e2fe6379a4ebeae4cb4449ce02221d608
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:34:46 2020 +0100

    browse/snapshot_context: Put processed snapshot sizes in cache
    
    Put processed snapshot sizes in cache to avoid sending the same set of
    database queries each time a page is loaded when browsing the archive
    in the context of a specific snapshot.
    
    Closes T2734

commit c469412a897ac980ccab72ba5c6d71b159e6a672
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Oct 26 18:26:20 2020 +0100

    common/archive: Avoid db timeouts in lookup_snapshot_sizes
    
    When querying all branch aliases in a snapshot, the underlying database
    query can timeout as it is not properly indexable.
    
    That commit stops to try resolving aliases when getting snapshot sizes
    and only resolve an alias when required for display.
    
    Related to T2734

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

Harbormaster completed remote builds in B16879: Diff 15657.Nov 5 2020, 2:19 PM

vlorentz accepted this revision.Nov 5 2020, 9:15 PM

This revision is now accepted and ready to land.Nov 5 2020, 9:15 PM

Closed by commit rDWAPPSc469412a897a: common/archive: Avoid db timeouts in lookup_snapshot_sizes (authored by anlambert). · Explain WhyNov 6 2020, 11:26 AM

This revision was automatically updated to reflect the committed changes.

anlambert added a commit: rDWAPPSc469412a897a: common/archive: Avoid db timeouts in lookup_snapshot_sizes.

anlambert added a commit: rDWAPPS3c59879e2fe6: browse/snapshot_context: Put processed snapshot sizes in cache.

common/archive: Avoid db timeouts in lookup_snapshot_sizes
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D4356 (id=15405)

Changes applied before test

Patch application report for D4356 (id=15419)

Changes applied before test

Patch application report for D4356 (id=15456)

Changes applied before test

Patch application report for D4356 (id=15620)

Changes applied before test

Patch application report for D4356 (id=15624)

Changes applied before test

Patch application report for D4356 (id=15657)

Changes applied before test

Revision Contents
Changeset List

Diff 15624

swh/web/browse/snapshot_context.py

swh/web/common/archive.py

swh/web/common/typing.py

swh/web/tests/browse/test_snapshot_context.py

swh/web/tests/browse/views/test_content.py

swh/web/tests/browse/views/test_directory.py

swh/web/tests/browse/views/test_origin.py

swh/web/tests/common/test_archive.py

swh/web/tests/conftest.py

common/archive: Avoid db timeouts in lookup_snapshot_sizesClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D4356 (id=15405)

Changes applied before test

Patch application report for D4356 (id=15419)

Changes applied before test

Patch application report for D4356 (id=15456)

Changes applied before test

Patch application report for D4356 (id=15620)

Changes applied before test

Patch application report for D4356 (id=15624)

Changes applied before test

Patch application report for D4356 (id=15657)

Changes applied before test

Revision ContentsChangeset List

Diff 15624

swh/web/browse/snapshot_context.py

swh/web/common/archive.py

swh/web/common/typing.py

swh/web/tests/browse/test_snapshot_context.py

swh/web/tests/browse/views/test_content.py

swh/web/tests/browse/views/test_directory.py

swh/web/tests/browse/views/test_origin.py

swh/web/tests/common/test_archive.py

swh/web/tests/conftest.py

common/archive: Avoid db timeouts in lookup_snapshot_sizes
ClosedPublic
Actions

Revision Contents
Changeset List