Skip to content

chore(spanner): introduce commitRpcTimeout to connection properties#13801

Draft
sagnghos wants to merge 1 commit into
googleapis:mainfrom
sagnghos:sagnghos/ycsb
Draft

chore(spanner): introduce commitRpcTimeout to connection properties#13801
sagnghos wants to merge 1 commit into
googleapis:mainfrom
sagnghos:sagnghos/ycsb

Conversation

@sagnghos

Copy link
Copy Markdown
Contributor

This explicitly sets an operation timeout bound for Spanner transactions so that severe network blockholes do not lead to an infinite connection timeout at the YCSB or application layer when proxy mechanisms (like PGAdapter) are routing via EKS Load Balancers.

This explicitly sets an operation timeout bound for Spanner transactions
so that severe network blockholes do not lead to an infinite connection timeout at the YCSB or application layer when proxy mechanisms (like PGAdapter) are routing via EKS Load Balancers.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces a new commitRpcTimeout configuration option to customize the RPC timeout specifically for commit operations in Spanner. The feedback highlights a few areas for improvement: correcting misplaced Javadoc comments in ConnectionOptions.java, ensuring consistent private final visibility and this reference usage for the new field in SpannerPoolKey, and adding a precondition check in SpannerOptions.Builder to prevent negative timeout values.

Comment on lines 1098 to 1106
/** The gRPC keepalive time for this connection. */
/** The commit RPC timeout for this connection. */
public Duration getCommitRpcTimeout() {
return getInitialConnectionPropertyValue(COMMIT_RPC_TIMEOUT);
}

public Duration getGrpcKeepAliveTime() {
return getInitialConnectionPropertyValue(GRPC_KEEPALIVE_TIME);
}

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.

medium

The Javadoc comment for getGrpcKeepAliveTime() was accidentally moved above getCommitRpcTimeout(), resulting in duplicated Javadoc blocks for getCommitRpcTimeout() and leaving getGrpcKeepAliveTime() without any documentation. Please correct the placement of these Javadoc comments.

Suggested change
/** The gRPC keepalive time for this connection. */
/** The commit RPC timeout for this connection. */
public Duration getCommitRpcTimeout() {
return getInitialConnectionPropertyValue(COMMIT_RPC_TIMEOUT);
}
public Duration getGrpcKeepAliveTime() {
return getInitialConnectionPropertyValue(GRPC_KEEPALIVE_TIME);
}
/** The commit RPC timeout for this connection. */
public Duration getCommitRpcTimeout() {
return getInitialConnectionPropertyValue(COMMIT_RPC_TIMEOUT);
}
/** The gRPC keepalive time for this connection. */
public Duration getGrpcKeepAliveTime() {
return getInitialConnectionPropertyValue(GRPC_KEEPALIVE_TIME);
}

private final String host;
private final String projectId;
private final Duration grpcKeepAliveTime;
final Duration commitRpcTimeout;

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.

medium

The field commitRpcTimeout in SpannerPoolKey is declared with package-private visibility, whereas all other fields in this class are declared as private final. For consistency and proper encapsulation, please make it private final.

Suggested change
final Duration commitRpcTimeout;
private final Duration commitRpcTimeout;

this.universeDomain,
this.grpcInterceptorProvider,
this.grpcKeepAliveTime,
commitRpcTimeout,

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.

medium

For consistency with the other fields referenced in this method, please use this.commitRpcTimeout instead of commitRpcTimeout.

Suggested change
commitRpcTimeout,
this.commitRpcTimeout,

Comment on lines +1720 to +1723
public Builder setCommitRpcTimeout(Duration commitRpcTimeout) {
this.commitRpcTimeout = commitRpcTimeout;
return this;
}

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.

medium

To prevent invalid configurations early, consider adding a check to ensure that commitRpcTimeout is not negative when it is non-null.

    public Builder setCommitRpcTimeout(Duration commitRpcTimeout) {
      if (commitRpcTimeout != null) {
        Preconditions.checkArgument(
            !commitRpcTimeout.isNegative(), "commitRpcTimeout cannot be negative");
      }
      this.commitRpcTimeout = commitRpcTimeout;
      return this;
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant