Page MenuHomeSoftware Heritage

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

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

Details

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
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 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.

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

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?

Fix type annotation

swh/storage/buffer.py
103

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

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.

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.

interface: Improve docstring sentence

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.

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

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

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.

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?

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 edited the summary of this revision. (Show Details)

Adapt according to review

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.

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