Page MenuHomeSoftware Heritage
Differential D7268

client: Do not depend on how the server chunks the response
ClosedPublic
Actions

Authored by vlorentz on Mar 1 2022, 3:14 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Maniphest Tasks
Restricted Maniphest Task
Summary

Currently, the server uses HTTP chunked encoding to send object ids
one by one, each in its own HTTP frame.

I think this is a mistake to rely on such a detail of the HTTP protocol
in a high-level API like this.

Additionally, a future commit will rewrite the server to use Flask
instead of aiohttp, which does not allow this kind of fine-grained
control about HTTP chunks.

Diff Detail

Event Timeline

vlorentz created this revision.Mar 1 2022, 3:14 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMar 1 2022, 3:14 PM
swh-public-ci added a comment.Mar 1 2022, 3:17 PM

Build is green

Patch application report for D7268 (id=26327)

Could not rebase; Attempt merge onto 76a5f36b41...

Updating 76a5f36..ff319b6
Fast-forward
 swh/objstorage/api/client.py              | 15 ++++++-------
 swh/objstorage/api/server.py              | 36 -------------------------------
 swh/objstorage/backends/pathslicing.py    | 16 ++++++++++++++
 swh/objstorage/backends/seaweedfs/http.py |  4 +---
 4 files changed, 24 insertions(+), 47 deletions(-)
Changes applied before test
commit ff319b60b2221333074d16659f8222a0d19bfe66
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 15:13:43 2022 +0100

    client: Do not depend on how the server chunks the response
    
    Currently, the server uses HTTP chunked encoding to send object ids
    one by one, each in its own HTTP frame.
    
    I think this is a mistake to rely on such a detail of the HTTP protocol
    in a high-level API like this.
    
    Additionally, a future commit will rewrite the server to use Flask
    instead of aiohttp, which does not allow this kind of fine-grained
    control about HTTP chunks.

commit b159f04b8d591947003cf3a4cd15fbcf0f3cb31e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 13:36:02 2022 +0100

    Remove method add_stream from the RPC API.
    
    Rationale:
    
    1. Only the pathslicing backend implements it
    2. No other package uses it (besides a dead code path in the vault)
    3. A future commit will rewrite the RPC server to use Flask instead of
       aiohttp, and rewriting this view correctly is going to be hard
       (though possible)

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

Harbormaster completed remote builds in B27200: Diff 26327.Mar 1 2022, 3:17 PM
vlorentz requested review of this revision.Mar 1 2022, 3:17 PM
vlorentz added a child revision: D7269: server: Group multiple object ids in the same HTTP chunk.Mar 1 2022, 3:18 PM
vlorentz added a child revision: D7272: Use flask instead of aiohttp as RPC server..Mar 1 2022, 4:32 PM
olasd added a subscriber: olasd.Mar 1 2022, 5:02 PM

Does the get_stream method guarantee the size of the received chunks? By default I would assume that it chunks contents using the passed size as a maximum.

vlorentz added a comment.Mar 1 2022, 5:07 PM

Oh... indeed, it's not https://docs.python-requests.org/en/latest/api/#requests.Response.iter_content

vlorentz updated this revision to Diff 26343.Mar 2 2022, 11:32 AM

fix chunking

swh-public-ci added a comment.Mar 2 2022, 11:34 AM

Build is green

Patch application report for D7268 (id=26343)

Rebasing onto 76a5f36b41...

First, rewinding head to replay your work on top of it...
Fast-forwarded diff-target to base-revision-127-D7268.
Changes applied before test



See https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/127/ for more details.
Harbormaster completed remote builds in B27219: Diff 26343.Mar 2 2022, 11:34 AM
olasd added a comment.Mar 2 2022, 11:38 AM
@vlorentz looks like you didn't push the right commit in this diff update
vlorentz updated this revision to Diff 26344.Mar 2 2022, 11:42 AM
fix chunking
vlorentz added a comment.Mar 2 2022, 11:42 AM
ahahah what



looks like arc diff --update D7268 -m "fix chunking" HEAD^ while resolving a git-rebase todo error doesn't do what I thought it would do
swh-public-ci added a comment.Mar 2 2022, 11:43 AM
Build has FAILED



Patch application report for D7268 (id=26344)



Could not rebase; Attempt merge onto 76a5f36b41...



Updating 76a5f36..a2424b7
Fast-forward
 swh/objstorage/api/client.py              | 16 +++++++-------
 swh/objstorage/api/server.py              | 36 -------------------------------
 swh/objstorage/backends/pathslicing.py    | 16 ++++++++++++++
 swh/objstorage/backends/seaweedfs/http.py |  4 +---
 4 files changed, 25 insertions(+), 47 deletions(-)





Changes applied before test



commit a2424b7a13e9a696329a9e9efa6c86f9864dd60b
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 15:13:43 2022 +0100

    client: Do not depend on how the server chunks the response
    
    Currently, the server uses HTTP chunked encoding to send object ids
    one by one, each in its own HTTP frame.
    
    I think this is a mistake to rely on such a detail of the HTTP protocol
    in a high-level API like this.
    
    Additionally, a future commit will rewrite the server to use Flask
    instead of aiohttp, which does not allow this kind of fine-grained
    control about HTTP chunks.

commit b159f04b8d591947003cf3a4cd15fbcf0f3cb31e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 13:36:02 2022 +0100

    Remove method add_stream from the RPC API.
    
    Rationale:
    
    1. Only the pathslicing backend implements it
    2. No other package uses it (besides a dead code path in the vault)
    3. A future commit will rewrite the RPC server to use Flask instead of
       aiohttp, and rewriting this view correctly is going to be hard
       (though possible)



Link to build: https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/128/

See console output for more information: https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/128/console
Harbormaster failed remote builds in B27220: Diff 26344!Mar 2 2022, 11:43 AM
vlorentz added a parent revision: D7275: utils: Add a new 'iter_chunks' function.Mar 2 2022, 11:43 AM
vlorentz updated this revision to Diff 26345.Mar 2 2022, 11:44 AM
bump dep on swh-core
swh-public-ci added a comment.Mar 2 2022, 11:44 AM
Build has FAILED



Patch application report for D7268 (id=26345)



Could not rebase; Attempt merge onto 76a5f36b41...



Updating 76a5f36..ebfdab5
Fast-forward
 requirements-swh.txt                      |  2 +-
 swh/objstorage/api/client.py              | 16 +++++++-------
 swh/objstorage/api/server.py              | 36 -------------------------------
 swh/objstorage/backends/pathslicing.py    | 16 ++++++++++++++
 swh/objstorage/backends/seaweedfs/http.py |  4 +---
 5 files changed, 26 insertions(+), 48 deletions(-)





Changes applied before test



commit ebfdab59fd28a18ebef09d398b128ea043eaf0e9
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 15:13:43 2022 +0100

    client: Do not depend on how the server chunks the response
    
    Currently, the server uses HTTP chunked encoding to send object ids
    one by one, each in its own HTTP frame.
    
    I think this is a mistake to rely on such a detail of the HTTP protocol
    in a high-level API like this.
    
    Additionally, a future commit will rewrite the server to use Flask
    instead of aiohttp, which does not allow this kind of fine-grained
    control about HTTP chunks.

commit b159f04b8d591947003cf3a4cd15fbcf0f3cb31e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 13:36:02 2022 +0100

    Remove method add_stream from the RPC API.
    
    Rationale:
    
    1. Only the pathslicing backend implements it
    2. No other package uses it (besides a dead code path in the vault)
    3. A future commit will rewrite the RPC server to use Flask instead of
       aiohttp, and rewriting this view correctly is going to be hard
       (though possible)



Link to build: https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/129/

See console output for more information: https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/129/console
Harbormaster failed remote builds in B27221: Diff 26345!Mar 2 2022, 11:44 AM
vlorentz added a comment.Mar 2 2022, 11:44 AM
(build is failing because of the dep on D7275)
swh-public-ci added a comment.Mar 2 2022, 11:48 AM
Build has FAILED



Patch application report for D7268 (id=26345)



Could not rebase; Attempt merge onto 76a5f36b41...



Updating 76a5f36..ebfdab5
Fast-forward
 requirements-swh.txt                      |  2 +-
 swh/objstorage/api/client.py              | 16 +++++++-------
 swh/objstorage/api/server.py              | 36 -------------------------------
 swh/objstorage/backends/pathslicing.py    | 16 ++++++++++++++
 swh/objstorage/backends/seaweedfs/http.py |  4 +---
 5 files changed, 26 insertions(+), 48 deletions(-)





Changes applied before test



commit ebfdab59fd28a18ebef09d398b128ea043eaf0e9
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 15:13:43 2022 +0100

    client: Do not depend on how the server chunks the response
    
    Currently, the server uses HTTP chunked encoding to send object ids
    one by one, each in its own HTTP frame.
    
    I think this is a mistake to rely on such a detail of the HTTP protocol
    in a high-level API like this.
    
    Additionally, a future commit will rewrite the server to use Flask
    instead of aiohttp, which does not allow this kind of fine-grained
    control about HTTP chunks.

commit b159f04b8d591947003cf3a4cd15fbcf0f3cb31e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 13:36:02 2022 +0100

    Remove method add_stream from the RPC API.
    
    Rationale:
    
    1. Only the pathslicing backend implements it
    2. No other package uses it (besides a dead code path in the vault)
    3. A future commit will rewrite the RPC server to use Flask instead of
       aiohttp, and rewriting this view correctly is going to be hard
       (though possible)



Link to build: https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/130/

See console output for more information: https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/130/console
swh-public-ci added a comment.Mar 2 2022, 12:56 PM
Build is green



Patch application report for D7268 (id=26345)



Could not rebase; Attempt merge onto 76a5f36b41...



Updating 76a5f36..ebfdab5
Fast-forward
 requirements-swh.txt                      |  2 +-
 swh/objstorage/api/client.py              | 16 +++++++-------
 swh/objstorage/api/server.py              | 36 -------------------------------
 swh/objstorage/backends/pathslicing.py    | 16 ++++++++++++++
 swh/objstorage/backends/seaweedfs/http.py |  4 +---
 5 files changed, 26 insertions(+), 48 deletions(-)





Changes applied before test



commit ebfdab59fd28a18ebef09d398b128ea043eaf0e9
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 15:13:43 2022 +0100

    client: Do not depend on how the server chunks the response
    
    Currently, the server uses HTTP chunked encoding to send object ids
    one by one, each in its own HTTP frame.
    
    I think this is a mistake to rely on such a detail of the HTTP protocol
    in a high-level API like this.
    
    Additionally, a future commit will rewrite the server to use Flask
    instead of aiohttp, which does not allow this kind of fine-grained
    control about HTTP chunks.

commit b159f04b8d591947003cf3a4cd15fbcf0f3cb31e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 1 13:36:02 2022 +0100

    Remove method add_stream from the RPC API.
    
    Rationale:
    
    1. Only the pathslicing backend implements it
    2. No other package uses it (besides a dead code path in the vault)
    3. A future commit will rewrite the RPC server to use Flask instead of
       aiohttp, and rewriting this view correctly is going to be hard
       (though possible)



See https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/134/ for more details.
Harbormaster completed remote builds in B27221: Diff 26345.Mar 2 2022, 12:56 PM
olasd accepted this revision.Mar 2 2022, 12:59 PM
Cool, thanks (see inline comment).
swh/objstorage/api/client.py


12–13
This should probably be derived from swh.objstorage.objstorage.ID_HASH_LENGTH (or at least set there)
This revision is now accepted and ready to land.Mar 2 2022, 12:59 PM
vlorentz mentioned this in D7277: Rename and document constants in objstorage.py.Mar 2 2022, 1:30 PM
vlorentz added inline comments.Mar 2 2022, 1:30 PM
swh/objstorage/api/client.py


12–13
Done in D7277
vlorentz closed this revision.Mar 7 2022, 9:02 AM
a2424b7a13e9a696329a9e9efa6c86f9864dd60b
vlorentz added a task: Restricted Maniphest Task.Mar 7 2022, 9:25 AM
Revision Contents
Changeset List
PathSizeCoverage (All)Coverage (Touched)
2 lines--
swh/
objstorage/
api/
9 lines97%
Loading...
Diff 26345
View Options
requirements-swh.txt
Loading...
View Options
swh/objstorage/api/client.py
Loading...
About · Developer information · Legal