Skip to content

Commit e2c9822

Browse files
bajajneha27bajajneha27
committed
Refactoring
1 parent e9dbaa3 commit e2c9822

5 files changed

Lines changed: 77 additions & 42 deletions

File tree

‎google/cloud/storage/internal/async/connection_tracing.cc‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414

1515
#include "google/cloud/storage/internal/async/connection_tracing.h"
1616
#include "google/cloud/storage/async/writer_connection.h"
17-
#include "google/cloud/storage/internal/async/default_options.h"
1817
#include "google/cloud/storage/internal/async/object_descriptor_connection_tracing.h"
1918
#include "google/cloud/storage/internal/async/reader_connection_tracing.h"
2019
#include "google/cloud/storage/internal/async/rewriter_connection_tracing.h"
@@ -42,7 +41,7 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
4241
public:
4342
explicit AsyncConnectionTracing(
4443
std::shared_ptr<storage::AsyncConnection> impl)
45-
: impl_(std::move(impl)) {}
44+
: impl_(std::move(impl)), cache_(BucketMetadataCache::Singleton()) {}
4645

4746
~AsyncConnectionTracing() override {
4847
for (auto& f : bg_tasks_) {
@@ -263,26 +262,33 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
263262
std::string const& bucket_name) {
264263
CleanupCompletedTasks();
265264

266-
if (!BucketMetadataCache::Singleton().StartFetch(bucket_name)) {
265+
if (!cache_.StartFetch(bucket_name)) {
267266
return;
268267
}
269268

270-
auto f = std::async(std::launch::async, [bucket_name, options]() {
271-
google::cloud::internal::OptionsSpan span(options);
272-
auto conn = MakeStorageConnection(options);
273-
auto const normalized =
274-
BucketMetadataCache::NormalizeBucketName(bucket_name);
275-
storage::internal::GetBucketMetadataRequest request(normalized);
276-
auto metadata = conn->GetBucketMetadata(request);
277-
if (metadata.ok()) {
278-
BucketMetadataCache::Singleton().Put(
279-
bucket_name, BucketCacheEntry::FromMetadata(*metadata));
280-
} else if (metadata.status().code() == StatusCode::kPermissionDenied) {
281-
BucketMetadataCache::Singleton().Put(
282-
bucket_name, {"projects/_/buckets/" + normalized, "global"});
269+
std::shared_ptr<storage::internal::StorageConnection> conn;
270+
{
271+
std::lock_guard<std::mutex> lk(mu_);
272+
if (!sync_conn_) {
273+
sync_conn_ = MakeStorageConnection(impl_->options());
283274
}
284-
BucketMetadataCache::Singleton().EndFetch(bucket_name);
285-
});
275+
conn = sync_conn_;
276+
}
277+
278+
auto f =
279+
std::async(std::launch::async, [bucket_name, options, conn, this]() {
280+
google::cloud::internal::OptionsSpan span(options);
281+
auto const normalized_bucket_name =
282+
BucketMetadataCache::NormalizeBucketName(bucket_name);
283+
storage::internal::GetBucketMetadataRequest request(
284+
normalized_bucket_name);
285+
auto metadata = conn->GetBucketMetadata(request);
286+
auto entry = BucketCacheEntry::Create(bucket_name, metadata);
287+
if (entry) {
288+
cache_.Put(bucket_name, std::move(*entry));
289+
}
290+
cache_.EndFetch(bucket_name);
291+
});
286292

287293
std::unique_lock<std::mutex> lk(mu_);
288294
bg_tasks_.push_back(std::move(f));
@@ -291,7 +297,7 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
291297
void EnrichSpan(opentelemetry::trace::Span& span, Options const& options,
292298
std::string const& bucket_name) {
293299
if (bucket_name.empty()) return;
294-
auto entry = BucketMetadataCache::Singleton().Get(bucket_name);
300+
auto entry = cache_.Get(bucket_name);
295301
if (entry.has_value()) {
296302
span.SetAttribute("gcp.resource.destination.id", entry->id);
297303
span.SetAttribute("gcp.resource.destination.location", entry->location);
@@ -301,6 +307,8 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
301307
}
302308

303309
std::shared_ptr<storage::AsyncConnection> impl_;
310+
BucketMetadataCache& cache_;
311+
std::shared_ptr<storage::internal::StorageConnection> sync_conn_;
304312
std::vector<std::future<void>> bg_tasks_;
305313
std::mutex mu_;
306314
};

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
#include "google/cloud/storage/internal/bucket_metadata_cache.h"
1616
#include "google/cloud/storage/bucket_metadata.h"
17+
#include "google/cloud/status.h"
1718
#include <mutex>
1819
#include <utility>
1920

@@ -39,6 +40,21 @@ BucketCacheEntry BucketCacheEntry::FromMetadata(
3940
std::move(loc)};
4041
}
4142

43+
absl::optional<BucketCacheEntry> BucketCacheEntry::Create(
44+
std::string const& bucket_name,
45+
StatusOr<storage::BucketMetadata> const& metadata) {
46+
if (metadata.ok()) {
47+
return FromMetadata(*metadata);
48+
}
49+
if (metadata.status().code() == StatusCode::kPermissionDenied) {
50+
return BucketCacheEntry{
51+
"projects/_/buckets/" +
52+
BucketMetadataCache::NormalizeBucketName(bucket_name),
53+
"global"};
54+
}
55+
return absl::nullopt;
56+
}
57+
4258
void BucketMetadataCache::MoveToFront(std::list<std::string>::iterator it) {
4359
list_.splice(list_.begin(), list_, it);
4460
}

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@
1515
#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_BUCKET_METADATA_CACHE_H
1616
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_BUCKET_METADATA_CACHE_H
1717

18+
#include "google/cloud/storage/bucket_metadata.h"
1819
#include "google/cloud/storage/version.h"
20+
#include "google/cloud/status_or.h"
1921
#include "absl/strings/match.h"
2022
#include "absl/types/optional.h"
2123
#include <cstddef>
@@ -29,11 +31,6 @@
2931

3032
namespace google {
3133
namespace cloud {
32-
namespace storage {
33-
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
34-
class BucketMetadata;
35-
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
36-
} // namespace storage
3734
namespace storage_internal {
3835
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
3936

@@ -42,12 +39,15 @@ struct BucketCacheEntry {
4239
std::string location;
4340

4441
static BucketCacheEntry FromMetadata(storage::BucketMetadata const& m);
42+
static absl::optional<BucketCacheEntry> Create(
43+
std::string const& bucket_name,
44+
StatusOr<storage::BucketMetadata> const& metadata);
4545
};
4646

4747
class BucketMetadataCache {
4848
public:
4949
explicit BucketMetadataCache(std::size_t max_size = 10000)
50-
: max_size_(max_size) {}
50+
: max_size_(max_size == 0 ? 1 : max_size) {}
5151

5252
static BucketMetadataCache& Singleton();
5353

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

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -92,20 +92,19 @@ void TracingConnection::MaybeTriggerBackgroundFetch(
9292
}
9393

9494
auto current_options = google::cloud::internal::SaveCurrentOptions();
95-
auto f = std::async(std::launch::async, [this, bucket_name,
96-
current_options]() {
97-
google::cloud::internal::OptionsSpan span(current_options);
98-
storage::internal::GetBucketMetadataRequest request(bucket_name);
99-
auto result = impl_->GetBucketMetadata(request);
100-
101-
if (result.ok()) {
102-
cache().Put(bucket_name, BucketCacheEntry::FromMetadata(*result));
103-
} else if (result.status().code() == StatusCode::kPermissionDenied) {
104-
cache().Put(bucket_name, {"projects/_/buckets/" + bucket_name, "global"});
105-
}
106-
107-
cache().EndFetch(bucket_name);
108-
});
95+
auto f =
96+
std::async(std::launch::async, [this, bucket_name, current_options]() {
97+
google::cloud::internal::OptionsSpan span(current_options);
98+
storage::internal::GetBucketMetadataRequest request(bucket_name);
99+
auto result = impl_->GetBucketMetadata(request);
100+
101+
auto entry = BucketCacheEntry::Create(bucket_name, result);
102+
if (entry) {
103+
cache().Put(bucket_name, std::move(*entry));
104+
}
105+
106+
cache().EndFetch(bucket_name);
107+
});
109108

110109
bg_tasks_.push_back(std::move(f));
111110
}

‎google/cloud/storage/tests/object_plenty_clients_serially_integration_test.cc‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,21 @@ TEST_F(ObjectPlentyClientsSeriallyIntegrationTest, PlentyClientsSerially) {
6868
EXPECT_EQ(StatusCode::kUnimplemented, num_fds_before_test.status().code());
6969
}
7070
std::size_t delta = 0;
71+
if (track_open_files) {
72+
// Warmup to find the maximum delta, accounting for any asynchronous
73+
// background requests (like IAM AllowedLocations) that might or might
74+
// not have opened their sockets yet when we measure.
75+
for (int i = 0; i != 5; ++i) {
76+
auto read_client = MakeIntegrationTestClient(options);
77+
auto stream = read_client.ReadObject(bucket_name_, object_name);
78+
char c;
79+
stream.read(&c, 1);
80+
auto num_fds_during_test = GetNumOpenFiles();
81+
ASSERT_STATUS_OK(num_fds_during_test);
82+
delta = (std::max)(delta, *num_fds_during_test - *num_fds_before_test);
83+
}
84+
}
85+
7186
for (int i = 0; i != 100; ++i) {
7287
auto read_client = MakeIntegrationTestClient(options);
7388
auto stream = read_client.ReadObject(bucket_name_, object_name);
@@ -76,9 +91,6 @@ TEST_F(ObjectPlentyClientsSeriallyIntegrationTest, PlentyClientsSerially) {
7691
if (track_open_files) {
7792
auto num_fds_during_test = GetNumOpenFiles();
7893
ASSERT_STATUS_OK(num_fds_during_test);
79-
if (delta == 0) {
80-
delta = *num_fds_during_test - *num_fds_before_test;
81-
}
8294
EXPECT_GE(*num_fds_before_test + delta, *num_fds_during_test)
8395
<< "Expect each client to create the same number of file descriptors"
8496
<< ", num_fds_before_test=" << *num_fds_before_test

0 commit comments

Comments
 (0)