Details

Reviewers

Group Reviewers

Commits

Summary

This will allow to align the loader package's implementations with the
loader-core's regarding retry policy (no retry policy on the package ones).

Also, as a next step, this will allow again to simplify the loader-core's
implementation.

This should help in fixing or at least decreasing the frequency of [1] (which happens for all package loaders, not only cran's).

endpoints:

origin_visit_add
origin_metadata_add
origin_visit_update
directory_add
revision_add
release_add
snapshot_add
tool_add
metadata_provider_add

Note:
The diff is a tad big because of tests.

[1] https://sentry.softwareheritage.org/share/issue/d4f1208b7eec4b43b11e38494ff039cc/

Related to D2511 (which only added content_add)

Test Plan

tox

Diff Detail

Repository

rDSTO Storage manager

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Jan 11 2020, 12:37 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptJan 11 2020, 12:37 PM

ardumont edited the summary of this revision. (Show Details)Jan 11 2020, 12:38 PM

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

Harbormaster completed remote builds in B10044: Diff 8942.Jan 11 2020, 12:40 PM

ardumont edited the summary of this revision. (Show Details)Jan 13 2020, 10:49 AM

from irc discussion [1], it appears, this is not needed.
With the current state of affairs, only the content_add endpoint requires this.
So closing the task

[1]

10:56 <+olasd> are we sure we really want to retry adding all object types? the only object type where we know there are transient failures that are worth retrying are contents. I believe a failure to add anything else is a symptom of a deeper issue, that won't be fixed by retrying three times
11:03 <+pinkieval> I said that too, but ardumont had a compelling answer which I forgot
11:04 <+pinkieval> ah yes, it's that we're already doing the retries in loader-core
11:04 <+pinkieval> (and I assumed there was a reason we were doing this in loader-core in the first place)
11:08 <douardda> olasd: thinking about this, not sure why it could not be the case for other objects than content?
11:09 <douardda> I mean if 2 workers load 2 forks of the same git repo they may very well (attempt to) create the exact same objects
11:10 <douardda> for any entity types other than snapshots and so
11:10 <douardda> even if the collision window for these smaller objects is narrower
11:11 <douardda> how sure are we of this statement "the only object type where we know there are transient failures that are worth retrying are contents"?
11:32 <+olasd> douardda: the addition of the other kinds of objects is properly idempotent as they have a single primary key; contents are special and have multiple unique columns which makes idempotency harder
11:33 <+olasd> postgres knows how to serialize the addition of all other kinds of objects between transactions (because of the single unique primary key), not contents
11:33 <+ardumont> good, that means, i can close 2518 then
11:39 <+olasd> douardda: that's what vlorentz tried to address with D2248 (creating a read dependency to force the serialization of the transactions) but I'm not quite sure of the full implications of it
11:39 -- Notice(swhbot): D2248 (author: vlorentz, Needs Review) on swh-storage: [WIP] In case of race condition in content_add, raise SerializationFailure instead of HashCollision. <https://forge.softwareheritage.org/D2248>

ardumont abandoned this revision.Jan 13 2020, 11:43 AM

Further discussion revealed we need this in the end:

12:00 <+ardumont> > double check that the retry behavior: well, i was going for dropping code, then staging + sentry initially but docker-dev should be enough at first
12:09 <+olasd> ardumont: I think the current retry behavior logs something, doesn't it?
12:10 <+ardumont> ah yes, it logs a warning
12:10 <+olasd> could be worth a poke at kibana
12:10 <+ardumont> i looked-up only retry which is too generic
12:10 <+ardumont> (in kibana)
12:10 <+ardumont> Retry loading a batch
12:10 <+ardumont> is better indeed
12:10 <+olasd> aha
12:18 <+ardumont> it's used, i see integrity errors on directory_add and revision_add (for the person adding step for example)
12:20 <+ardumont> (trying to paste a link is harder than it feels apparently)
12:21 <+ardumont> http://kibana0.internal.softwareheritage.org:5601/goto/8449adafcc24d8b89c3e71a0fa8c9c67
12:25 <+ardumont> so makes 2518 needed after all, at least on revision and directory
12:26 <+ardumont> i guess for the release_add endpoints as well as person can be created through the author field as well
13:07 <+olasd> blerg, ok; I guess we can generalize the retries then
13:08 <+olasd> but we should probably add a random backoff, because right now it looks like they're just failing three times in a row
13:08 <+ardumont> right, i'll check retrying's doc and adapt accordingly
13:08 <+olasd> thanks for checking
13:10 <+olasd> before adding the backoff, we might want to consider tenacity as a retrying replacement, which is more expressive in the way it does retries (and has the good taste of being maintained upstream)
13:11 <+ardumont> ack
13:12 <+ardumont> looking up tenacity
13:13 <+olasd> something like `@retry(stop=stop_after_attempt(5), wait=wait_fixed(2) + wait_random(0, 3))` (in tenacity)
13:13 <+olasd> or wait_random_exponential(multiplier=1, max=20)

Rebase on latest master and:

retry: Migrate to tenacity
retry: Implement content_add_metadata endpoint with retry policy

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

Harbormaster completed remote builds in B10069: Diff 8968.Jan 14 2020, 12:09 PM

ardumont added inline comments.Jan 14 2020, 12:30 PM

swh/storage/retry.py

Could be written as reduce...

r = reduce(
    lambda x, y: x | retry_if_exception_type(y),
    RETRY_EXCEPTIONS[1:],
    retry_if_exception_type(RETRY_EXCEPTIONS[0])
)
retry = r(error)

or even:

for exc in RETRY_EXCEPTIONS:
    if retry_if_exception_type(exc)(error):
        error_name = error.__module__ + '.' + error.__class__.__name__
        logger.warning('Retry adding a batch', exc_info=False, extra={
            'swh_type': 'storage_retry',
            'swh_exception_type': error_name,
            'swh_exception': traceback.format_exc(),
        })
        return True
return False

Simplify should_retry_adding function

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/855/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/855/console

Harbormaster failed remote builds in B10070: Diff 8969!Jan 14 2020, 1:02 PM

Fix typo (auto-indent played game with me and i missed it ;)

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

Harbormaster completed remote builds in B10071: Diff 8970.Jan 14 2020, 1:09 PM

vlorentz requested changes to this revision.Jan 14 2020, 1:20 PM

vlorentz added a subscriber: vlorentz.

vlorentz added inline comments.

swh/storage/in_memory.py
371–372 ↗	(On Diff #8970)	No, `self._contents` items are supposed to always have a ctime.
swh/storage/retry.py
23–24	I don't think it needs such a specific comment. Just "when the server is restarting" is enough.
58–64	To much duplication. my_retry = retry(retry=should_retry_adding, wait=wait_random_exponential(multiplier=1, max=10), stop=stop_after_attempt(3)) class RetryingProxyStorage: # ... @my_retry def content_add(self, content: List[Dict]) -> Dict: # ... @my_retry def content_add_metadata(self, content: List[Dict]) -> Dict: # ...
swh/storage/tests/conftest.py
219	Wrong format. `data.cont3` has a `data` field and is missing a `ctime`.

This revision now requires changes to proceed.Jan 14 2020, 1:20 PM

ardumont added inline comments.Jan 14 2020, 1:20 PM

swh/storage/in_memory.py
372 ↗	(On Diff #8970)	This is for `content_add_metadata` tests. I do not understand: why we remove this in the first place though. or why there is content without `ctime` if it's mandatory...

vlorentz added inline comments.Jan 14 2020, 1:25 PM

swh/storage/in_memory.py
372 ↗	(On Diff #8970)	why we remove this in the first place though. because that's what the postgresql storage did. But it's not a very good reason. or why there is content without ctime if it's mandatory... because the in-mem storage doesn't check it's provided when using `content_metadata_add`

ardumont added inline comments.Jan 14 2020, 1:32 PM

swh/storage/in_memory.py
372 ↗	(On Diff #8970)	because that's what the postgresql storage did. But it's not a very good reason. i meant the initial reason... because the in-mem storage doesn't check it's provided when using content_metadata_add it should be fixed then (in another diff). i checked back (again) the pg schema and there is a not null constraint there. so there discrepancy which is annoying T.T
swh/storage/retry.py
23–24	ack
58–64	indeed i was gonna address it at some point. should have prior to update the diff maybe.