Conversation
product-auto-label
bot
added
size: l
...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/csm/MetricsImpl.java
Outdated
Show resolved
Hide resolved
...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/DirectAccessChecker.java
Outdated
Show resolved
Hide resolved
...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java
Outdated
Show resolved
Hide resolved
...-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/UnaryDirectAccessChecker.java
Outdated
Show resolved
Hide resolved
...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableChannelFactory.java
Outdated
Show resolved
Hide resolved
...-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/UnaryDirectAccessChecker.java
Outdated
Show resolved
Hide resolved
sushanb
changed the title
nimf
left a comment
nimf
left a comment
There was a problem hiding this comment.
First comments from the first pass. Will do another pass soon.
...able/src/main/java/com/google/cloud/bigtable/gaxx/grpc/BigtableTransportChannelProvider.java
Show resolved
Hide resolved
...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void recordSuccess(String ipPreference) { |
There was a problem hiding this comment.
nit, can this be an enum?
|
|
||
| boolean isEligible = | ||
| Optional.ofNullable(sidebandData) | ||
| .map(MetadataExtractorInterceptor.SidebandData::getPeerInfo) |
There was a problem hiding this comment.
getPeerInfo is marked Nullable, so this could cause an npe
| sidebandData.getIpProtocol() != null | ||
| ? sidebandData.getIpProtocol().toString().toLowerCase() | ||
| : "unknown"; |
There was a problem hiding this comment.
can this formatting move to com.google.cloud.bigtable.data.v2.internal.csm.attributes.Util?
| return isEligible; | ||
|
|
||
| } catch (Exception e) { | ||
| LOG.log(Level.FINE, "Failed to evaluate direct access eligibility.", e); |
There was a problem hiding this comment.
probably a WARN if it threw an exception
| return this; | ||
| } | ||
|
|
||
| public Builder setEnableDirectPathByDefault(boolean enableDirectPathByDefault) { |
There was a problem hiding this comment.
do you want endusers to be calling this? I suspect we will want to remove this
| } | ||
|
|
||
| @Override | ||
| public boolean check(Supplier<ManagedChannel> supplier) { |
There was a problem hiding this comment.
Im confused why this has to be abstracted to a supplier? why cant this just be a Channel?
| /** Applies common pool, message size, and keep-alive settings to the provided builder. */ | ||
| private static InstantiatingGrpcChannelProvider.Builder commonTraits( | ||
| InstantiatingGrpcChannelProvider.Builder builder) { | ||
| return builder |
There was a problem hiding this comment.
why does this method still exist?
| private Builder() { | ||
| // TODO(enable this by default) | ||
| // Only runs Direct Access Checker if user explicitly sets CBT_ENABLE_DIRECTPATH=true | ||
| this.enableDirectPathByDefault = Boolean.parseBoolean(System.getenv("CBT_ENABLE_DIRECTPATH")); |
There was a problem hiding this comment.
Can leave the env logic at the top of the file and just reference its result here?
|
|
||
| // TODO(meeral-k): add documentation | ||
| private static final boolean DIRECT_PATH_ENABLED = | ||
| Boolean.parseBoolean(System.getenv("CBT_ENABLE_DIRECTPATH")); | ||
|
|
There was a problem hiding this comment.
can we keep the env var a constant at the top of the file so we dont have to looking for it? ie
boolean DIRECT_PATH_ENABLED =
Optional.ofNullable(System.getenv("CBT_ENABLE_DIRECTPATH"))
.map(Boolean::parseBoolean)
// TODO change this to false when enabling directpath by default
.orElse(false);
| directPathCompatibleTracer, | ||
| enableDirectAccessChecker); |
There was a problem hiding this comment.
can this just be an Optional<DirectPathCompa...>? instead of parallel variables?
3 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.