Page MenuHomePhabricator

[services][commtest] Replace gRPC Blob client with HTTP
ClosedPublic

Authored by bartek on Jun 30 2023, 10:09 PM.
Tags
None
Referenced Files
F3402821: D8408.diff
Mon, Dec 2, 9:16 PM
Unknown Object (File)
Sun, Dec 1, 4:33 PM
Unknown Object (File)
Sat, Nov 23, 2:29 PM
Unknown Object (File)
Sat, Nov 23, 2:28 PM
Unknown Object (File)
Sat, Nov 23, 2:28 PM
Unknown Object (File)
Mon, Nov 18, 2:43 PM
Unknown Object (File)
Sat, Nov 16, 11:28 PM
Unknown Object (File)
Sat, Nov 16, 11:28 PM
Subscribers

Details

Summary

Resolves ENG-3869. Decided to finish this now to unblock gRPC removal and leave Commtest unbroken when using the new Blob service DB schema that will require the new HTTP interface.

A brief summary of the changes:

  • Added reqwest dependency - a HTTP client. Also added serde but we already use it in other services - added @ashoat as a reviewer
  • Replaced gRPC BlobServiceClient utilities with HTTP reqwest calls.
  • Removed some unnecessary &mut references

This diff is pretty big but I had difficulties splitting it into smaller ones. I added inline comments to explain the changes.

Test Plan

Started Blob service in trace log level, with the following command:

RUST_LOG=blob=trace cargo run -- --sandbox --localstack-url "http://localstack:4566" --http-port 50053

Important thing is to set the port to 50053 because this is what the commtest expects - it reads the COMM_SERVICES_PORT_BLOB env var that is set in services/.env

Then started both integration and performance tests:

cd services
yarn run-integration-tests blob
yarn run-performance-tests blob

Observed blob service trace logs and confirmed it is doing steps that are expected for the tests.

Diff Detail

Repository
rCOMM Comm
Branch
barthap/blob-database
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Jun 30 2023, 10:34 PM
bartek added 2 blocking reviewer(s): ashoat, michal.
bartek added inline comments.
services/commtest/Cargo.toml
29–30

cc @ashoat:

The reqwest is a HTTP client library. It is double-licensed: MIT or Apache 2.0

Serde is already used in other services

Dependency looks good

Looks good, we could potentially use reqwest:Url and .set_port(), .join() for URL manipulation.

This revision is now accepted and ready to land.Jul 3 2023, 3:50 AM

Rebase. Use reqwest::Url instead of strings