[sakthivelmanii]

Problem

• DatabaseAdminClient.updateDatabaseDdlAsync sends an UpdateDatabaseDdlRequest with an empty operation_id.
• GAX retries UNAVAILABLE or DEADLINE_EXCEEDED requests without deduplication, leading to a second execution that fails with Duplicate column name Venues.Revenue.

Solution

• Use ApiFutures.catchingAsync on the initial UnaryCallable to catch AlreadyExistsException and resume via GetOperation without blocking the calling thread.
• Preserve explicit operation_id semantics in GapicSpannerRpc and SpannerImpl.
• Add UpdateDatabaseDdlRetryTest to verify behavior.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements client-side idempotent retries for UpdateDatabaseDdl operations when the operation_id is omitted, generating a unique ID and resuming the operation via GetOperation if an ALREADY_EXISTS error occurs. It also adds comprehensive unit tests for this retry logic. Feedback on the changes identifies a critical bug in toOperationSnapshot where successful operations are incorrectly treated as failures because getErrorCode() returns a non-null status code even when no error is present; checking operation.hasError() is recommended to fix this.

[gemini-code-assist]

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.

[gemini-code-assist]

Code Review

This pull request refactors the database admin client creation in SpannerImpl and introduces client-side idempotency for UpdateDatabaseDdl operations in GapicSpannerRpc when the caller omits an operation_id. It generates a unique client-side operation_id per logical call and catches AlreadyExistsException to resume the operation via GetOperation if a retry fails. Unit tests are added in UpdateDatabaseDdlRetryTest to verify this behavior. A critical review comment points out a compilation error in GapicSpannerRpc.java where SpanName.of is incorrectly called with an ApiTracerContext argument.

[gemini-code-assist]

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");

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces client-side idempotency for UpdateDatabaseDdl operations when the caller omits an operation_id by generating a client-side ID and catching AlreadyExistsException to resume the operation via GetOperation. The review feedback identifies two critical issues: first, the generated operation_id exceeds the 30-character limit enforced by the Spanner production service, which would cause requests to fail with INVALID_ARGUMENT; second, getErrorCode() and getErrorMessage() in toOperationSnapshot must return null when there is no error to prevent GAX from incorrectly treating successful operations as failures.

[gemini-code-assist]

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);

[gemini-code-assist]

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()).