⚙ D2966 storage.buffer: Add a new clear_buffers operation for the buffer proxy

ardumont created this revision.Apr 7 2020, 2:57 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptApr 7 2020, 2:57 PM

Build is green

Patch application report for D2966 (id=10553)

Rebasing onto 82b41bac01...

Current branch diff-target is up to date.

Changes applied before test

commit 18b38aec91d3bd281a4e1a9d3eb44e2845644d29
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage.buffer: Add a new clear operation for the buffer proxy
    
    Related to T2352

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

Harbormaster completed remote builds in B11641: Diff 10553.Apr 7 2020, 3:02 PM

This is missing tests for directories and snapshots :)

To make things clearer, I think a few things can be changed:

the method should be renamed clear_buffers (we don't want people to think calling that method will wipe their storage!);
the docstring should have a preeminent warning that data that has not been flushed to storage is lost when this method is called; this should only be called when flush() fails and you want to continue your processing;
the method should be implemented as a pass-through for other filters (i.e. have a recursive call to self.storage.clear_buffers()) and be a noop for all backends (so we don't need to poke around checking whether the current storage really implements it or not).

But overall, I think this will be very useful!

swh/storage/tests/test_buffer.py
324–328	you can probably add a lower bound `0 < len(XXX)` too (i.e. check that we're not testing on empty lists...)

This revision now requires changes to proceed.Apr 7 2020, 3:11 PM

This is missing tests for directories and snapshots :)

directories and skipped_contents!

I thought it was enough to be partial about it but ok, adding them then ;)

the method should be renamed clear_buffers (we don't want people to think
calling that method will wipe their storage!);

done

the docstring should have a preeminent warning that data that has not been
flushed to storage is lost when this method is called; this should only be
called when flush() fails and you want to continue your processing;

indeed

the method should be implemented as a pass-through for other filters (i.e.
have a recursive call to self.storage.clear_buffers()) and be a noop for all
backends (so we don't need to poke around checking whether the current
storage really implements it or not).

I guess you meant the actual "real" backend (pg-storage, cassandra, in-memory).
Ok, then updating them also.

Rename method clear to clear_buffers
Add clear_buffers on all storage endpoints, passthrough for proxy storage, noop endpoint for main backend implementations
Explicit the warning about losing data when calling the clear_buffers endpoint
complete tests with directories and skipped_contents

ardumont added inline comments.Apr 7 2020, 3:45 PM

swh/storage/retry.py

183

Wondering if that'd make sense to define it as:

def clear_buffers(
        self, object_types: Optional[Iterable[str]] = None) -> None:
    """Clear objects from current filter

    """
    if object_types is None:
        object_types = self.object_types

    for object_type in object_types:
        self.object_seen[object_type] = set()

    if hasattr(self.storage, 'clear_buffers'):
        return self.storage.clear_buffers(object_types)
    return None

That's not quite the same meaning, isn't it?

Build is green

Patch application report for D2966 (id=10554)

Rebasing onto 82b41bac01...

Current branch diff-target is up to date.

Changes applied before test

commit 3f3925c66bd80ee25a459610b2974a332d43195d
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage*: Add `clear_buffers` operation for proxy storages
    
    This also adds the endpoint as noop for the main backend implementations.
    
    Related to T2352

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

Harbormaster completed remote builds in B11642: Diff 10554.Apr 7 2020, 3:45 PM

If you add clear_buffers to the basic signature of Storage, I think you can skip all the hasattrs.

swh/storage/buffer.py
150	You're missing the recursive call here.
swh/storage/retry.py
183	Put your comment in the wrong place? And yes, I think the `filter.py`'s `clear_buffers` should do that, or the contents will grow out of bounds.

ardumont added inline comments.Apr 7 2020, 3:52 PM

swh/storage/buffer.py
150	ah yeah, thx. i saw it after proposing the filter one (where i added it ;) i was thinking about adding it but got lost in thought ;)
swh/storage/retry.py
183	mmmph, yeah wrong proxy ;)

If you add clear_buffers to the basic signature of Storage, I think you can skip all the hasattrs.

oh right!

Remove the check on hasattr as all storage implementations have the clear_buffers
Add the clear_buffers implementations on filter proxy storage (+ tests)

Update test-filter docstring

Build is green

Patch application report for D2966 (id=10557)

Rebasing onto 82b41bac01...

Current branch diff-target is up to date.

Changes applied before test

commit ce392bd5db5da1af6f4c718cc7a5cda1824d82bb
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage*: Add `clear_buffers` operation for proxy storages
    
    This also adds the endpoint as noop for the main backend implementations.
    
    Related to T2352

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

Harbormaster completed remote builds in B11645: Diff 10557.Apr 7 2020, 4:23 PM

Build is green

Patch application report for D2966 (id=10558)

Rebasing onto 82b41bac01...

Current branch diff-target is up to date.

Changes applied before test

commit cd33dd6d46a28d99a8c05358ebab8ae4c6299f63
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage*: Add `clear_buffers` operation for proxy storages
    
    This also adds the endpoint as noop for the main backend implementations.
    
    Related to T2352

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

Harbormaster completed remote builds in B11646: Diff 10558.Apr 7 2020, 4:30 PM

ardumont mentioned this in D2973: package.loader: Insert consistently data to storage, clear buffer proxy state in case of error when skipping artifact.Apr 7 2020, 5:33 PM

The get_storage_with_filter_config refactoring could have been a new storage fixture (local to this module) instead (and it should be in a separate commit).

You really need to add the clear_buffers method to interface.py as well. The method needs to be available on the rpc client/server, because we're recursively calling it on all backends.

You should add a basic "call clear_buffers" test to the main storage test suite too, that way we make sure that the method works on all backends.

This revision now requires changes to proceed.Apr 8 2020, 11:15 AM

The get_storage_with_filter_config refactoring could have been a new storage fixture (local to this module) instead

i aligned with the filter one but ok.

(and it should be in a separate commit).

ok

You really need to add the clear_buffers method to interface.py as well. The method needs to be available on the rpc client/server, because we're recursively calling it on all backends.

Thanks, i was unsure about that.

You should add a basic "call clear_buffers" test to the main storage test suite too, that way we make sure that the method works on all backends.

right.

storage*: Add clear_buffers operation for proxy storages (including interface, rpc/server)
test_filter: Extract the filter storage into a fixture

Build is green

Patch application report for D2966 (id=10574)

Rebasing onto 8e8577e8d4...

Current branch diff-target is up to date.

Changes applied before test

commit efe4c02cbb2d495faa5c5f64cde291fd32ddfef8
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 11:27:30 2020 +0200

    test_filter: Extract the filter storage into a fixture

commit f755f5a2e39dfd7215d6704ba570d2c00e6709d9
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage*: Add `clear_buffers` operation for proxy storages
    
    This also adds the endpoint as noop for the main backend implementations.
    
    Related to T2352

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

Harbormaster completed remote builds in B11662: Diff 10574.Apr 8 2020, 11:39 AM

ardumont retitled this revision from storage.buffer: Add a new clear operation for the buffer proxy to storage.buffer: Add a new clear_buffers operation for the buffer proxy.Apr 8 2020, 2:00 PM

Heads up.

This needs a rebase.
Which i keep pending for now as the current master is red (due to flake8 issues).
The storage will get blackified which should take care of those.

ardumont mentioned this in D2980: storage*: Add flush endpoints to storage implems (backend, proxy).Apr 8 2020, 3:28 PM

Rebase on latest master and blackify

Build is green

Patch application report for D2966 (id=10594)

Rebasing onto cd52a03bee...

Current branch diff-target is up to date.

Changes applied before test

commit 8d7909582115255d2d44bcc4805154d2de21ef82
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 11:27:30 2020 +0200

    test_filter: Extract the filter storage into a fixture

commit e0349fcf44abccfb3690a80a83a5ba6f70b2f3e8
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage*: Add `clear_buffers` operation for proxy storages
    
    This also adds the endpoint as noop for the main backend implementations.
    
    Related to T2352

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

Harbormaster completed remote builds in B11683: Diff 10594.Apr 8 2020, 3:48 PM

Rebase on latest master

interface: Improve docstring sentence

Build is green

Patch application report for D2966 (id=10614)

Rebasing onto ed4097c761...

Current branch diff-target is up to date.

Changes applied before test

commit cf152edb21ca8f6ba952141aa3e3afeb7ed437b6
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 11:27:30 2020 +0200

    test_filter: Extract the filter storage into a fixture

commit 6be5c035175b5d9a4b4d2a0db86c185285d0088b
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage*: Add `clear_buffers` operation for proxy storages
    
    This also adds the endpoint as noop for the main backend implementations.
    
    Related to T2352

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

Harbormaster completed remote builds in B11736: Diff 10614.Apr 9 2020, 9:38 AM

Build has FAILED

Patch application report for D2966 (id=10616)

Rebasing onto ed4097c761...

Current branch diff-target is up to date.

Changes applied before test

commit b0b0313c9645052822800197e15009690e43837a
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 11:27:30 2020 +0200

    test_filter: Extract the filter storage into a fixture

commit 566c325cce881a3f73c63a39ac1128db638d4bdb
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage*: Add `clear_buffers` operation for proxy storages
    
    This also adds the endpoint as noop for the main backend implementations.
    
    Related to T2352

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

Harbormaster failed remote builds in B11738: Diff 10616!Apr 9 2020, 9:49 AM

Build is green

Patch application report for D2966 (id=10616)

Rebasing onto ed4097c761...

Current branch diff-target is up to date.

Changes applied before test

commit b0b0313c9645052822800197e15009690e43837a
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 11:27:30 2020 +0200

    test_filter: Extract the filter storage into a fixture

commit 566c325cce881a3f73c63a39ac1128db638d4bdb
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 7 14:56:46 2020 +0200

    storage*: Add `clear_buffers` operation for proxy storages
    
    This also adds the endpoint as noop for the main backend implementations.
    
    Related to T2352

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

Harbormaster completed remote builds in B11738: Diff 10616.Apr 9 2020, 9:55 AM

olasd accepted this revision.Apr 9 2020, 11:03 AM

This revision is now accepted and ready to land.Apr 9 2020, 11:03 AM

Closed by commit rDSTO566c325cce88: storage*: Add `clear_buffers` operation for proxy storages (authored by ardumont). · Explain WhyApr 9 2020, 11:11 AM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDSTO566c325cce88: storage*: Add `clear_buffers` operation for proxy storages.

ardumont added a commit: rDSTOb0b0313c9645: test_filter: Extract the filter storage into a fixture.

olasd: successfully used magit to push as you explain to me \m/

Fixing my magit setup a while back, i finally understood why i could not use the interactive rebase.
I completely forgot about the "exec" command prefix in the line (i just put the command right there...).

Magit's explain docstring within the git-rebase-todo mode showed me my erroneous ways...

ardumont mentioned this in rDSTO54b2907e4d5f: storage*: Add flush endpoints to storage implems (backend, proxy).Apr 10 2020, 1:52 PM

ardumont added a task: T2352: nixguix: Fails to finish as it's stuck in a loop up to memory error.Apr 14 2020, 10:25 AM

storage.buffer: Add a new clear_buffers operation for the buffer proxy
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D2966 (id=10553)

Changes applied before test

Patch application report for D2966 (id=10554)

Changes applied before test

Patch application report for D2966 (id=10557)

Changes applied before test

Patch application report for D2966 (id=10558)

Changes applied before test

Patch application report for D2966 (id=10574)

Changes applied before test

Patch application report for D2966 (id=10594)

Changes applied before test

Patch application report for D2966 (id=10614)

Changes applied before test

Patch application report for D2966 (id=10616)

Changes applied before test

Patch application report for D2966 (id=10616)

Changes applied before test

Revision Contents
Changeset List

Diff 10594

swh/storage/buffer.py

swh/storage/cassandra/storage.py

swh/storage/filter.py

swh/storage/in_memory.py

swh/storage/interface.py

swh/storage/retry.py

swh/storage/storage.py

swh/storage/tests/test_buffer.py

swh/storage/tests/test_filter.py

swh/storage/tests/test_storage.py

swh/storage/validate.py

storage.buffer: Add a new clear_buffers operation for the buffer proxyClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D2966 (id=10553)

Changes applied before test

Patch application report for D2966 (id=10554)

Changes applied before test

Patch application report for D2966 (id=10557)

Changes applied before test

Patch application report for D2966 (id=10558)

Changes applied before test

Patch application report for D2966 (id=10574)

Changes applied before test

Patch application report for D2966 (id=10594)

Changes applied before test

Patch application report for D2966 (id=10614)

Changes applied before test

Patch application report for D2966 (id=10616)

Changes applied before test

Patch application report for D2966 (id=10616)

Changes applied before test

Revision ContentsChangeset List

Diff 10594

swh/storage/buffer.py

swh/storage/cassandra/storage.py

swh/storage/filter.py

swh/storage/in_memory.py

swh/storage/interface.py

swh/storage/retry.py

swh/storage/storage.py

swh/storage/tests/test_buffer.py

swh/storage/tests/test_filter.py

swh/storage/tests/test_storage.py

swh/storage/validate.py

storage.buffer: Add a new clear_buffers operation for the buffer proxy
ClosedPublic
Actions

Revision Contents
Changeset List