gemini-code-assist[bot] commented on code in PR #39149:
URL: https://github.com/apache/beam/pull/39149#discussion_r3491914855
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerAccessor.java:
##########
@@ -116,7 +122,24 @@ public static SpannerAccessor getOrCreate(SpannerConfig
spannerConfig) {
@VisibleForTesting
static SpannerOptions buildSpannerOptions(SpannerConfig spannerConfig) {
+ return buildSpannerOptions(spannerConfig, GlobalOpenTelemetry.get());
+ }
+
+ @VisibleForTesting
+ static SpannerOptions buildSpannerOptions(SpannerConfig spannerConfig,
OpenTelemetry otel) {
SpannerOptions.Builder builder = SpannerOptions.newBuilder();
+ if (otel != null) {
+ builder.setOpenTelemetry(otel);
+ }
+ if (spannerConfig.getEnableOpenTelemetryTracing() != null
+ && spannerConfig.getEnableOpenTelemetryTracing().get()) {
Review Comment:
Calling `.get()` on a `ValueProvider` without checking `isAccessible()` can
throw an `IllegalStateException` if it is a `RuntimeValueProvider` (e.g., when
using templates) and accessed during pipeline construction time (such as inside
`SpannerIO.read().expand()`). Additionally, if `get()` returns `null`, it will
cause a `NullPointerException` due to auto-unboxing to `boolean` in the `if`
condition.
We should check `isAccessible()` and use `Boolean.TRUE.equals()` to safely
handle null values.
```suggestion
org.apache.beam.sdk.options.ValueProvider<Boolean> enableOtelTracing =
spannerConfig.getEnableOpenTelemetryTracing();
if (enableOtelTracing != null
&& enableOtelTracing.isAccessible()
&& Boolean.TRUE.equals(enableOtelTracing.get())) {
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/dofn/ReadChangeStreamPartitionDoFn.java:
##########
@@ -195,10 +197,11 @@ public ReadChangeStreamPartitionRangeTracker newTracker(
* ChangeStreamRecordMapper}, {@link PartitionMetadataMapper}, {@link
DataChangeRecordAction},
* {@link HeartbeatRecordAction}, {@link ChildPartitionsRecordAction}, {@link
* PartitionStartRecordAction}, {@link PartitionEndRecordAction}, {@link
- * PartitionEventRecordAction} and {@link QueryChangeStreamAction}.
+ * {@link PartitionEventRecordAction} and {@link QueryChangeStreamAction}.
Review Comment:
This change introduces a duplicate/nested `{@link` tag because the previous
line (line 199) already ends with `{@link`. This will result in invalid Javadoc
syntax (i.e., `{@link {@link PartitionEventRecordAction}`). We should revert
this line to start directly with `PartitionEventRecordAction}`.
```suggestion
* PartitionEventRecordAction} and {@link QueryChangeStreamAction}.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
