Skip to content

test(spanner): unflake SpannerStandaloneExamplesIT.addNumericColumn_shouldSuccessfullyAddColumn #14945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sakthivelmanii wants to merge 3 commits into
base: main
Choose a base branch
from
Open

test(spanner): unflake SpannerStandaloneExamplesIT.addNumericColumn_shouldSuccessfullyAddColumn #14945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6338705
test(spanner): unflake SpannerStandaloneExamplesIT.addNumericColumn_s…
sakthivelmanii Nov 8, 2026
a98fe78
fix(review): address automated code review feedback on getErrorCode a…
sakthivelmanii Nov 8, 2026
c0d9b49
Addressed comments
sakthivelmanii Nov 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.cloud.grpc.GrpcTransportOptions;
import com.google.cloud.spanner.SessionClient.SessionId;
import com.google.cloud.spanner.SpannerOptions.CloseableExecutorProvider;
import com.google.cloud.spanner.admin.database.v1.stub.DatabaseAdminStub;
import com.google.cloud.spanner.admin.database.v1.stub.DatabaseAdminStubSettings;
import com.google.cloud.spanner.admin.instance.v1.stub.InstanceAdminStubSettings;
import com.google.cloud.spanner.spi.v1.GapicSpannerRpc;
Expand Down Expand Up @@ -267,8 +268,11 @@ public DatabaseAdminClient getDatabaseAdminClient() {
try {
final DatabaseAdminStubSettings settings =
Preconditions.checkNotNull(gapicRpc.getDatabaseAdminStubSettings());
return com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient.create(
settings.createStub());
final DatabaseAdminStub stub =
gapicRpc instanceof GapicSpannerRpc
? ((GapicSpannerRpc) gapicRpc).createDatabaseAdminStub()
: settings.createStub();
return com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient.create(stub);
} catch (IOException ex) {
throw SpannerExceptionFactory.newSpannerException(ex);
}
Expand Down
265 changes: 226 additions & 39 deletions ...r/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,40 +23,54 @@

import com.google.api.core.ApiFunction;
import com.google.api.core.ApiFuture;
import com.google.api.core.ApiFutures;
import com.google.api.core.InternalApi;
import com.google.api.core.NanoClock;
import com.google.api.gax.core.CredentialsProvider;
import com.google.api.gax.core.GaxProperties;
import com.google.api.gax.grpc.GaxGrpcProperties;
import com.google.api.gax.grpc.GrpcCallContext;
import com.google.api.gax.grpc.GrpcCallSettings;
import com.google.api.gax.grpc.GrpcCallableFactory;
import com.google.api.gax.grpc.GrpcStatusCode;
import com.google.api.gax.grpc.GrpcStubCallableFactory;
import com.google.api.gax.grpc.GrpcTransportChannel;
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
import com.google.api.gax.longrunning.OperationFuture;
import com.google.api.gax.longrunning.OperationSnapshot;
import com.google.api.gax.retrying.ResultRetryAlgorithm;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.retrying.TimedAttemptSettings;
import com.google.api.gax.rpc.AlreadyExistsException;
import com.google.api.gax.rpc.ApiCallContext;
import com.google.api.gax.rpc.ApiClientHeaderProvider;
import com.google.api.gax.rpc.ApiException;
import com.google.api.gax.rpc.Callables;
import com.google.api.gax.rpc.ClientContext;
import com.google.api.gax.rpc.ErrorDetails;
import com.google.api.gax.rpc.FixedHeaderProvider;
import com.google.api.gax.rpc.HeaderProvider;
import com.google.api.gax.rpc.InstantiatingWatchdogProvider;
import com.google.api.gax.rpc.LibraryMetadata;
import com.google.api.gax.rpc.LongRunningClient;
import com.google.api.gax.rpc.OperationCallSettings;
import com.google.api.gax.rpc.OperationCallable;
import com.google.api.gax.rpc.ResponseObserver;
import com.google.api.gax.rpc.ServerStream;
import com.google.api.gax.rpc.StatusCode;
import com.google.api.gax.rpc.StatusCode.Code;
import com.google.api.gax.rpc.StreamController;
import com.google.api.gax.rpc.TranslatingUnaryCallable;
import com.google.api.gax.rpc.TransportChannel;
import com.google.api.gax.rpc.TransportChannelProvider;
import com.google.api.gax.rpc.UnaryCallSettings;
import com.google.api.gax.rpc.UnaryCallable;
import com.google.api.gax.rpc.UnavailableException;
import com.google.api.gax.rpc.WatchdogProvider;
import com.google.api.gax.tracing.ApiTracerContext;
import com.google.api.gax.tracing.SpanName;
import com.google.api.gax.tracing.TracedOperationCallable;
import com.google.api.gax.tracing.TracedOperationInitialCallable;
import com.google.api.pathtemplate.PathTemplate;
import com.google.auth.Credentials;
import com.google.cloud.RetryHelper;
Expand Down Expand Up @@ -87,7 +101,6 @@
import com.google.cloud.spanner.admin.database.v1.stub.DatabaseAdminStub;
import com.google.cloud.spanner.admin.database.v1.stub.DatabaseAdminStubSettings;
import com.google.cloud.spanner.admin.database.v1.stub.GrpcDatabaseAdminCallableFactory;
import com.google.cloud.spanner.admin.database.v1.stub.GrpcDatabaseAdminStub;
import com.google.cloud.spanner.admin.instance.v1.stub.GrpcInstanceAdminStub;
import com.google.cloud.spanner.admin.instance.v1.stub.InstanceAdminStub;
import com.google.cloud.spanner.admin.instance.v1.stub.InstanceAdminStubSettings;
Expand All @@ -102,6 +115,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.io.Resources;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.RateLimiter;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.iam.v1.GetIamPolicyRequest;
Expand All @@ -111,9 +125,11 @@
import com.google.iam.v1.TestIamPermissionsRequest;
import com.google.iam.v1.TestIamPermissionsResponse;
import com.google.longrunning.CancelOperationRequest;
import com.google.longrunning.DeleteOperationRequest;
import com.google.longrunning.GetOperationRequest;
import com.google.longrunning.Operation;
import com.google.longrunning.OperationsGrpc;
import com.google.longrunning.stub.OperationsStub;
import com.google.protobuf.Empty;
import com.google.protobuf.FieldMask;
import com.google.protobuf.InvalidProtocolBufferException;
Expand Down Expand Up @@ -204,6 +220,7 @@
import io.grpc.ManagedChannel;
import io.grpc.ManagedChannelBuilder;
import io.grpc.MethodDescriptor;
import io.grpc.Status;
import io.grpc.auth.MoreCallCredentials;
import io.opentelemetry.api.OpenTelemetry;
import java.io.IOException;
Expand All @@ -222,6 +239,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.Callable;
import java.util.concurrent.CancellationException;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -560,44 +578,7 @@ public GapicSpannerRpc(final SpannerOptions options) {
/* isAdminClient= */ true, isEmulatorEnabled(options, emulatorHost)))
.build();

// Automatically retry RESOURCE_EXHAUSTED for GetOperation if auto-throttling of
// administrative requests has been set. The GetOperation RPC is called repeatedly by gax
// while polling long-running operations for their progress and can also cause these errors.
// The default behavior is not to retry these errors, and this option should normally only
// be enabled for (integration) testing.
if (options.isAutoThrottleAdministrativeRequests()) {
GrpcStubCallableFactory factory =
new GrpcDatabaseAdminCallableFactory() {
@Override
public <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> createUnaryCallable(
GrpcCallSettings<RequestT, ResponseT> grpcCallSettings,
UnaryCallSettings<RequestT, ResponseT> callSettings,
ClientContext clientContext) {
// Make GetOperation retry on RESOURCE_EXHAUSTED to prevent polling operations
// from failing with an Administrative requests limit exceeded error.
if (grpcCallSettings
.getMethodDescriptor()
.getFullMethodName()
.equals("google.longrunning.Operations/GetOperation")) {
Set<StatusCode.Code> codes =
ImmutableSet.<StatusCode.Code>builderWithExpectedSize(
callSettings.getRetryableCodes().size() + 1)
.addAll(callSettings.getRetryableCodes())
.add(StatusCode.Code.RESOURCE_EXHAUSTED)
.build();
callSettings = callSettings.toBuilder().setRetryableCodes(codes).build();
}
return super.createUnaryCallable(grpcCallSettings, callSettings, clientContext);
}
};
this.databaseAdminStub =
new GrpcDatabaseAdminStubWithCustomCallableFactory(
databaseAdminStubSettings,
ClientContext.create(databaseAdminStubSettings),
factory);
} else {
this.databaseAdminStub = GrpcDatabaseAdminStub.create(databaseAdminStubSettings);
}
this.databaseAdminStub = createDatabaseAdminStub();

// Check whether the SPANNER_EMULATOR_HOST env var has been set, and if so, if the emulator
// is actually running.
Expand Down Expand Up @@ -2771,6 +2752,212 @@ public DatabaseAdminStubSettings getDatabaseAdminStubSettings() {
return databaseAdminStubSettings;
}

public DatabaseAdminStub createDatabaseAdminStub() throws IOException {
Preconditions.checkNotNull(databaseAdminStubSettings);
return new GrpcDatabaseAdminStubWithCustomCallableFactory(
databaseAdminStubSettings,
ClientContext.create(databaseAdminStubSettings),
createDatabaseAdminCallableFactory(throttleAdministrativeRequests));
}

private static GrpcStubCallableFactory createDatabaseAdminCallableFactory(
final boolean throttleAdministrativeRequests) {
return new GrpcDatabaseAdminCallableFactory() {
@Override
public <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> createUnaryCallable(
GrpcCallSettings<RequestT, ResponseT> grpcCallSettings,
UnaryCallSettings<RequestT, ResponseT> callSettings,
ClientContext clientContext) {
// Make GetOperation retry on RESOURCE_EXHAUSTED to prevent polling operations
// from failing with an Administrative requests limit exceeded error.
if (throttleAdministrativeRequests
&& grpcCallSettings
.getMethodDescriptor()
.getFullMethodName()
.equals("google.longrunning.Operations/GetOperation")) {
Set<StatusCode.Code> codes =
ImmutableSet.<StatusCode.Code>builderWithExpectedSize(
callSettings.getRetryableCodes().size() + 1)
.addAll(callSettings.getRetryableCodes())
.add(StatusCode.Code.RESOURCE_EXHAUSTED)
.build();
callSettings = callSettings.toBuilder().setRetryableCodes(codes).build();
}
return super.createUnaryCallable(grpcCallSettings, callSettings, clientContext);
}

@Override
@SuppressWarnings("unchecked")
public <RequestT, ResponseT, MetadataT>
OperationCallable<RequestT, ResponseT, MetadataT> createOperationCallable(
GrpcCallSettings<RequestT, Operation> grpcCallSettings,
OperationCallSettings<RequestT, ResponseT, MetadataT> callSettings,
ClientContext clientContext,
OperationsStub operationsStub) {
if (grpcCallSettings
.getMethodDescriptor()
.getFullMethodName()
.equals(DatabaseAdminGrpc.getUpdateDatabaseDdlMethod().getFullMethodName())) {
return (OperationCallable<RequestT, ResponseT, MetadataT>)
createIdempotentUpdateDatabaseDdlCallable(
(GrpcCallSettings<UpdateDatabaseDdlRequest, Operation>) grpcCallSettings,
(OperationCallSettings<
UpdateDatabaseDdlRequest, Empty, UpdateDatabaseDdlMetadata>)
callSettings,
clientContext,
operationsStub);
}
return super.createOperationCallable(
grpcCallSettings, callSettings, clientContext, operationsStub);
}
};
}

/**
* Ensures retries of {@code UpdateDatabaseDdl} are idempotent when the caller omits {@code
* operation_id}. Generates a client-side {@code operation_id} per logical call and resumes the
* existing operation via {@code GetOperation} if a retry fails with {@code ALREADY_EXISTS} after
* the initial attempt already started on the server.
*/
private static OperationCallable<UpdateDatabaseDdlRequest, Empty, UpdateDatabaseDdlMetadata>
createIdempotentUpdateDatabaseDdlCallable(
GrpcCallSettings<UpdateDatabaseDdlRequest, Operation> grpcCallSettings,
OperationCallSettings<UpdateDatabaseDdlRequest, Empty, UpdateDatabaseDdlMetadata>
callSettings,
ClientContext clientContext,
OperationsStub operationsStub) {
ApiTracerContext apiTracerContext =
ApiTracerContext.newBuilder()
.setFullMethodName(grpcCallSettings.getMethodDescriptor().getFullMethodName())
.setTransport(ApiTracerContext.Transport.GRPC)
.setLibraryMetadata(LibraryMetadata.empty())
.build();
SpanName initialSpanName = SpanName.of(apiTracerContext);

@gemini-code-assist gemini-code-assist Bot Nov 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The SpanName class in com.google.api.gax.tracing does not have an overload of of that accepts ApiTracerContext. This will cause a compilation error. Instead, you should use SpanName.of(String, String) or construct the SpanName using the client and method names directly.

Suggested change
SpanName initialSpanName = SpanName.of(apiTracerContext);
SpanName initialSpanName = SpanName.of("DatabaseAdminClient", "UpdateDatabaseDdl");

SpanName operationSpanName =
SpanName.of(initialSpanName.getClientName(), initialSpanName.getMethodName() + "Operation");

final UnaryCallable<UpdateDatabaseDdlRequest, Operation> baseInitialCallable =
GrpcCallableFactory.createBaseUnaryCallable(
grpcCallSettings, callSettings.getInitialCallSettings(), clientContext);

UnaryCallable<UpdateDatabaseDdlRequest, OperationSnapshot> initialSnapshotCallable =
new UnaryCallable<UpdateDatabaseDdlRequest, OperationSnapshot>() {
@Override
public ApiFuture<OperationSnapshot> futureCall(
UpdateDatabaseDdlRequest request, ApiCallContext context) {
final ApiFuture<Operation> operationFuture;
if (request.getOperationId().isEmpty()) {
final String operationId = "r" + UUID.randomUUID().toString().replace("-", "_");

@gemini-code-assist gemini-code-assist Bot Nov 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The Spanner production service enforces a strict length limit of 30 characters for the operation_id in UpdateDatabaseDdlRequest (must be between 2 and 30 characters, matching [a-z][a-z0-9_]*).

The current implementation generates an ID of 37 characters (1 for 'r' + 36 for the UUID with underscores), which will cause all requests without an explicit operation ID to fail with INVALID_ARGUMENT in production.

To fix this, we should limit the length of the generated ID (e.g., by taking a substring of the UUID) to ensure it is within the 30-character limit while retaining sufficient entropy.

Suggested change
final String operationId = "r" + UUID.randomUUID().toString().replace("-", "_");
final String operationId =
"r" + UUID.randomUUID().toString().replace("-", "").substring(0, 20);

final UpdateDatabaseDdlRequest requestWithOperationId =
request.toBuilder().setOperationId(operationId).build();
final String operationName = request.getDatabase() + "/operations/" + operationId;
ApiFuture<Operation> attemptFuture =
baseInitialCallable.futureCall(requestWithOperationId, context);
operationFuture =
ApiFutures.catchingAsync(
attemptFuture,
AlreadyExistsException.class,
ignored ->
operationsStub
.getOperationCallable()
.futureCall(
GetOperationRequest.newBuilder().setName(operationName).build(),
context),
MoreExecutors.directExecutor());
} else {
operationFuture = baseInitialCallable.futureCall(request, context);
}
return ApiFutures.transform(
operationFuture,
GapicSpannerRpc::toOperationSnapshot,
MoreExecutors.directExecutor());
}
};

UnaryCallable<UpdateDatabaseDdlRequest, OperationSnapshot> tracedInitialCallable =
new TracedOperationInitialCallable<>(
initialSnapshotCallable, clientContext.getTracerFactory(), apiTracerContext);

LongRunningClient longRunningClient = createLongRunningClient(operationsStub);
OperationCallable<UpdateDatabaseDdlRequest, Empty, UpdateDatabaseDdlMetadata>
operationCallable =
Callables.longRunningOperation(
tracedInitialCallable, callSettings, clientContext, longRunningClient);

return new TracedOperationCallable<>(
operationCallable, clientContext.getTracerFactory(), operationSpanName)
.withDefaultCallContext(clientContext.getDefaultCallContext());
}

private static LongRunningClient createLongRunningClient(OperationsStub operationsStub) {
return new LongRunningClient() {
@Override
public UnaryCallable<String, OperationSnapshot> getOperationCallable() {
return TranslatingUnaryCallable.create(
operationsStub.getOperationCallable(),
operationName -> GetOperationRequest.newBuilder().setName(operationName).build(),
GapicSpannerRpc::toOperationSnapshot);
}

@Override
public UnaryCallable<String, Void> cancelOperationCallable() {
return TranslatingUnaryCallable.create(
operationsStub.cancelOperationCallable(),
operationName -> CancelOperationRequest.newBuilder().setName(operationName).build(),
ignored -> null);
}

@Override
public UnaryCallable<String, Void> deleteOperationCallable() {
return TranslatingUnaryCallable.create(
operationsStub.deleteOperationCallable(),
operationName -> DeleteOperationRequest.newBuilder().setName(operationName).build(),
ignored -> null);
}
};
}

private static OperationSnapshot toOperationSnapshot(final Operation operation) {
return new OperationSnapshot() {
@Override
public String getName() {
return operation.getName();
}

@Override
public Object getMetadata() {
return operation.getMetadata();
}

@Override
public boolean isDone() {
return operation.getDone();
}

@Override
public Object getResponse() {
return operation.getResponse();
}

@Override
public StatusCode getErrorCode() {
return GrpcStatusCode.of(Status.fromCodeValue(operation.getError().getCode()).getCode());
}

@Override
public String getErrorMessage() {
return operation.getError().getMessage();
}

@Override
public ErrorDetails getErrorDetails() {
return ErrorDetails.builder()
.setRawErrorMessages(operation.getError().getDetailsList())
.build();
}
Comment on lines +2942 to +2957

@gemini-code-assist gemini-code-assist Bot Nov 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The current implementation of toOperationSnapshot always attempts to retrieve the error code, message, and details from the operation object without checking if an error actually occurred. If the operation succeeded, operation.getError() returns a default Status instance with code 0 (OK). This causes getErrorCode() to return GrpcStatusCode.of(Status.Code.OK) instead of null.

In GAX, returning a non-null StatusCode from getErrorCode() is interpreted as an operation failure, which will cause successful operations to be incorrectly treated as failed. We should check operation.hasError() and return null if no error is present, matching the standard behavior of GrpcOperationSnapshot in GAX. Additionally, to minimize null pointer risks, we should prefer returning an empty ErrorDetails object instead of null for getErrorDetails() when no error is present.

      @Override
      public StatusCode getErrorCode() {
        if (operation.hasError()) {
          return GrpcStatusCode.of(
              Status.fromCodeValue(operation.getError().getCode()).getCode());
        }
        return null;
      }

      @Override
      public String getErrorMessage() {
        if (operation.hasError()) {
          return operation.getError().getMessage();
        }
        return null;
      }

      @Override
      public ErrorDetails getErrorDetails() {
        if (operation.hasError()) {
          return ErrorDetails.builder()
              .setRawErrorMessages(operation.getError().getDetailsList())
              .build();
        }
        return ErrorDetails.builder().build();
      }
References
  1. Prefer returning empty objects (such as an empty ErrorDetails or empty collections) instead of null to minimize null pointer risks, and document this behavior in Javadocs.

Comment on lines +2942 to +2957

@gemini-code-assist gemini-code-assist Bot Nov 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

According to the OperationSnapshot contract, getErrorCode() and getErrorMessage() must return null if the operation succeeded without an error. However, to minimize null pointer risks in accordance with our repository guidelines, we should prefer returning an empty ErrorDetails object instead of null for getErrorDetails(), and document this behavior in Javadocs.

Currently, these methods will return non-null values (e.g., GrpcStatusCode.of(Status.Code.OK) and empty strings/builders) even when the operation is successful or still in progress. This can cause GAX's long-running operation machinery to incorrectly treat successful operations as failures.

We should check operation.hasError() and return null for code/message, and an empty ErrorDetails for details if no error is present.

      @Override
      public StatusCode getErrorCode() {
        if (!operation.hasError()) {
          return null;
        }
        return GrpcStatusCode.of(
            Status.fromCodeValue(operation.getError().getCode()).getCode());
      }

      @Override
      public String getErrorMessage() {
        if (!operation.hasError()) {
          return null;
        }
        return operation.getError().getMessage();
      }

      @Override
      public ErrorDetails getErrorDetails() {
        if (!operation.hasError()) {
          return ErrorDetails.builder().build();
        }
        return ErrorDetails.builder()
            .setRawErrorMessages(operation.getError().getDetailsList())
            .build();
      }
References
  1. Prefer returning empty objects (such as an empty ErrorDetails or empty collections) instead of null to minimize null pointer risks, and document this behavior in Javadocs (e.g., how to pair it with state checks like isDone()).

};
}

@Override
public InstanceAdminStubSettings getInstanceAdminStubSettings() {
return instanceAdminStubSettings;
Expand Down
Loading
Loading