Skip to content

Commit 57e1541

Browse files
scotthartscotthart
committed
fix clang-tidy and more refactoring
1 parent 0b27844 commit 57e1541

3 files changed

Lines changed: 78 additions & 81 deletions

File tree

‎google/cloud/bigtable/internal/directpath_prober.cc‎

Lines changed: 35 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -114,51 +114,45 @@ StatusOr<DirectPathProbeResult> DirectPathProber::Probe(
114114
std::shared_ptr<internal::GrpcAuthenticationStrategy> const& auth,
115115
bigtable::InstanceResource const& instance_resource, Options const& options,
116116
CompletionQueue const& cq) {
117-
if (!auth) {
118-
return internal::InternalError("Auth strategy cannot be null",
119-
GCP_ERROR_INFO());
120-
}
121-
122-
auto constexpr kDirectPathEndpoint = "google-c2p:///bigtable.googleapis.com";
123-
auto constexpr kAuthority = "bigtable.googleapis.com";
124-
125-
Options probe_options = options;
126-
probe_options.set<::google::cloud::bigtable_internal::DataEndpointOption>(
127-
kDirectPathEndpoint);
128-
probe_options.set<EndpointOption>(kDirectPathEndpoint);
129-
probe_options.set<AuthorityOption>(kAuthority);
130-
probe_options.set<bigtable::experimental::DirectPathModeOption>(
131-
bigtable::experimental::DirectPathMode::kEnabled);
132-
probe_options.set<GrpcNumChannelsOption>(1);
133-
probe_options.set<bigtable::MinConnectionRefreshOption>(
134-
std::chrono::milliseconds::zero());
135-
probe_options.set<bigtable::MaxConnectionRefreshOption>(
136-
std::chrono::milliseconds::zero());
137-
138-
auto stub_factory = [](std::shared_ptr<grpc::Channel> channel) {
139-
return std::make_shared<DefaultBigtableStub>(
140-
google::bigtable::v2::Bigtable::NewStub(std::move(channel)));
141-
};
142-
std::shared_ptr<BigtableStub> stub =
143-
CreateDecoratedStubs(auth, cq, probe_options, stub_factory);
144-
145-
return Probe(stub, instance_resource, probe_options);
146-
}
147-
148-
StatusOr<DirectPathProbeResult> DirectPathProber::Probe(
149-
std::shared_ptr<BigtableStub> stub,
150-
bigtable::InstanceResource const& instance_resource,
151-
Options const& options) {
152-
return Probe(std::move(stub), instance_resource, options, "", nullptr);
117+
return Probe(auth, instance_resource, options, cq, nullptr, "", nullptr);
153118
}
154119

155120
StatusOr<DirectPathProbeResult> DirectPathProber::Probe(
156-
std::shared_ptr<BigtableStub> stub,
121+
std::shared_ptr<internal::GrpcAuthenticationStrategy> const& auth,
157122
bigtable::InstanceResource const& instance_resource, Options const& options,
123+
CompletionQueue const& cq, std::shared_ptr<BigtableStub> const& stub,
158124
std::string const& peer_address,
159-
std::shared_ptr<grpc::AuthContext const> auth_context) {
160-
if (!stub) {
161-
return internal::InternalError("Stub cannot be null", GCP_ERROR_INFO());
125+
std::shared_ptr<grpc::AuthContext const> const& auth_context) {
126+
std::shared_ptr<BigtableStub> effective_stub = stub;
127+
if (!effective_stub) {
128+
if (!auth) {
129+
return internal::InternalError("Auth strategy cannot be null",
130+
GCP_ERROR_INFO());
131+
}
132+
133+
auto constexpr kDirectPathEndpoint =
134+
"google-c2p:///bigtable.googleapis.com";
135+
auto constexpr kAuthority = "bigtable.googleapis.com";
136+
137+
Options probe_options = options;
138+
probe_options.set<::google::cloud::bigtable_internal::DataEndpointOption>(
139+
kDirectPathEndpoint);
140+
probe_options.set<EndpointOption>(kDirectPathEndpoint);
141+
probe_options.set<AuthorityOption>(kAuthority);
142+
probe_options.set<bigtable::experimental::DirectPathModeOption>(
143+
bigtable::experimental::DirectPathMode::kEnabled);
144+
probe_options.set<GrpcNumChannelsOption>(1);
145+
probe_options.set<bigtable::MinConnectionRefreshOption>(
146+
std::chrono::milliseconds::zero());
147+
probe_options.set<bigtable::MaxConnectionRefreshOption>(
148+
std::chrono::milliseconds::zero());
149+
150+
auto stub_factory = [](std::shared_ptr<grpc::Channel> channel) {
151+
return std::make_shared<DefaultBigtableStub>(
152+
google::bigtable::v2::Bigtable::NewStub(std::move(channel)));
153+
};
154+
effective_stub =
155+
CreateDecoratedStubs(auth, cq, probe_options, stub_factory);
162156
}
163157

164158
std::chrono::milliseconds timeout =
@@ -177,7 +171,7 @@ StatusOr<DirectPathProbeResult> DirectPathProber::Probe(
177171
}
178172
OperationContext op_ctx;
179173
StatusOr<google::bigtable::v2::PingAndWarmResponse> response =
180-
stub->PingAndWarm(client_context, options, request, op_ctx);
174+
effective_stub->PingAndWarm(client_context, options, request, op_ctx);
181175
if (!response.ok()) return response.status();
182176

183177
DirectPathProbeResult result;

‎google/cloud/bigtable/internal/directpath_prober.h‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,16 +55,12 @@ class DirectPathProber {
5555

5656
/// For testing only.
5757
static StatusOr<DirectPathProbeResult> Probe(
58-
std::shared_ptr<BigtableStub> stub,
59-
bigtable::InstanceResource const& instance_resource,
60-
Options const& options);
61-
62-
/// For testing only.
63-
static StatusOr<DirectPathProbeResult> Probe(
64-
std::shared_ptr<BigtableStub> stub,
58+
std::shared_ptr<internal::GrpcAuthenticationStrategy> const& auth,
6559
bigtable::InstanceResource const& instance_resource,
66-
Options const& options, std::string const& peer_address,
67-
std::shared_ptr<grpc::AuthContext const> auth_context);
60+
Options const& options, CompletionQueue const& cq,
61+
std::shared_ptr<BigtableStub> const& stub,
62+
std::string const& peer_address,
63+
std::shared_ptr<grpc::AuthContext const> const& auth_context);
6864
};
6965

7066
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END

‎google/cloud/bigtable/internal/directpath_prober_test.cc‎

Lines changed: 38 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -229,10 +229,7 @@ TEST(DirectPathProberTest, ProbeReturnsErrorWhenRpcFails) {
229229
HasSubstr("service unavailable")));
230230
}
231231

232-
class FakeAuthPropertyIterator : public grpc::AuthPropertyIterator {
233-
public:
234-
FakeAuthPropertyIterator() : grpc::AuthPropertyIterator() {}
235-
};
232+
class FakeAuthPropertyIterator : public grpc::AuthPropertyIterator {};
236233

237234
class FakeAuthContext : public grpc::AuthContext {
238235
public:
@@ -249,7 +246,8 @@ class FakeAuthContext : public grpc::AuthContext {
249246
std::string const& name) const override {
250247
if (name == "transport_security_type") {
251248
std::vector<grpc::string_ref> res;
252-
for (auto const& st : transport_security_types_) {
249+
res.reserve(transport_security_types_.size());
250+
for (std::string const& st : transport_security_types_) {
253251
res.emplace_back(st.data(), st.size());
254252
}
255253
return res;
@@ -286,8 +284,9 @@ TEST(DirectPathProberTest, ProbeWithAltsNegotiatedIpv4) {
286284
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"alts"});
287285
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
288286

289-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
290-
mock_stub, instance, Options{}, "ipv4:34.126.1.1:443", auth_ctx);
287+
StatusOr<DirectPathProbeResult> const result =
288+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
289+
mock_stub, "ipv4:34.126.1.1:443", auth_ctx);
291290

292291
ASSERT_THAT(result, IsOk());
293292
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(true)),
@@ -309,8 +308,9 @@ TEST(DirectPathProberTest, ProbeWithAltsNegotiatedIpv6) {
309308
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"alts"});
310309
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
311310

312-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
313-
mock_stub, instance, Options{}, "ipv6:[2001:db8::1]:443", auth_ctx);
311+
StatusOr<DirectPathProbeResult> const result =
312+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
313+
mock_stub, "ipv6:[2001:db8::1]:443", auth_ctx);
314314

315315
ASSERT_THAT(result, IsOk());
316316
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(true)),
@@ -332,9 +332,9 @@ TEST(DirectPathProberTest, ProbeWithAltsNegotiatedUnknownPeer) {
332332
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"alts"});
333333
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
334334

335-
StatusOr<DirectPathProbeResult> const result =
336-
DirectPathProber::Probe(mock_stub, instance, Options{},
337-
"dns:///bigtable.googleapis.com", auth_ctx);
335+
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
336+
nullptr, instance, Options{}, CompletionQueue{}, mock_stub,
337+
"dns:///bigtable.googleapis.com", auth_ctx);
338338

339339
ASSERT_THAT(result, IsOk());
340340
EXPECT_THAT(
@@ -357,8 +357,9 @@ TEST(DirectPathProberTest, ProbeWithPeerAuthenticatedDirectPathIpv4) {
357357
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"ssl"});
358358
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
359359

360-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
361-
mock_stub, instance, Options{}, "ipv4:34.126.0.1:443", auth_ctx);
360+
StatusOr<DirectPathProbeResult> const result =
361+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
362+
mock_stub, "ipv4:34.126.0.1:443", auth_ctx);
362363

363364
ASSERT_THAT(result, IsOk());
364365
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(true)),
@@ -380,8 +381,9 @@ TEST(DirectPathProberTest, ProbeWithPeerAuthenticatedDirectPathIpv4MaxSubnet) {
380381
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"ssl"});
381382
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
382383

383-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
384-
mock_stub, instance, Options{}, "ipv4:34.126.63.255:443", auth_ctx);
384+
StatusOr<DirectPathProbeResult> const result =
385+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
386+
mock_stub, "ipv4:34.126.63.255:443", auth_ctx);
385387

386388
ASSERT_THAT(result, IsOk());
387389
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(true)),
@@ -403,9 +405,9 @@ TEST(DirectPathProberTest, ProbeWithPeerAuthenticatedDirectPathIpv6) {
403405
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"ssl"});
404406
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
405407

406-
StatusOr<DirectPathProbeResult> const result =
407-
DirectPathProber::Probe(mock_stub, instance, Options{},
408-
"ipv6:[2607:f8b0:4000:800::200a]:443", auth_ctx);
408+
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
409+
nullptr, instance, Options{}, CompletionQueue{}, mock_stub,
410+
"ipv6:[2607:f8b0:4000:800::200a]:443", auth_ctx);
409411

410412
ASSERT_THAT(result, IsOk());
411413
EXPECT_THAT(
@@ -428,8 +430,9 @@ TEST(DirectPathProberTest, ProbeWithPeerAuthenticatedNonDirectPathIpv4) {
428430
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"ssl"});
429431
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
430432

431-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
432-
mock_stub, instance, Options{}, "ipv4:34.126.64.1:443", auth_ctx);
433+
StatusOr<DirectPathProbeResult> const result =
434+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
435+
mock_stub, "ipv4:34.126.64.1:443", auth_ctx);
433436

434437
ASSERT_THAT(result, IsOk());
435438
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(false)),
@@ -451,8 +454,9 @@ TEST(DirectPathProberTest, ProbeWithPeerAuthenticatedDifferentOctetIpv4) {
451454
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"ssl"});
452455
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
453456

454-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
455-
mock_stub, instance, Options{}, "ipv4:34.125.1.1:443", auth_ctx);
457+
StatusOr<DirectPathProbeResult> const result =
458+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
459+
mock_stub, "ipv4:34.125.1.1:443", auth_ctx);
456460

457461
ASSERT_THAT(result, IsOk());
458462
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(false)),
@@ -474,8 +478,9 @@ TEST(DirectPathProberTest, ProbeWithPeerAuthenticatedDifferentFirstOctetIpv4) {
474478
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"ssl"});
475479
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
476480

477-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
478-
mock_stub, instance, Options{}, "ipv4:35.126.1.1:443", auth_ctx);
481+
StatusOr<DirectPathProbeResult> const result =
482+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
483+
mock_stub, "ipv4:35.126.1.1:443", auth_ctx);
479484

480485
ASSERT_THAT(result, IsOk());
481486
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(false)),
@@ -497,8 +502,9 @@ TEST(DirectPathProberTest, ProbeWithPeerAuthenticatedInvalidIpv4) {
497502
std::make_shared<FakeAuthContext>(true, std::vector<std::string>{"ssl"});
498503
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
499504

500-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
501-
mock_stub, instance, Options{}, "ipv4:invalid:format", auth_ctx);
505+
StatusOr<DirectPathProbeResult> const result =
506+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
507+
mock_stub, "ipv4:invalid:format", auth_ctx);
502508

503509
ASSERT_THAT(result, IsOk());
504510
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(false)),
@@ -520,8 +526,9 @@ TEST(DirectPathProberTest, ProbeWithPeerUnauthenticated) {
520526
false, std::vector<std::string>{"alts"});
521527
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
522528

523-
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
524-
mock_stub, instance, Options{}, "ipv4:34.126.1.1:443", auth_ctx);
529+
StatusOr<DirectPathProbeResult> const result =
530+
DirectPathProber::Probe(nullptr, instance, Options{}, CompletionQueue{},
531+
mock_stub, "ipv4:34.126.1.1:443", auth_ctx);
525532

526533
ASSERT_THAT(result, IsOk());
527534
EXPECT_THAT(*result, AllOf(ProbeSuccess(Eq(false)),
@@ -531,8 +538,8 @@ TEST(DirectPathProberTest, ProbeWithPeerUnauthenticated) {
531538

532539
TEST(DirectPathProberTest, ProbeNullStubFails) {
533540
bigtable::InstanceResource const instance(Project("test-proj"), "test-inst");
534-
StatusOr<DirectPathProbeResult> const result =
535-
DirectPathProber::Probe(nullptr, instance, Options{});
541+
StatusOr<DirectPathProbeResult> const result = DirectPathProber::Probe(
542+
nullptr, instance, Options{}, CompletionQueue{}, nullptr, "", nullptr);
536543
EXPECT_THAT(result, StatusIs(StatusCode::kInternal));
537544
}
538545

0 commit comments

Comments
 (0)