Skip to content

Commit 9bc244d

Browse files
bajajneha27bajajneha27
committed
address review comments
1 parent 394c2ee commit 9bc244d

3 files changed

Lines changed: 100 additions & 26 deletions

File tree

‎google/cloud/storage/internal/tracing_connection.cc‎

Lines changed: 6 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ TracingConnection::TracingConnection(std::shared_ptr<StorageConnection> impl)
3333
: impl_(std::move(impl)) {}
3434

3535
TracingConnection::~TracingConnection() {
36+
std::lock_guard<std::mutex> lk(mu_);
3637
for (auto& f : bg_tasks_) {
3738
if (f.valid()) f.wait();
3839
}
@@ -87,9 +88,13 @@ void TracingConnection::MaybeTriggerBackgroundFetch(
8788
cache().EndFetch(bucket_name);
8889
});
8990

90-
bg_tasks_.push_back(std::move(f));
91+
{
92+
std::lock_guard<std::mutex> lk(mu_);
93+
bg_tasks_.push_back(std::move(f));
94+
}
9195
}
9296

97+
9398
void TracingConnection::EnrichSpan(opentelemetry::trace::Span& span,
9499
std::string const& bucket_name) {
95100
if (bucket_name.empty()) return;
@@ -228,7 +233,6 @@ StatusOr<storage::ObjectMetadata> TracingConnection::InsertObjectMedia(
228233
auto scope = opentelemetry::trace::Scope(span);
229234
EnrichSpan(*span, request.bucket_name());
230235
auto result = impl_->InsertObjectMedia(request);
231-
MaybeInvalidate(result, request.bucket_name());
232236
return internal::EndSpan(*span, std::move(result));
233237
}
234238

@@ -238,7 +242,6 @@ StatusOr<storage::ObjectMetadata> TracingConnection::CopyObject(
238242
auto scope = opentelemetry::trace::Scope(span);
239243
EnrichSpan(*span, request.destination_bucket());
240244
auto result = impl_->CopyObject(request);
241-
MaybeInvalidate(result, request.destination_bucket());
242245
return internal::EndSpan(*span, std::move(result));
243246
}
244247

@@ -248,7 +251,6 @@ StatusOr<storage::ObjectMetadata> TracingConnection::GetObjectMetadata(
248251
auto scope = opentelemetry::trace::Scope(span);
249252
EnrichSpan(*span, request.bucket_name());
250253
auto result = impl_->GetObjectMetadata(request);
251-
MaybeInvalidate(result, request.bucket_name());
252254
return internal::EndSpan(*span, std::move(result));
253255
}
254256

@@ -260,7 +262,6 @@ TracingConnection::ReadObject(
260262
EnrichSpan(*span, request.bucket_name());
261263
auto reader = impl_->ReadObject(request);
262264
if (!reader) {
263-
MaybeInvalidate(reader, request.bucket_name());
264265
return internal::EndSpan(*span, std::move(reader));
265266
}
266267
return std::unique_ptr<storage::internal::ObjectReadSource>(
@@ -285,7 +286,6 @@ StatusOr<storage::internal::EmptyResponse> TracingConnection::DeleteObject(
285286
auto scope = opentelemetry::trace::Scope(span);
286287
EnrichSpan(*span, request.bucket_name());
287288
auto result = impl_->DeleteObject(request);
288-
MaybeInvalidate(result, request.bucket_name());
289289
return internal::EndSpan(*span, std::move(result));
290290
}
291291

@@ -295,7 +295,6 @@ StatusOr<storage::ObjectMetadata> TracingConnection::UpdateObject(
295295
auto scope = opentelemetry::trace::Scope(span);
296296
EnrichSpan(*span, request.bucket_name());
297297
auto result = impl_->UpdateObject(request);
298-
MaybeInvalidate(result, request.bucket_name());
299298
return internal::EndSpan(*span, std::move(result));
300299
}
301300

@@ -305,7 +304,6 @@ StatusOr<storage::ObjectMetadata> TracingConnection::MoveObject(
305304
auto scope = opentelemetry::trace::Scope(span);
306305
EnrichSpan(*span, request.bucket_name());
307306
auto result = impl_->MoveObject(request);
308-
MaybeInvalidate(result, request.bucket_name());
309307
return internal::EndSpan(*span, std::move(result));
310308
}
311309

@@ -315,7 +313,6 @@ StatusOr<storage::ObjectMetadata> TracingConnection::PatchObject(
315313
auto scope = opentelemetry::trace::Scope(span);
316314
EnrichSpan(*span, request.bucket_name());
317315
auto result = impl_->PatchObject(request);
318-
MaybeInvalidate(result, request.bucket_name());
319316
return internal::EndSpan(*span, std::move(result));
320317
}
321318

@@ -325,7 +322,6 @@ StatusOr<storage::ObjectMetadata> TracingConnection::ComposeObject(
325322
auto scope = opentelemetry::trace::Scope(span);
326323
EnrichSpan(*span, request.bucket_name());
327324
auto result = impl_->ComposeObject(request);
328-
MaybeInvalidate(result, request.bucket_name());
329325
return internal::EndSpan(*span, std::move(result));
330326
}
331327

@@ -336,7 +332,6 @@ TracingConnection::RewriteObject(
336332
auto scope = opentelemetry::trace::Scope(span);
337333
EnrichSpan(*span, request.destination_bucket());
338334
auto result = impl_->RewriteObject(request);
339-
MaybeInvalidate(result, request.destination_bucket());
340335
return internal::EndSpan(*span, std::move(result));
341336
}
342337

@@ -346,7 +341,6 @@ StatusOr<storage::ObjectMetadata> TracingConnection::RestoreObject(
346341
auto scope = opentelemetry::trace::Scope(span);
347342
EnrichSpan(*span, request.bucket_name());
348343
auto result = impl_->RestoreObject(request);
349-
MaybeInvalidate(result, request.bucket_name());
350344
return internal::EndSpan(*span, std::move(result));
351345
}
352346

@@ -359,7 +353,6 @@ TracingConnection::CreateResumableUpload(
359353
auto scope = opentelemetry::trace::Scope(span);
360354
EnrichSpan(*span, request.bucket_name());
361355
auto result = impl_->CreateResumableUpload(request);
362-
MaybeInvalidate(result, request.bucket_name());
363356
return internal::EndSpan(*span, std::move(result));
364357
}
365358

@@ -398,7 +391,6 @@ StatusOr<std::unique_ptr<std::string>> TracingConnection::UploadFileSimple(
398391
auto scope = opentelemetry::trace::Scope(span);
399392
EnrichSpan(*span, request.bucket_name());
400393
auto result = impl_->UploadFileSimple(file_name, file_size, request);
401-
if (!result) MaybeInvalidate(result.status(), request.bucket_name());
402394
return internal::EndSpan(*span, std::move(result));
403395
}
404396

@@ -410,7 +402,6 @@ StatusOr<std::unique_ptr<std::istream>> TracingConnection::UploadFileResumable(
410402
auto scope = opentelemetry::trace::Scope(span);
411403
EnrichSpan(*span, request.bucket_name());
412404
auto result = impl_->UploadFileResumable(file_name, request);
413-
if (!result) MaybeInvalidate(result.status(), request.bucket_name());
414405
return internal::EndSpan(*span, std::move(result));
415406
}
416407

@@ -423,7 +414,6 @@ Status TracingConnection::DownloadStreamToFile(
423414
EnrichSpan(*span, request.bucket_name());
424415
auto result =
425416
impl_->DownloadStreamToFile(std::move(stream), file_name, request);
426-
MaybeInvalidate(result, request.bucket_name());
427417
return internal::EndSpan(*span, result);
428418
}
429419

@@ -509,7 +499,6 @@ TracingConnection::ListObjectAcl(
509499
auto scope = opentelemetry::trace::Scope(span);
510500
EnrichSpan(*span, request.bucket_name());
511501
auto result = impl_->ListObjectAcl(request);
512-
MaybeInvalidate(result, request.bucket_name());
513502
return internal::EndSpan(*span, std::move(result));
514503
}
515504

@@ -519,7 +508,6 @@ StatusOr<storage::ObjectAccessControl> TracingConnection::CreateObjectAcl(
519508
auto scope = opentelemetry::trace::Scope(span);
520509
EnrichSpan(*span, request.bucket_name());
521510
auto result = impl_->CreateObjectAcl(request);
522-
MaybeInvalidate(result, request.bucket_name());
523511
return internal::EndSpan(*span, std::move(result));
524512
}
525513

@@ -529,7 +517,6 @@ StatusOr<storage::internal::EmptyResponse> TracingConnection::DeleteObjectAcl(
529517
auto scope = opentelemetry::trace::Scope(span);
530518
EnrichSpan(*span, request.bucket_name());
531519
auto result = impl_->DeleteObjectAcl(request);
532-
MaybeInvalidate(result, request.bucket_name());
533520
return internal::EndSpan(*span, std::move(result));
534521
}
535522

@@ -539,7 +526,6 @@ StatusOr<storage::ObjectAccessControl> TracingConnection::GetObjectAcl(
539526
auto scope = opentelemetry::trace::Scope(span);
540527
EnrichSpan(*span, request.bucket_name());
541528
auto result = impl_->GetObjectAcl(request);
542-
MaybeInvalidate(result, request.bucket_name());
543529
return internal::EndSpan(*span, std::move(result));
544530
}
545531

@@ -549,7 +535,6 @@ StatusOr<storage::ObjectAccessControl> TracingConnection::UpdateObjectAcl(
549535
auto scope = opentelemetry::trace::Scope(span);
550536
EnrichSpan(*span, request.bucket_name());
551537
auto result = impl_->UpdateObjectAcl(request);
552-
MaybeInvalidate(result, request.bucket_name());
553538
return internal::EndSpan(*span, std::move(result));
554539
}
555540

@@ -559,7 +544,6 @@ StatusOr<storage::ObjectAccessControl> TracingConnection::PatchObjectAcl(
559544
auto scope = opentelemetry::trace::Scope(span);
560545
EnrichSpan(*span, request.bucket_name());
561546
auto result = impl_->PatchObjectAcl(request);
562-
MaybeInvalidate(result, request.bucket_name());
563547
return internal::EndSpan(*span, std::move(result));
564548
}
565549

‎google/cloud/storage/internal/tracing_connection.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -185,10 +185,10 @@ class TracingConnection : public storage::internal::StorageConnection {
185185
private:
186186
void EnrichSpan(opentelemetry::trace::Span& span,
187187
std::string const& bucket_name);
188-
static void EnrichSpan(opentelemetry::trace::Span& span,
189-
storage::BucketMetadata const& metadata);
190-
static void EnrichSpan(opentelemetry::trace::Span& span,
191-
BucketCacheEntry const& entry);
188+
void EnrichSpan(opentelemetry::trace::Span& span,
189+
storage::BucketMetadata const& metadata);
190+
void EnrichSpan(opentelemetry::trace::Span& span,
191+
BucketCacheEntry const& entry);
192192
void MaybeTriggerBackgroundFetch(std::string const& bucket_name);
193193
void CleanupCompletedTasks();
194194

‎google/cloud/storage/internal/tracing_connection_test.cc‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1685,6 +1685,96 @@ TEST(TracingClientTest, DeleteNotification) {
16851685
"gl-cpp.status_code", code_str)))));
16861686
}
16871687

1688+
TEST(TracingClientTest, BucketMetadataMaybeInvalidateBucketLevelEvict) {
1689+
TracingConnection::ResetCacheForTesting();
1690+
auto mock = std::make_shared<MockClient>();
1691+
1692+
EXPECT_CALL(*mock, options)
1693+
.WillRepeatedly(testing::Return(
1694+
Options{}.set<storage_experimental::OTelSpanEnrichmentOption>(true)));
1695+
1696+
// Seed cache
1697+
EXPECT_CALL(*mock, GetBucketMetadata).WillOnce([](auto const&) {
1698+
storage::BucketMetadata metadata;
1699+
metadata.set_name("test-bucket");
1700+
metadata.set_project_number(123456);
1701+
metadata.set_location("us-east1");
1702+
metadata.set_location_type("regional");
1703+
return metadata;
1704+
});
1705+
1706+
auto under_test = TracingConnection(mock);
1707+
(void)under_test.GetBucketMetadata(
1708+
storage::internal::GetBucketMetadataRequest("test-bucket"));
1709+
1710+
// Fail a bucket-level operation with 404 (DeleteBucket)
1711+
EXPECT_CALL(*mock, DeleteBucket).WillOnce([](auto const&) {
1712+
return Status(StatusCode::kNotFound, "Bucket not found");
1713+
});
1714+
(void)under_test.DeleteBucket(
1715+
storage::internal::DeleteBucketRequest("test-bucket"));
1716+
1717+
// Verify that the cache entry was evicted.
1718+
testing::Mock::VerifyAndClearExpectations(mock.get());
1719+
EXPECT_CALL(*mock, options)
1720+
.WillRepeatedly(testing::Return(
1721+
Options{}.set<storage_experimental::OTelSpanEnrichmentOption>(true)));
1722+
1723+
EXPECT_CALL(*mock, GetObjectMetadata).WillOnce([](auto const&) {
1724+
return storage::ObjectMetadata();
1725+
});
1726+
EXPECT_CALL(*mock, GetBucketMetadata).WillOnce([](auto const&) {
1727+
return Status(StatusCode::kNotFound, "Bucket not found");
1728+
});
1729+
1730+
(void)under_test.GetObjectMetadata(
1731+
storage::internal::GetObjectMetadataRequest("test-bucket", "test-object"));
1732+
}
1733+
1734+
TEST(TracingClientTest, BucketMetadataMaybeInvalidateObjectLevelNoEvict) {
1735+
TracingConnection::ResetCacheForTesting();
1736+
auto mock = std::make_shared<MockClient>();
1737+
1738+
EXPECT_CALL(*mock, options)
1739+
.WillRepeatedly(testing::Return(
1740+
Options{}.set<storage_experimental::OTelSpanEnrichmentOption>(true)));
1741+
1742+
// Seed cache
1743+
EXPECT_CALL(*mock, GetBucketMetadata).WillOnce([](auto const&) {
1744+
storage::BucketMetadata metadata;
1745+
metadata.set_name("test-bucket");
1746+
metadata.set_project_number(123456);
1747+
metadata.set_location("us-east1");
1748+
metadata.set_location_type("regional");
1749+
return metadata;
1750+
});
1751+
1752+
auto under_test = TracingConnection(mock);
1753+
(void)under_test.GetBucketMetadata(
1754+
storage::internal::GetBucketMetadataRequest("test-bucket"));
1755+
1756+
// Fail an object-level operation with 404 (GetObjectMetadata)
1757+
EXPECT_CALL(*mock, GetObjectMetadata).WillOnce([](auto const&) {
1758+
return Status(StatusCode::kNotFound, "Object not found");
1759+
});
1760+
(void)under_test.GetObjectMetadata(
1761+
storage::internal::GetObjectMetadataRequest("test-bucket", "test-object"));
1762+
1763+
// Verify that the cache entry was NOT evicted.
1764+
testing::Mock::VerifyAndClearExpectations(mock.get());
1765+
EXPECT_CALL(*mock, options)
1766+
.WillRepeatedly(testing::Return(
1767+
Options{}.set<storage_experimental::OTelSpanEnrichmentOption>(true)));
1768+
1769+
EXPECT_CALL(*mock, GetObjectMetadata).WillOnce([](auto const&) {
1770+
return storage::ObjectMetadata();
1771+
});
1772+
EXPECT_CALL(*mock, GetBucketMetadata).Times(0);
1773+
1774+
(void)under_test.GetObjectMetadata(
1775+
storage::internal::GetObjectMetadataRequest("test-bucket", "test-object"));
1776+
}
1777+
16881778
} // namespace
16891779
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
16901780
} // namespace storage_internal

0 commit comments

Comments
 (0)