feat(storage): add feature tracker for multistream feature#16180
feat(storage): add feature tracker for multistream feature#16180v-pratap wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a client-side feature tracking mechanism (FeatureTracker) to report adopted optimizations and configuration choices via the x-goog-storage-cpp-features header in both gRPC and REST paths. While the overall implementation is solid, there are critical issues with how the header is attached. Specifically, adding the tracker to fixed_metadata in CreateDecoratedStubs makes the header static, which violates per-request options and fails to capture dynamic features. Additionally, the asynchronous streaming path in OpenObject needs to respect the EnableFeatureReportsOption rather than adding the header unconditionally. Addressing these issues will also allow the removal of the now-redundant FeatureTrackerInStorageMetadata test.
| std::multimap<std::string, std::string> fixed_metadata; | ||
| bool const enable_reports = | ||
| !options.has<storage::EnableFeatureReportsOption>() || | ||
| options.get<storage::EnableFeatureReportsOption>(); | ||
| if (enable_reports && | ||
| options.has<storage::internal::FeatureTrackerOption>()) { | ||
| auto const& tracker = | ||
| options.get<storage::internal::FeatureTrackerOption>(); | ||
| if (tracker) { | ||
| auto const val = tracker->HeaderValue(); | ||
| if (!val.empty()) { | ||
| fixed_metadata.emplace(storage::internal::kFeatureTrackerHeaderName, | ||
| val); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Adding the feature tracker header to fixed_metadata in CreateDecoratedStubs makes it a static header for the lifetime of the client. This has two major drawbacks:
- It violates the request-specific
EnableFeatureReportsOption(false)becauseStorageMetadata'sfixed_metadatais appended unconditionally to every request, bypassing any per-request options. - It cannot support any dynamic or operation-level features that might be registered on the tracker later, as the header value is copied once during stub creation.
Since ApplyQueryParameters (for gRPC) and AddHeaders (for REST) already dynamically and correctly append this header while respecting request-specific options, we should remove the feature tracker from fixed_metadata in CreateDecoratedStubs.
std::multimap<std::string, std::string> fixed_metadata;| if (tracker) { | ||
| auto v = tracker->HeaderValue(); | ||
| if (!v.empty()) { | ||
| context->AddMetadata(storage::internal::kFeatureTrackerHeaderName, | ||
| std::move(v)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The asynchronous streaming RPC path currently adds the feature tracker header unconditionally if a tracker is present. It should respect the EnableFeatureReportsOption from the request-specific options to allow users to disable feature reporting for specific operations.
bool const enable_reports =
!options.has<storage::EnableFeatureReportsOption>() ||
options.get<storage::EnableFeatureReportsOption>();
if (enable_reports && tracker) {
auto v = tracker->HeaderValue();
if (!v.empty()) {
context->AddMetadata(storage::internal::kFeatureTrackerHeaderName,
std::move(v));
}
}| TEST_F(StorageStubFactory, FeatureTrackerInStorageMetadata) { | ||
| auto tracker = std::make_shared<storage::internal::FeatureTracker>(); | ||
| tracker->RegisterFeature(storage::internal::TrackedFeature::kPCU); | ||
|
|
||
| MockFactory factory; | ||
| EXPECT_CALL(factory, Call) | ||
| .WillOnce([&](std::shared_ptr<grpc::Channel> const&) { | ||
| auto mock = std::make_shared<MockStorageStub>(); | ||
| EXPECT_CALL(*mock, DeleteBucket) | ||
| .WillOnce([&](grpc::ClientContext& context, Options const&, | ||
| google::storage::v2::DeleteBucketRequest const&) { | ||
| ValidateMetadataFixture fixture; | ||
| auto metadata = fixture.GetMetadata(context); | ||
| EXPECT_THAT(metadata, | ||
| Contains(Pair(storage::internal::kFeatureTrackerHeaderName, | ||
| tracker->HeaderValue()))); | ||
| return internal::AbortedError("fail"); | ||
| }); | ||
| return mock; | ||
| }); | ||
|
|
||
| internal::AutomaticallyCreatedBackgroundThreads pool; | ||
| auto stub = CreateTestStub( | ||
| pool.cq(), factory.AsStdFunction(), | ||
| Options{} | ||
| .set<GrpcNumChannelsOption>(1) | ||
| .set<storage::internal::FeatureTrackerOption>(tracker)); | ||
| grpc::ClientContext context; | ||
| (void)stub->DeleteBucket(context, Options{}, {}); | ||
| } |
276d61c to
89bca1a
Compare
No description provided.