Page MenuHomeSoftware Heritage

Rework the retry and reporting system in replay.py
ClosedPublic

Authored by douardda on Nov 26 2021, 1:32 PM.

Details

Summary

split the 'copy_object()' in 2 parts, 'get_object()' and 'put_object()'
and make each of these decorated by '@content_replay_retry' (instead of
the whole 'copy_object()'), so that a failing 'put_object' does not
trigger getting the object from the src objstorage again.

Also, only log in statsd the end result (error or success) instead of
logging each attempt. We don't need these intermediate results in
statsd, and it makes them much harder to use in a dashboard.

Last, for the sake of implementation ease, use function name as
"operation" tag in stastds reports (so "get_object", "put_object" and
"obj_in_storage") instead of "copy" and "in_dst".

Doing so, the 'operation' argument of the ReplayError exception has been
dropped.

Add doctrings and comments in test_cli.py

Small code refactoring in test_cli: make '_fill_objstorage_and_kafka()'
take the actual objstorage as argument, instead of the dict of
objstorages ("src", "dst"). The "dst" objstorage is not used in the
function, and it make the intent of this later clearer.

Diff Detail

Repository
rDOBJSRPL Content replayer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build is green

Patch application report for D6692 (id=24312)

Rebasing onto 720b7af2bf...

Current branch diff-target is up to date.
Changes applied before test
commit cbb915109c82a63d98c4006e19dbc3d2bfa572a5
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 18:02:54 2021 +0100

    Add doctrings and comments in test_cli.py

commit 604e00e30439ba5bd9dce24fd505637da81dd75f
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 17:49:00 2021 +0100

    Rework the retry and reporting system in replay.py
    
    split the 'copy_object()' in 2 parts, 'get_object()' and 'put_object()'
    and make each of these decorated by '@content_replay_retry' (instead of
    the whole 'copy_object()'), so that a failing 'put_object' does not
    trigger getting the object from the src objstorage again.
    
    Also, only log in statsd the end result (error or success) instead of
    logging each attempt. We don't need these intermediate results in
    statsd, and it makes them much harder to use in a dashboard.
    
    Last, for the sake of implementation ease, use function name as
    "operation" tag in stastds reports (so "get_object", "put_object" and
    "obj_in_storage") instead of "copy" and "in_dst".
    
    Doing so, the 'operation' argument of the ReplayError exception has been
    dropped.

commit 002443567791de9ffef1a2b443eb7471d120bebe
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 17:36:27 2021 +0100

    Small code refactoring in test_cli
    
    make '_fill_objstorage_and_kafka()' take the actual objstorage as
    argument, instead of the dict of objstorages ("src", "dst"). The "dst"
    objstorage is not used in the function, and it make the intent of this
    later clearer.

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

This revision is now accepted and ready to land.Nov 26 2021, 3:44 PM
olasd requested changes to this revision.Nov 26 2021, 3:53 PM
olasd added a subscriber: olasd.

You seem to have dropped the CONTENT_BYTES_METRIC ?

This revision now requires changes to proceed.Nov 26 2021, 3:53 PM

Could you make sure that this dashboard https://grafana.softwareheritage.org/d/d3l2oqXWz/s3-object-copy?orgId=1 is not affected (or that its functionality can be replaced easily?)

In D6692#174126, @olasd wrote:

You seem to have dropped the CONTENT_BYTES_METRIC ?

oops

In D6692#174129, @olasd wrote:

Could you make sure that this dashboard https://grafana.softwareheritage.org/d/d3l2oqXWz/s3-object-copy?orgId=1 is not affected (or that its functionality can be replaced easily?)

I think it should be OK, I only modify the way CONTENT_RETRY_METRIC is being used (aka swh_content_replayer_retries_total), and it looks to me this probe is not used in this dashboard.

Add tests for expected statsd reports during a content replay session

Build has FAILED

Patch application report for D6692 (id=24427)

Rebasing onto 720b7af2bf...

Current branch diff-target is up to date.
Changes applied before test
commit ef9d30c009d7e70226d53e52c3a382813e9e33d5
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed Dec 1 17:28:56 2021 +0100

    Add tests for expected statsd reports during a content replay session

commit 002443567791de9ffef1a2b443eb7471d120bebe
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 17:36:27 2021 +0100

    Small code refactoring in test_cli
    
    make '_fill_objstorage_and_kafka()' take the actual objstorage as
    argument, instead of the dict of objstorages ("src", "dst"). The "dst"
    objstorage is not used in the function, and it make the intent of this
    later clearer.

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

Rebase on D6724 and adapt statsd tests to match new statsd probes

In D6692#174126, @olasd wrote:

You seem to have dropped the CONTENT_BYTES_METRIC ?

Et voila! Because of you I had to write D6724...

Build is green

Patch application report for D6692 (id=24429)

Could not rebase; Attempt merge onto 720b7af2bf...

Updating 720b7af..d972c24
Fast-forward
 requirements-swh.txt                         |   2 +-
 swh/objstorage/replayer/replay.py            | 104 ++++++++++++----------
 swh/objstorage/replayer/tests/test_cli.py    | 127 +++++++++++++++++++++------
 swh/objstorage/replayer/tests/test_statsd.py | 127 +++++++++++++++++++++++++++
 4 files changed, 290 insertions(+), 70 deletions(-)
 create mode 100644 swh/objstorage/replayer/tests/test_statsd.py
Changes applied before test
commit d972c2470804d39213e1f922cfc71ce806e83f8a
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 18:02:54 2021 +0100

    Add doctrings and comments in test_cli.py

commit 1703d37ac5888b85b1866a0e1dcd18cb8ef1627a
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 17:49:00 2021 +0100

    Rework the retry and reporting system in replay.py
    
    split the 'copy_object()' in 2 parts, 'get_object()' and 'put_object()'
    and make each of these decorated by '@content_replay_retry' (instead of
    the whole 'copy_object()'), so that a failing 'put_object' does not
    trigger getting the object from the src objstorage again.
    
    Also, only log in statsd the end result (error or success) instead of
    logging each attempt. We don't need these intermediate results in
    statsd, and it makes them much harder to use in a dashboard.
    
    Last, for the sake of implementation ease, use function name as
    "operation" tag in stastds reports (so "get_object", "put_object" and
    "obj_in_storage") instead of "copy" and "in_dst".
    
    Doing so, the 'operation' argument of the ReplayError exception has been
    dropped.

commit ef9d30c009d7e70226d53e52c3a382813e9e33d5
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed Dec 1 17:28:56 2021 +0100

    Add tests for expected statsd reports during a content replay session

commit 002443567791de9ffef1a2b443eb7471d120bebe
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 17:36:27 2021 +0100

    Small code refactoring in test_cli
    
    make '_fill_objstorage_and_kafka()' take the actual objstorage as
    argument, instead of the dict of objstorages ("src", "dst"). The "dst"
    objstorage is not used in the function, and it make the intent of this
    later clearer.

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

Build is green

Patch application report for D6692 (id=24434)

Could not rebase; Attempt merge onto 720b7af2bf...

Updating 720b7af..a7bd6bc
Fast-forward
 swh/objstorage/replayer/replay.py            | 104 ++++++++++++----------
 swh/objstorage/replayer/tests/test_cli.py    | 127 +++++++++++++++++++++------
 swh/objstorage/replayer/tests/test_statsd.py | 119 +++++++++++++++++++++++++
 3 files changed, 281 insertions(+), 69 deletions(-)
 create mode 100644 swh/objstorage/replayer/tests/test_statsd.py
Changes applied before test
commit a7bd6bc4427bf35741c8953751ec1330f91445df
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 18:02:54 2021 +0100

    Add doctrings and comments in test_cli.py

commit 8098798820bb2025f9bfc7c6b2b46e08a818b797
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 17:49:00 2021 +0100

    Rework the retry and reporting system in replay.py
    
    split the 'copy_object()' in 2 parts, 'get_object()' and 'put_object()'
    and make each of these decorated by '@content_replay_retry' (instead of
    the whole 'copy_object()'), so that a failing 'put_object' does not
    trigger getting the object from the src objstorage again.
    
    Also, only log in statsd the end result (error or success) instead of
    logging each attempt. We don't need these intermediate results in
    statsd, and it makes them much harder to use in a dashboard.
    
    Last, for the sake of implementation ease, use function name as
    "operation" tag in stastds reports (so "get_object", "put_object" and
    "obj_in_storage") instead of "copy" and "in_dst".
    
    Doing so, the 'operation' argument of the ReplayError exception has been
    dropped.

commit 1d8ea80c7d01847dba16343508b74a037a1da864
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed Dec 1 17:28:56 2021 +0100

    Add tests for expected statsd reports during a content replay session

commit 002443567791de9ffef1a2b443eb7471d120bebe
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Nov 25 17:36:27 2021 +0100

    Small code refactoring in test_cli
    
    make '_fill_objstorage_and_kafka()' take the actual objstorage as
    argument, instead of the dict of objstorages ("src", "dst"). The "dst"
    objstorage is not used in the function, and it make the intent of this
    later clearer.

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

This revision is now accepted and ready to land.Dec 2 2021, 12:28 PM