Page MenuHomeSoftware Heritage
Differential D2980

storage*: Add flush endpoints to storage implems (backend, proxy)
ClosedPublic
Actions

Authored by ardumont on Apr 8 2020, 3:28 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Commits
rDSTO54b2907e4d5f: storage*: Add flush endpoints to storage implems (backend, proxy)
Summary

All storage now define one flush endpoint even if it's mostly noop.
This avoids introspection surprises and is consistent with other similar
endpoint (D2966)

Test Plan

tox

Diff Detail

Repository
rDSTO Storage manager
Branch
add-flush-operation
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11779
Build 17860: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 17859: arc lint + arc unit

Event Timeline

ardumont created this revision.Apr 8 2020, 3:28 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 8 2020, 3:28 PM
swh-public-ci added a comment.Apr 8 2020, 3:34 PM

Build is green

Patch application report for D2980 (id=10593)

Rebasing onto cd52a03bee...

Current branch diff-target is up to date.
Changes applied before test
commit aa7fa7a38da1134ef502986e22fa93939a60c3e1
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 15:27:08 2020 +0200

    storage*: Add flush endpoints to storage implems (backend, proxy)
    
    All storage defines one endpoint even if it's mostly noop. This avoids
    introspection surprises.
    
    Related to D2966 (to be consistent)

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

Harbormaster completed remote builds in B11682: Diff 10593.Apr 8 2020, 3:34 PM
vlorentz requested changes to this revision.Apr 8 2020, 3:38 PM
vlorentz added a subscriber: vlorentz.

I think it's dangerous to not explicitly error when it's called via the RPC API.

Each worker will have its own buffer, and calling .flush() will flush a single worker.

swh/storage/buffer.py
103

summary: Dict[str, int]

This revision now requires changes to proceed.Apr 8 2020, 3:38 PM
ardumont added a comment.Apr 8 2020, 3:47 PM

I think it's dangerous to not explicitly error when it's called via the RPC API.

How so? that does not do anything backend wise.

In any case, how would you raise rpc side?

ardumont updated this revision to Diff 10595.Apr 8 2020, 3:51 PM

Fix type annotation

swh/storage/buffer.py
103

mmm, right! I missed the comment was wrong ;)

swh-public-ci added a comment.Apr 8 2020, 3:56 PM

Build is green

Patch application report for D2980 (id=10595)

Rebasing onto cd52a03bee...

Current branch diff-target is up to date.
Changes applied before test
commit 18ccdab886f0717da85927c7b7ba5e662defc877
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 15:27:08 2020 +0200

    storage*: Add flush endpoints to storage implems (backend, proxy)
    
    All storage defines one endpoint even if it's mostly noop. This avoids
    introspection surprises.
    
    Related to D2966 (to be consistent)

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

Harbormaster completed remote builds in B11685: Diff 10595.Apr 8 2020, 3:56 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 8 2020, 4:18 PM
ardumont updated this revision to Diff 10613.Apr 9 2020, 9:28 AM

Rebase on latest master

swh-public-ci added a comment.Apr 9 2020, 9:33 AM

Build is green

Patch application report for D2980 (id=10613)

Rebasing onto ed4097c761...

Current branch diff-target is up to date.
Changes applied before test
commit e94d309f5ce9ca205dc5702c78e4b4fea80f92db
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 15:27:08 2020 +0200

    storage*: Add flush endpoints to storage implems (backend, proxy)
    
    All storage defines one endpoint even if it's mostly noop. This avoids
    introspection surprises.
    
    Related to D2966 (to be consistent)

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

Harbormaster completed remote builds in B11735: Diff 10613.Apr 9 2020, 9:33 AM
ardumont updated this revision to Diff 10615.Apr 9 2020, 9:34 AM

interface: Improve docstring sentence

swh-public-ci added a comment.Apr 9 2020, 9:44 AM

Build is green

Patch application report for D2980 (id=10615)

Rebasing onto ed4097c761...

Current branch diff-target is up to date.
Changes applied before test
commit 66d0f467c756d778f846b6f7dc2cb6399ae43895
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 15:27:08 2020 +0200

    storage*: Add flush endpoints to storage implems (backend, proxy)
    
    All storage defines one endpoint even if it's mostly noop. This avoids
    introspection surprises.
    
    Related to D2966 (to be consistent)

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

Harbormaster completed remote builds in B11737: Diff 10615.Apr 9 2020, 9:44 AM
vlorentz added a comment.Apr 9 2020, 10:28 AM
In D2980#72106, @ardumont wrote:

I think it's dangerous to not explicitly error when it's called via the RPC API.

How so? that does not do anything backend wise.

In any case, how would you raise rpc side?

I mean, if you configure a buffer on the server side.

I know we currently don't have any reason to do it, but I think we should add a safeguard for the future

ardumont updated this revision to Diff 10622.Apr 9 2020, 11:18 AM

Rebase on latest master

swh-public-ci added a comment.Apr 9 2020, 11:18 AM

Build has FAILED

Patch application report for D2980 (id=10622)

Rebasing onto b0b0313c96...

Current branch diff-target is up to date.
Changes applied before test
commit 1eab085090bb6d7a9a84a416384b12373d716db5
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 15:27:08 2020 +0200

    storage*: Add flush endpoints to storage implems (backend, proxy)
    
    All storage defines one endpoint even if it's mostly noop. This avoids
    introspection surprises.
    
    Related to D2966 (to be consistent)

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

Harbormaster failed remote builds in B11742: Diff 10622!Apr 9 2020, 11:18 AM
ardumont updated this revision to Diff 10623.Apr 9 2020, 11:21 AM

Fix mistake in rebase

swh-public-ci added a comment.Apr 9 2020, 11:26 AM

Build is green

Patch application report for D2980 (id=10623)

Rebasing onto b0b0313c96...

Current branch diff-target is up to date.
Changes applied before test
commit 59b2e70e7e27f817c39801649a913ae0161b3185
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 15:27:08 2020 +0200

    storage*: Add flush endpoints to storage implems (backend, proxy)
    
    All storage defines one endpoint even if it's mostly noop. This avoids
    introspection surprises.
    
    Related to D2966 (to be consistent)

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

Harbormaster completed remote builds in B11743: Diff 10623.Apr 9 2020, 11:26 AM
ardumont edited the summary of this revision. (Show Details)Apr 10 2020, 9:31 AM
ardumont added a comment.Apr 10 2020, 9:45 AM

I mean, if you configure a buffer on the server side.

ok

I know we currently don't have any reason to do it,

And indeed, we do not setup any currently.

but I think we should add a safeguard for the future

I'm not convinced this is needed.

How do you implement this safeguard?

Removing @remote_api_endpoint("flush") from the interface.py file and adding
explicitely a function/route (flush) in swh.storage.api.server to raise there,
is that it?

Or simply raise a "NotSupported" in the 3 backends if that method is called?

vlorentz added a comment.Apr 10 2020, 10:38 AM
In D2980#72591, @ardumont wrote:

Removing @remote_api_endpoint("flush") from the interface.py file and adding
explicitely a function/route (flush) in swh.storage.api.server to raise there,
is that it?

Just removing @remote_api_endpoint("flush") should work. If it doesn't, you can override this endpoint in the RPCServer to always error

Or simply raise a "NotSupported" in the 3 backends if that method is called?

That wouldn't change anything if there's a buffer server side; and all callers would need to catch it or a conditional like before this diff.

ardumont added a comment.Apr 10 2020, 10:51 AM

Thanks.

trying.

ardumont updated this revision to Diff 10659.Apr 10 2020, 11:08 AM
ardumont edited the summary of this revision. (Show Details)

Adapt according to review

swh-public-ci added a comment.Apr 10 2020, 11:13 AM

Build is green

Patch application report for D2980 (id=10659)

Rebasing onto b0b0313c96...

Current branch diff-target is up to date.
Changes applied before test
commit 54b2907e4d5f3333f1dec1c86aa32e84b46ecc60
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Wed Apr 8 15:27:08 2020 +0200

    storage*: Add flush endpoints to storage implems (backend, proxy)
    
    All storage defines one endpoint even if it's mostly noop. This avoids
    introspection surprises.
    
    Related to D2966 (to be consistent)

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

Harbormaster completed remote builds in B11779: Diff 10659.Apr 10 2020, 11:13 AM
vlorentz accepted this revision.Apr 10 2020, 1:24 PM

This probably deserves a comment explaining why it's not available

This revision is now accepted and ready to land.Apr 10 2020, 1:24 PM
Closed by commit rDSTO54b2907e4d5f: storage*: Add flush endpoints to storage implems (backend, proxy) (authored by ardumont). · Explain WhyApr 10 2020, 1:52 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDSTO54b2907e4d5f: storage*: Add flush endpoints to storage implems (backend, proxy).

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
storage/
2 lines98%
Loading...
cassandra/
3 lines93%
Loading...
3 lines97%
Loading...
3 lines96%
Loading...
6 lines63%
Loading...
11 lines90%
Loading...
4 lines92%
Loading...
tests/
10 lines100%
Loading...
6 lines98%
Loading...
5 lines98%
Loading...

Diff 10659

View Options

swh/storage/buffer.py

Loading...
View Options

swh/storage/cassandra/storage.py

Loading...
View Options

swh/storage/filter.py

Loading...
View Options

swh/storage/in_memory.py

Loading...
View Options

swh/storage/interface.py

Loading...
View Options

swh/storage/retry.py

Loading...
View Options

swh/storage/storage.py

Loading...
View Options

swh/storage/tests/test_server.py

Loading...
View Options

swh/storage/tests/test_storage.py

Loading...
View Options

swh/storage/validate.py

Loading...
About · Developer information · Legal