Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[internal] use CAS BatchReadBlobs API for small blob reads #15969

Merged
merged 2 commits into from
Jun 28, 2022

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Jun 28, 2022

In #12537, Pants learned how to send small blob writes to a remote store via the BatchUpdateBlobs API of the REAPI instead of the ByteStream.Write streaming API. This PR similarly teaches Pants how to use the BatchReadBlobs API for small blob reads instead of the ByteStream.Read API.

The goal is to reduce the overhead of small blob reads by avoiding the multiple message exchanges required by the streaming API.

Note: This PR does not batch multiple small blob reads together, which is an approach that could be worthwhile to evaluate in the future.

Tom Dyas added 2 commits June 28, 2022 14:34
[ci skip-build-wheels]
[ci skip-build-wheels]
@tdyas tdyas added the category:internal CI, fixes for not-yet-released features, etc. label Jun 28, 2022
@tdyas
Copy link
Contributor Author

tdyas commented Jun 28, 2022

Note: The tests will need to be reworked to test both small and large blob paths. This could be done by directly calling the load_bytes_with_batch and load_bytes_with_stream methods in the tests. The same should be done for the write tests. I can do this as a follow-up.

@Eric-Arellano
Copy link
Contributor

Needs cherry-pick to 2.13?

@tdyas
Copy link
Contributor Author

tdyas commented Jun 28, 2022

Needs cherry-pick to 2.13?

Eventually maybe but not today. I need to do a follow-up which properly invokes both the bath and stream paths in the tests.

@tdyas tdyas merged commit 5874884 into pantsbuild:main Jun 28, 2022
@tdyas tdyas deleted the reapi_batch_read branch June 28, 2022 20:33
tdyas pushed a commit to tdyas/pants that referenced this pull request Jun 29, 2022
…d#15969)

In pantsbuild#12537, Pants learned how to send small blob writes to a remote store via the `BatchUpdateBlobs` API of the REAPI instead of the `ByteStream.Write` streaming API. This PR similarly teaches Pants how to use the `BatchReadBlobs` API for small blob reads instead of the `ByteStream.Read` API. 

The goal is to reduce the overhead of small blob reads by avoiding the multiple message exchanges required by the streaming API.

Note: This PR does not batch multiple small blob reads together, which is an approach that could be worthwhile to evaluate in the future.
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
tdyas pushed a commit that referenced this pull request Jun 29, 2022
…ck of #15969) (#15997)

In #12537, Pants learned how to send small blob writes to a remote store via the `BatchUpdateBlobs` API of the REAPI instead of the `ByteStream.Write` streaming API. This PR similarly teaches Pants how to use the `BatchReadBlobs` API for small blob reads instead of the `ByteStream.Read` API. 

The goal is to reduce the overhead of small blob reads by avoiding the multiple message exchanges required by the streaming API.

Note: This PR does not batch multiple small blob reads together, which is an approach that could be worthwhile to evaluate in the future.
@stuhood stuhood mentioned this pull request Jun 29, 2022
kaos added a commit to kaos/pants that referenced this pull request Jun 30, 2022
…antsbuild#15969)"

This reverts commit 5874884.

This resolves and issue with running `./pants check` on more than just a few files.
tdyas pushed a commit to tdyas/pants that referenced this pull request Jun 30, 2022
tdyas pushed a commit to tdyas/pants that referenced this pull request Jun 30, 2022
tdyas pushed a commit that referenced this pull request Jun 30, 2022
Causes stack blowout due to the Future being too big.

* Revert "[internal] test batch and stream API paths for load/store from remote store (#15996)"

This reverts commit 2fa8671.

[ci skip-build-wheels]

* Revert "[internal] use CAS BatchReadBlobs API for small blob reads (#15969)"

This reverts commit 5874884.

[ci skip-build-wheels]
tdyas pushed a commit that referenced this pull request Jun 30, 2022
…herry-pick of #15969) (#15997)" (#16017)

This reverts commit bcbfda5.

[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants