Skip to content

*: enable drpc using rpc context config#161728

Merged
craig[bot] merged 7 commits intocockroachdb:masterfrom
cthumuluru-crdb:drpc-remove-cluster-setting
Jan 29, 2026
Merged

*: enable drpc using rpc context config#161728
craig[bot] merged 7 commits intocockroachdb:masterfrom
cthumuluru-crdb:drpc-remove-cluster-setting

Conversation

@cthumuluru-crdb
Copy link
Contributor

@cthumuluru-crdb cthumuluru-crdb commented Jan 24, 2026

Previously, drpc usage for inter-node communication was controlled by the rpc.experimental_drpc.enabled cluster setting. However, this setting is not available on all code paths.

With the introduction of the --use-new-rpc CLI flag, drpc usage can be determined using rpc context config, making the cluster setting redundant.

This commit updates all code paths to use rpc context config rather than cluster setting when deciding whether to use drpc for inter-node communication and removes the cluster setting.

Informs: #155600
Epic: CRDB-55200
Release note: none

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cthumuluru-crdb cthumuluru-crdb force-pushed the drpc-remove-cluster-setting branch 7 times, most recently from 652dc87 to 7ab3024 Compare January 27, 2026 12:00
@cthumuluru-crdb cthumuluru-crdb marked this pull request as ready for review January 27, 2026 12:00
@cthumuluru-crdb cthumuluru-crdb requested review from a team as code owners January 27, 2026 12:00
@cthumuluru-crdb cthumuluru-crdb requested review from dhartunian, herkolategan, michae2 and williamchoe3 and removed request for a team January 27, 2026 12:00
@cthumuluru-crdb cthumuluru-crdb changed the title modify nodedialer behavior to use rpcCtx over cluster settings to dec… *: enable drpc using rpc context config Jan 27, 2026
Copy link
Contributor

@shubhamdhama shubhamdhama left a comment

Choose a reason for hiding this comment

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

LGTM! I have left couple of minor comments.

Copy link
Contributor

@shubhamdhama shubhamdhama left a comment

Choose a reason for hiding this comment

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

I think this PR is safe to merge as tests can ensure the correctness. Can you also randomly hand picked 2-3 tests to ensure they are indeed using DRPC (not just the test log statement, but in the cockroach logs).

@cthumuluru-crdb cthumuluru-crdb added the do-not-merge bors won't merge a PR with this label. label Jan 29, 2026
Copy link
Contributor

@Nukitt Nukitt left a comment

Choose a reason for hiding this comment

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

:lgtm:

@Nukitt reviewed 13 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @herkolategan, @michae2, @shubhamdhama, @suj-krishnan, and @williamchoe3).

@cthumuluru-crdb
Copy link
Contributor Author

I think this PR is safe to merge as tests can ensure the correctness. Can you also randomly hand picked 2-3 tests to ensure they are indeed using DRPC (not just the test log statement, but in the cockroach logs).

Yes, I verified this with a couple of test runs.

./dev test //pkg/kv/kvserver/loqrecovery:loqrecovery_test --filter=TestStreamRestart --ignore-cache --stream-output --show-logs -- --test_env=COCKROACH_TEST_DRPC=true | tee test.log

--- logs ----
W260129 06:57:34.764032 10869 rpc/drpc.go:348  [-] 474  drpc server error manager closed: closed: read tcp 127.0.0.1:52471 -> 127.0.0.1:52569: read: connection reset by peer
W260129 06:57:34.764053 10647 rpc/drpc.go:348  [-] 475  drpc server error manager closed: closed: read tcp 127.0.0.1:52471 -> 127.0.0.1:52568: read: connection reset by peer

Previously, drpc usage for inter-node communication was controlled by the
`rpc.experimental_drpc.enabled` cluster setting. However, this setting is not
available on all code paths.

With the introduction of the `--use-new-rpc` CLI flag, drpc usage can be
determined using rpc context config, making the cluster setting redundant.

This commit updates the code paths in rpc and sql packages to use rpc context
config rather than cluster setting when deciding whether to use drpc
for inter-node communication.

Informs: cockroachdb#155600
Epic: CRDB-55200
Release note: none
Previously, drpc usage for inter-node communication was controlled by the
`rpc.experimental_drpc.enabled` cluster setting. However, this setting is not
available on all code paths.

With the introduction of the `--use-new-rpc` CLI flag, drpc usage can be
determined using rpc context config, making the cluster setting redundant.

This commit updates the code paths in rpc and server packages to use rpc context
config rather than cluster setting when deciding whether to use drpc
for inter-node communication.

Informs: cockroachdb#155600
Epic: CRDB-55200
Release note: none
Previously, drpc usage for inter-node communication was controlled by the
`rpc.experimental_drpc.enabled` cluster setting. However, this setting is not
available on all code paths.

With the introduction of the `--use-new-rpc` CLI flag, drpc usage can be
determined using rpc context config, making the cluster setting redundant.

This commit updates the code paths in server package to use rpc context
config rather than cluster setting when deciding whether to use drpc
for inter-node communication.

Informs: cockroachdb#155600
Epic: CRDB-55200
Release note: none
Previously, drpc usage for inter-node communication was controlled by the
`rpc.experimental_drpc.enabled` cluster setting. However, this setting is not
available on all code paths.

With the introduction of the `--use-new-rpc` CLI flag, drpc usage can be
determined using rpc context config, making the cluster setting redundant.

This commit updates the code paths in server and upgrade packages to use rpc context
config rather than cluster setting when deciding whether to use drpc
for inter-node communication.

Informs: cockroachdb#155600
Epic: CRDB-55200
Release note: none
@cthumuluru-crdb cthumuluru-crdb force-pushed the drpc-remove-cluster-setting branch from f904b54 to f770ee3 Compare January 29, 2026 08:25
This commit refactors the closed timestamp sender to accept a
`NewSideTransportClient` function instead, which abstracts away the client
creation logic. This allows the sender to remain agnostic to whether drpc or
gRPC is used.

Informs: cockroachdb#155600
Epic: CRDB-55200
Release note: none
Remove the `rpc.experimental_drpc.enabled` cluster setting helper function now
that all call sites have been updated to use the rpc context config to enable
drpc.

Informs: cockroachdb#155600
Epic: CRDB-55200
Release note: none
Previously, drpc usage for inter-node communication was controlled by the
`rpc.experimental_drpc.enabled` cluster setting. However, this setting is not
available on all code paths.

With the introduction of the `--use-new-rpc` CLI flag, roachtest has been
updated to use CLI flag to enable drpc.

Informs: cockroachdb#155600
Epic: CRDB-55200
Release note: none
@cthumuluru-crdb cthumuluru-crdb force-pushed the drpc-remove-cluster-setting branch from f770ee3 to f655191 Compare January 29, 2026 08:26
@cthumuluru-crdb cthumuluru-crdb removed the do-not-merge bors won't merge a PR with this label. label Jan 29, 2026
@cthumuluru-crdb
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 29, 2026

@craig craig bot merged commit 83741a9 into cockroachdb:master Jan 29, 2026
26 of 27 checks passed
@cthumuluru-crdb cthumuluru-crdb deleted the drpc-remove-cluster-setting branch January 29, 2026 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants