Skip to content

Commit f69837e

Browse files
sushanbsushanb
committed
address review comments
1 parent 30c17d5 commit f69837e

13 files changed

Lines changed: 355 additions & 319 deletions

File tree

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/csm/Metrics.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ public interface Metrics extends Closeable {
3232
@Nullable
3333
ChannelPoolMetricsTracer getChannelPoolMetricsTracer();
3434

35-
@Nullable
3635
DirectPathCompatibleTracer getDirectPathCompatibleTracer();
3736

3837
void start();

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/csm/MetricsImpl.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import com.google.cloud.bigtable.data.v2.internal.csm.tracers.CompositeTracerFactory;
3434
import com.google.cloud.bigtable.data.v2.internal.csm.tracers.DefaultDirectPathCompatibleTracer;
3535
import com.google.cloud.bigtable.data.v2.internal.csm.tracers.DirectPathCompatibleTracer;
36+
import com.google.cloud.bigtable.data.v2.internal.csm.tracers.NoopDirectPathCompatibleTracer;
3637
import com.google.cloud.bigtable.data.v2.internal.csm.tracers.Pacemaker;
3738
import com.google.common.base.Preconditions;
3839
import com.google.common.collect.ImmutableList;
@@ -114,7 +115,7 @@ public MetricsImpl(
114115
this.grpcOtel = null;
115116
this.pacemaker = null;
116117
this.channelPoolMetricsTracer = null;
117-
this.directPathCompatibleTracer = null;
118+
this.directPathCompatibleTracer = NoopDirectPathCompatibleTracer.INSTANCE;
118119
}
119120

120121
if (userOtel != null) {

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/csm/tracers/NoopDirectPathCompatibleTracer.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
* Copyright 2026 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.google.cloud.bigtable.data.v2.internal.csm.tracers;
17+
18+
import com.google.api.core.InternalApi;
19+
20+
@InternalApi
21+
public class NoopDirectPathCompatibleTracer implements DirectPathCompatibleTracer {
22+
23+
public static final NoopDirectPathCompatibleTracer INSTANCE =
24+
new NoopDirectPathCompatibleTracer();
25+
26+
private NoopDirectPathCompatibleTracer() {}
27+
28+
@Override
29+
public void recordSuccess(String ipPreference) {
30+
// No-op
31+
}
32+
33+
@Override
34+
public void recordFailure(String reason) {
35+
// No-op
36+
}
37+
}

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/UnaryDirectAccessChecker.java renamed to google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/dp/ClassicDirectAccessChecker.java

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,19 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package com.google.cloud.bigtable.data.v2.stub;
16+
package com.google.cloud.bigtable.data.v2.internal.dp;
1717

1818
import com.google.api.core.InternalApi;
1919
import com.google.bigtable.v2.PeerInfo;
2020
import com.google.cloud.bigtable.data.v2.internal.csm.tracers.DirectPathCompatibleTracer;
21+
import com.google.cloud.bigtable.data.v2.stub.MetadataExtractorInterceptor;
2122
import com.google.cloud.bigtable.gaxx.grpc.ChannelPrimer;
23+
import com.google.common.annotations.VisibleForTesting;
2224
import io.grpc.Channel;
2325
import io.grpc.ClientInterceptors;
2426
import io.grpc.ManagedChannel;
2527
import java.util.Optional;
28+
import java.util.function.Supplier;
2629
import java.util.logging.Level;
2730
import java.util.logging.Logger;
2831
import javax.annotation.Nullable;
@@ -32,25 +35,30 @@
3235
* inspecting the response headers.
3336
*/
3437
@InternalApi
35-
public class UnaryDirectAccessChecker implements DirectAccessChecker {
36-
private static final Logger LOG = Logger.getLogger(UnaryDirectAccessChecker.class.getName());
38+
public class ClassicDirectAccessChecker implements DirectAccessChecker {
39+
private static final Logger LOG = Logger.getLogger(ClassicDirectAccessChecker.class.getName());
3740
private final ChannelPrimer channelPrimer;
3841

39-
private UnaryDirectAccessChecker(ChannelPrimer channelPrimer) {
42+
private ClassicDirectAccessChecker(ChannelPrimer channelPrimer) {
4043
this.channelPrimer = channelPrimer;
4144
}
4245

43-
public static UnaryDirectAccessChecker create(ChannelPrimer channelPrimer) {
44-
return new UnaryDirectAccessChecker(channelPrimer);
46+
public static ClassicDirectAccessChecker create(ChannelPrimer channelPrimer) {
47+
return new ClassicDirectAccessChecker(channelPrimer);
48+
}
49+
50+
@VisibleForTesting
51+
MetadataExtractorInterceptor createInterceptor() {
52+
return new MetadataExtractorInterceptor();
4553
}
4654

4755
@Override
4856
public boolean check(
49-
BigtableChannelFactory channelFactory, @Nullable DirectPathCompatibleTracer tracer) {
57+
Supplier<ManagedChannel> channelSupplier, @Nullable DirectPathCompatibleTracer tracer) {
5058
ManagedChannel channel = null;
5159
try {
52-
channel = channelFactory.createSingleChannel();
53-
MetadataExtractorInterceptor interceptor = new MetadataExtractorInterceptor();
60+
channel = channelSupplier.get();
61+
MetadataExtractorInterceptor interceptor = createInterceptor();
5462
Channel interceptedChannel = ClientInterceptors.intercept(channel, interceptor);
5563
channelPrimer.primeChannel(interceptedChannel);
5664

@@ -64,16 +72,16 @@ public boolean check(
6472
.map(type -> type == PeerInfo.TransportType.TRANSPORT_TYPE_DIRECT_ACCESS)
6573
.orElse(false);
6674

67-
if (isEligible && tracer != null) {
75+
if (isEligible) {
6876
String ipProtocolStr =
69-
Optional.ofNullable(sidebandData)
70-
.map(MetadataExtractorInterceptor.SidebandData::getIpProtocol)
71-
.map(String::valueOf)
72-
.map(String::toLowerCase)
73-
.orElse("unknown");
77+
sidebandData.getIpProtocol() != null
78+
? sidebandData.getIpProtocol().toString().toLowerCase()
79+
: "unknown";
7480
tracer.recordSuccess(ipProtocolStr);
7581
}
82+
7683
return isEligible;
84+
7785
} catch (Exception e) {
7886
LOG.log(Level.FINE, "Failed to evaluate direct access eligibility.", e);
7987
return false;

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/DirectAccessChecker.java renamed to google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/dp/DirectAccessChecker.java

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package com.google.cloud.bigtable.data.v2.stub;
16+
package com.google.cloud.bigtable.data.v2.internal.dp;
1717

1818
import com.google.api.core.InternalApi;
1919
import com.google.cloud.bigtable.data.v2.internal.csm.tracers.DirectPathCompatibleTracer;
20+
import io.grpc.ManagedChannel;
21+
import java.util.function.Supplier;
2022
import javax.annotation.Nullable;
2123

2224
@InternalApi
@@ -25,8 +27,8 @@ public interface DirectAccessChecker {
2527
/**
2628
* Evaluates if Direct Access is available by creating a test channel.
2729
*
28-
* @param channelFactory A factory to create the test channel
30+
* @param supplier A supplier to create maybe direct access channel
2931
* @return true if the channel is eligible for Direct Access
3032
*/
31-
boolean check(BigtableChannelFactory channelFactory, @Nullable DirectPathCompatibleTracer tracer);
33+
boolean check(Supplier<ManagedChannel> supplier, @Nullable DirectPathCompatibleTracer tracer);
3234
}

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableChannelFactory.java renamed to google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableChannelSupplier.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@
1717

1818
import com.google.api.core.InternalApi;
1919
import io.grpc.ManagedChannel;
20-
import java.io.IOException;
20+
import java.util.function.Supplier;
2121

2222
@InternalApi
23-
public interface BigtableChannelFactory {
24-
ManagedChannel createSingleChannel() throws IOException;
25-
}
23+
public interface BigtableChannelSupplier extends Supplier<ManagedChannel> {}

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -641,8 +641,8 @@ private Builder() {
641641
FeatureFlags.newBuilder()
642642
.setReverseScans(true)
643643
.setLastScannedRowResponses(true)
644-
.setDirectAccessRequested(true)
645-
.setTrafficDirectorEnabled(true)
644+
.setDirectAccessRequested(DIRECT_PATH_ENABLED)
645+
.setTrafficDirectorEnabled(DIRECT_PATH_ENABLED)
646646
.setPeerInfo(true);
647647
}
648648

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/grpc/BigtableChannelPool.java‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717

1818
import com.google.api.core.InternalApi;
1919
import com.google.bigtable.v2.PeerInfo;
20-
import com.google.cloud.bigtable.data.v2.stub.BigtableChannelFactory;
2120
import com.google.cloud.bigtable.data.v2.stub.MetadataExtractorInterceptor;
2221
import com.google.cloud.bigtable.gaxx.grpc.ChannelPoolHealthChecker.ProbeResult;
2322
import com.google.common.annotations.VisibleForTesting;
@@ -67,7 +66,7 @@ public class BigtableChannelPool extends ManagedChannel implements BigtableChann
6766
private static final java.time.Duration REFRESH_PERIOD = java.time.Duration.ofMinutes(50);
6867

6968
private final BigtableChannelPoolSettings settings;
70-
private final BigtableChannelFactory channelFactory;
69+
private final Supplier<ManagedChannel> channelSupplier;
7170

7271
private final ChannelPrimer channelPrimer;
7372
private final Object entryWriteLock = new Object();
@@ -81,11 +80,11 @@ public class BigtableChannelPool extends ManagedChannel implements BigtableChann
8180

8281
public static BigtableChannelPool create(
8382
BigtableChannelPoolSettings settings,
84-
BigtableChannelFactory channelFactory,
83+
Supplier<ManagedChannel> channelSupplier,
8584
ChannelPrimer channelPrimer,
8685
ScheduledExecutorService backgroundExecutor)
8786
throws IOException {
88-
return new BigtableChannelPool(settings, channelFactory, channelPrimer, backgroundExecutor);
87+
return new BigtableChannelPool(settings, channelSupplier, channelPrimer, backgroundExecutor);
8988
}
9089

9190
/**
@@ -98,12 +97,12 @@ public static BigtableChannelPool create(
9897
@VisibleForTesting
9998
BigtableChannelPool(
10099
BigtableChannelPoolSettings settings,
101-
BigtableChannelFactory channelFactory,
100+
Supplier<ManagedChannel> channelSupplier,
102101
ChannelPrimer channelPrimer,
103102
ScheduledExecutorService executor)
104103
throws IOException {
105104
this.settings = settings;
106-
this.channelFactory = channelFactory;
105+
this.channelSupplier = channelSupplier;
107106
this.channelPrimer = channelPrimer;
108107
Clock systemClock = Clock.systemUTC();
109108
ChannelPoolHealthChecker channelPoolHealthChecker =
@@ -113,7 +112,7 @@ public static BigtableChannelPool create(
113112
ImmutableList.Builder<Entry> initialListBuilder = ImmutableList.builder();
114113

115114
for (int i = 0; i < settings.getInitialChannelCount(); i++) {
116-
ManagedChannel newChannel = channelFactory.createSingleChannel();
115+
ManagedChannel newChannel = channelSupplier.get();
117116
channelPrimer.primeChannel(newChannel);
118117
initialListBuilder.add(new Entry(newChannel));
119118
}
@@ -419,10 +418,10 @@ private void expand(int desiredSize) {
419418

420419
for (int i = 0; i < desiredSize - localEntries.size(); i++) {
421420
try {
422-
ManagedChannel newChannel = channelFactory.createSingleChannel();
421+
ManagedChannel newChannel = channelSupplier.get();
423422
this.channelPrimer.primeChannel(newChannel);
424423
newEntries.add(new Entry(newChannel));
425-
} catch (IOException e) {
424+
} catch (Exception e) {
426425
LOG.log(Level.WARNING, "Failed to add channel", e);
427426
}
428427
}
@@ -459,10 +458,10 @@ void refresh() {
459458

460459
for (int i = 0; i < newEntries.size(); i++) {
461460
try {
462-
ManagedChannel newChannel = channelFactory.createSingleChannel();
461+
ManagedChannel newChannel = channelSupplier.get();
463462
this.channelPrimer.primeChannel(newChannel);
464463
newEntries.set(i, new Entry(newChannel));
465-
} catch (IOException e) {
464+
} catch (Exception e) {
466465
LOG.log(Level.WARNING, "Failed to refresh channel, leaving old channel", e);
467466
}
468467
}

0 commit comments

Comments
 (0)