Skip to content

Commit 8cf1644

Browse files
bajajneha27bajajneha27
committed
refactor and remove unnecessary code
1 parent 7453c11 commit 8cf1644

5 files changed

Lines changed: 90 additions & 98 deletions

File tree

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

Lines changed: 44 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#include "google/cloud/storage/options.h"
2323
#include "google/cloud/internal/opentelemetry.h"
2424
#include "google/cloud/version.h"
25-
#include "absl/strings/match.h"
2625
#include "google/storage/v2/storage.pb.h"
2726
#include <algorithm>
2827
#include <chrono>
@@ -39,14 +38,6 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
3938

4039
namespace {
4140

42-
std::string NormalizeBucketName(std::string const& bucket) {
43-
auto const prefix = std::string("projects/_/buckets/");
44-
if (absl::StartsWith(bucket, prefix)) {
45-
return bucket.substr(prefix.size());
46-
}
47-
return bucket;
48-
}
49-
5041
class AsyncConnectionTracing : public storage::AsyncConnection {
5142
public:
5243
explicit AsyncConnectionTracing(
@@ -73,12 +64,9 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
7364
internal::OTelScope scope(span);
7465
return impl_->InsertObject(std::move(p))
7566
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
76-
span = std::move(span), this,
77-
bucket_name =
78-
p.request.write_object_spec().resource().bucket()](auto f) {
67+
span = std::move(span)](auto f) {
7968
auto result = f.get();
8069
internal::DetachOTelContext(oc);
81-
MaybeInvalidate(result, bucket_name);
8270
return internal::EndSpan(*span, std::move(result));
8371
});
8472
}
@@ -90,14 +78,12 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
9078
internal::OTelScope scope(span);
9179
return impl_->Open(std::move(p))
9280
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
93-
span = std::move(span), this,
94-
bucket_name = p.read_spec.bucket()](auto f)
81+
span = std::move(span)](auto f)
9582
-> StatusOr<
9683
std::shared_ptr<storage::ObjectDescriptorConnection>> {
9784
auto result = f.get();
9885
internal::DetachOTelContext(oc);
9986
if (!result) {
100-
MaybeInvalidate(result, bucket_name);
10187
return internal::EndSpan(*span, std::move(result).status());
10288
}
10389
return MakeTracingObjectDescriptorConnection(std::move(span),
@@ -111,13 +97,11 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
11197
EnrichSpan(*span, p.options, p.request.bucket());
11298
internal::OTelScope scope(span);
11399
auto wrap = [oc = opentelemetry::context::RuntimeContext::GetCurrent(),
114-
span = std::move(span), this,
115-
bucket_name = p.request.bucket()](auto f)
100+
span = std::move(span)](auto f)
116101
-> StatusOr<std::unique_ptr<storage::AsyncReaderConnection>> {
117102
auto reader = f.get();
118103
internal::DetachOTelContext(oc);
119104
if (!reader) {
120-
MaybeInvalidate(reader, bucket_name);
121105
return internal::EndSpan(*span, std::move(reader).status());
122106
}
123107
return MakeTracingReaderConnection(std::move(span), *std::move(reader));
@@ -132,11 +116,9 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
132116
internal::OTelScope scope(span);
133117
return impl_->ReadObjectRange(std::move(p))
134118
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
135-
span = std::move(span), this,
136-
bucket_name = p.request.bucket()](auto f) {
119+
span = std::move(span)](auto f) {
137120
auto result = f.get();
138121
internal::DetachOTelContext(oc);
139-
MaybeInvalidate(result, bucket_name);
140122
return internal::EndSpan(*span, std::move(result));
141123
});
142124
}
@@ -150,14 +132,11 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
150132
internal::OTelScope scope(span);
151133
return impl_->StartAppendableObjectUpload(std::move(p))
152134
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
153-
span = std::move(span), this,
154-
bucket_name =
155-
p.request.write_object_spec().resource().bucket()](auto f)
135+
span = std::move(span)](auto f)
156136
-> StatusOr<std::unique_ptr<storage::AsyncWriterConnection>> {
157137
auto w = f.get();
158138
internal::DetachOTelContext(oc);
159139
if (!w) {
160-
MaybeInvalidate(w, bucket_name);
161140
return internal::EndSpan(*span, std::move(w).status());
162141
}
163142
return MakeTracingWriterConnection(span, *std::move(w));
@@ -173,14 +152,11 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
173152
internal::OTelScope scope(span);
174153
return impl_->ResumeAppendableObjectUpload(std::move(p))
175154
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
176-
span = std::move(span), this,
177-
bucket_name =
178-
p.request.write_object_spec().resource().bucket()](auto f)
155+
span = std::move(span)](auto f)
179156
-> StatusOr<std::unique_ptr<storage::AsyncWriterConnection>> {
180157
auto w = f.get();
181158
internal::DetachOTelContext(oc);
182159
if (!w) {
183-
MaybeInvalidate(w, bucket_name);
184160
return internal::EndSpan(*span, std::move(w).status());
185161
}
186162
return MakeTracingWriterConnection(span, *std::move(w));
@@ -196,14 +172,11 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
196172
internal::OTelScope scope(span);
197173
return impl_->StartUnbufferedUpload(std::move(p))
198174
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
199-
span = std::move(span), this,
200-
bucket_name =
201-
p.request.write_object_spec().resource().bucket()](auto f)
175+
span = std::move(span)](auto f)
202176
-> StatusOr<std::unique_ptr<storage::AsyncWriterConnection>> {
203177
auto w = f.get();
204178
internal::DetachOTelContext(oc);
205179
if (!w) {
206-
MaybeInvalidate(w, bucket_name);
207180
return internal::EndSpan(*span, std::move(w).status());
208181
}
209182
return MakeTracingWriterConnection(span, *std::move(w));
@@ -219,14 +192,11 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
219192
internal::OTelScope scope(span);
220193
return impl_->StartBufferedUpload(std::move(p))
221194
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
222-
span = std::move(span), this,
223-
bucket_name =
224-
p.request.write_object_spec().resource().bucket()](auto f)
195+
span = std::move(span)](auto f)
225196
-> StatusOr<std::unique_ptr<storage::AsyncWriterConnection>> {
226197
auto w = f.get();
227198
internal::DetachOTelContext(oc);
228199
if (!w) {
229-
MaybeInvalidate(w, bucket_name);
230200
return internal::EndSpan(*span, std::move(w).status());
231201
}
232202
return MakeTracingWriterConnection(span, *std::move(w));
@@ -272,11 +242,9 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
272242
internal::OTelScope scope(span);
273243
return impl_->ComposeObject(std::move(p))
274244
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
275-
span = std::move(span), this,
276-
bucket_name = p.request.destination().bucket()](auto f) {
245+
span = std::move(span)](auto f) {
277246
auto result = f.get();
278247
internal::DetachOTelContext(oc);
279-
MaybeInvalidate(result, bucket_name);
280248
return internal::EndSpan(*span, std::move(result));
281249
});
282250
}
@@ -287,11 +255,9 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
287255
internal::OTelScope scope(span);
288256
return impl_->DeleteObject(std::move(p))
289257
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
290-
span = std::move(span), this,
291-
bucket_name = p.request.bucket()](auto f) {
258+
span = std::move(span)](auto f) {
292259
auto result = f.get();
293260
internal::DetachOTelContext(oc);
294-
MaybeInvalidate(result, bucket_name);
295261
return internal::EndSpan(*span, std::move(result));
296262
});
297263
}
@@ -308,25 +274,18 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
308274
auto span = internal::MakeSpan("storage::AsyncConnection::GetBucket");
309275
internal::OTelScope scope(span);
310276
auto const bucket_name = p.request.name();
277+
auto const options = p.options;
311278
return impl_->GetBucket(std::move(p))
312279
.then([oc = opentelemetry::context::RuntimeContext::GetCurrent(),
313-
span = std::move(span), this,
314-
bucket_name](future<StatusOr<google::storage::v2::Bucket>> f)
280+
span = std::move(span), this, bucket_name,
281+
options](future<StatusOr<google::storage::v2::Bucket>> f)
315282
-> StatusOr<google::storage::v2::Bucket> {
316283
auto result = f.get();
317284
internal::DetachOTelContext(oc);
318285
if (result.ok()) {
319-
std::string loc = result->location();
320-
if (result->location_type() == "multi-region" ||
321-
result->location_type() == "dual-region") {
322-
loc = "global";
323-
}
324-
BucketCacheEntry entry{result->project() + "/buckets/" +
325-
NormalizeBucketName(bucket_name),
326-
std::move(loc)};
327-
cache().Put(bucket_name, std::move(entry));
286+
EnrichSpan(*span, options, *result, bucket_name);
328287
} else {
329-
MaybeInvalidate(result, bucket_name);
288+
cache().MaybeInvalidate(result, bucket_name);
330289
}
331290
return internal::EndSpan(*span, std::move(result));
332291
});
@@ -335,18 +294,6 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
335294
private:
336295
BucketMetadataCache& cache() const { return *cache_; }
337296

338-
void MaybeInvalidate(Status const& status, std::string const& bucket_name) {
339-
if (!status.ok() && status.code() == StatusCode::kNotFound) {
340-
cache().Invalidate(bucket_name);
341-
}
342-
}
343-
344-
template <typename T>
345-
void MaybeInvalidate(StatusOr<T> const& result,
346-
std::string const& bucket_name) {
347-
MaybeInvalidate(result.status(), bucket_name);
348-
}
349-
350297
void MaybeTriggerBackgroundFetch(Options const& options,
351298
std::string const& bucket_name) {
352299
if (!cache().StartFetch(bucket_name)) {
@@ -355,7 +302,8 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
355302

356303
auto guard = ScopedFetch(cache_, bucket_name);
357304
google::storage::v2::GetBucketRequest request;
358-
auto const normalized_bucket_name = NormalizeBucketName(bucket_name);
305+
auto const normalized_bucket_name =
306+
BucketMetadataCache::NormalizeBucketName(bucket_name);
359307
request.set_name("projects/_/buckets/" + normalized_bucket_name);
360308
GetBucketParams params{std::move(request), options};
361309

@@ -364,25 +312,42 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
364312
future<StatusOr<google::storage::v2::Bucket>> f) {
365313
auto metadata = f.get();
366314
if (metadata.ok()) {
367-
std::string loc = metadata->location();
368-
if (metadata->location_type() == "multi-region" ||
369-
metadata->location_type() == "dual-region") {
370-
loc = "global";
371-
}
372-
BucketCacheEntry entry{metadata->project() + "/buckets/" +
373-
NormalizeBucketName(bucket_name),
374-
std::move(loc)};
315+
BucketCacheEntry entry = BucketCacheEntry::FromLocation(
316+
metadata->project() + "/buckets/" +
317+
BucketMetadataCache::NormalizeBucketName(bucket_name),
318+
metadata->location(), metadata->location_type());
375319
cache->Put(bucket_name, std::move(entry));
376320
} else if (metadata.status().code() ==
377321
StatusCode::kPermissionDenied) {
378322
BucketCacheEntry entry{
379-
"projects/_/buckets/" + NormalizeBucketName(bucket_name),
323+
"projects/_/buckets/" +
324+
BucketMetadataCache::NormalizeBucketName(bucket_name),
380325
"global"};
381326
cache->Put(bucket_name, std::move(entry));
382327
}
383328
});
384329
}
385330

331+
void EnrichSpan(opentelemetry::trace::Span& span,
332+
BucketCacheEntry const& entry) {
333+
span.SetAttribute("gcp.resource.destination.id", entry.id);
334+
span.SetAttribute("gcp.resource.destination.location", entry.location);
335+
}
336+
337+
void EnrichSpan(opentelemetry::trace::Span& span, Options const& options,
338+
google::storage::v2::Bucket const& bucket,
339+
std::string const& bucket_name) {
340+
auto const enabled = options.get<
341+
google::cloud::storage_experimental::OTelSpanEnrichmentOption>();
342+
if (!enabled) return;
343+
auto entry = BucketCacheEntry::FromLocation(
344+
bucket.project() + "/buckets/" +
345+
BucketMetadataCache::NormalizeBucketName(bucket_name),
346+
bucket.location(), bucket.location_type());
347+
EnrichSpan(span, entry);
348+
cache().Put(bucket_name, std::move(entry));
349+
}
350+
386351
void EnrichSpan(opentelemetry::trace::Span& span, Options const& options,
387352
std::string const& bucket_name) {
388353
if (bucket_name.empty()) return;
@@ -391,8 +356,7 @@ class AsyncConnectionTracing : public storage::AsyncConnection {
391356
if (!enabled) return;
392357
auto entry = cache().Get(bucket_name);
393358
if (entry.has_value()) {
394-
span.SetAttribute("gcp.resource.destination.id", entry->id);
395-
span.SetAttribute("gcp.resource.destination.location", entry->location);
359+
EnrichSpan(span, *entry);
396360
} else {
397361
MaybeTriggerBackgroundFetch(options, bucket_name);
398362
}

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

Lines changed: 20 additions & 7 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 "absl/strings/match.h"
1718
#include <mutex>
1819
#include <utility>
1920

@@ -22,16 +23,28 @@ namespace cloud {
2223
namespace storage_internal {
2324
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
2425

26+
BucketCacheEntry BucketCacheEntry::FromLocation(
27+
std::string id, std::string location, std::string const& location_type) {
28+
if (location_type == "multi-region" || location_type == "dual-region") {
29+
location = "global";
30+
}
31+
return {std::move(id), std::move(location)};
32+
}
33+
2534
BucketCacheEntry BucketCacheEntry::FromMetadata(
2635
storage::BucketMetadata const& m) {
27-
std::string loc = m.location();
28-
if (m.location_type() == "multi-region" ||
29-
m.location_type() == "dual-region") {
30-
loc = "global";
31-
}
32-
return {
36+
return FromLocation(
3337
"projects/" + std::to_string(m.project_number()) + "/buckets/" + m.name(),
34-
std::move(loc)};
38+
m.location(), m.location_type());
39+
}
40+
41+
std::string BucketMetadataCache::NormalizeBucketName(
42+
std::string const& bucket) {
43+
auto const prefix = std::string("projects/_/buckets/");
44+
if (absl::StartsWith(bucket, prefix)) {
45+
return bucket.substr(prefix.size());
46+
}
47+
return bucket;
3548
}
3649

3750
void BucketMetadataCache::MoveToFront(std::list<std::string>::iterator it) {

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_BUCKET_METADATA_CACHE_H
1717

1818
#include "google/cloud/storage/version.h"
19+
#include "google/cloud/status.h"
20+
#include "google/cloud/status_or.h"
1921
#include "absl/types/optional.h"
2022
#include <cstddef>
2123
#include <list>
@@ -40,6 +42,8 @@ struct BucketCacheEntry {
4042
std::string id;
4143
std::string location;
4244

45+
static BucketCacheEntry FromLocation(std::string id, std::string location,
46+
std::string const& location_type);
4347
static BucketCacheEntry FromMetadata(storage::BucketMetadata const& m);
4448
};
4549

@@ -48,13 +52,27 @@ class BucketMetadataCache {
4852
explicit BucketMetadataCache(std::size_t max_size = 10000)
4953
: max_size_(max_size) {}
5054

55+
static std::string NormalizeBucketName(std::string const& bucket);
56+
5157
absl::optional<BucketCacheEntry> Get(std::string const& bucket_name);
5258
void Put(std::string const& bucket_name, BucketCacheEntry entry);
5359
void Invalidate(std::string const& bucket_name);
5460
void Clear();
5561
bool StartFetch(std::string const& bucket_name);
5662
void EndFetch(std::string const& bucket_name);
5763

64+
void MaybeInvalidate(Status const& status, std::string const& bucket_name) {
65+
if (!status.ok() && status.code() == StatusCode::kNotFound) {
66+
Invalidate(bucket_name);
67+
}
68+
}
69+
70+
template <typename T>
71+
void MaybeInvalidate(StatusOr<T> const& result,
72+
std::string const& bucket_name) {
73+
MaybeInvalidate(result.status(), bucket_name);
74+
}
75+
5876
private:
5977
void MoveToFront(std::list<std::string>::iterator it);
6078

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ TEST_F(ObjectPlentyClientsSeriallyIntegrationTest, PlentyClientsSerially) {
5757

5858
auto client = MakeIntegrationTestClient(options);
5959
auto object_name = MakeRandomObjectName();
60+
6061
std::string expected = LoremIpsum();
6162

6263
StatusOr<ObjectMetadata> meta = client.InsertObject(

0 commit comments

Comments
 (0)