[Refactoring] Consolidate multiple create Tonic channel functions #2469

pcholakov · 2025-01-06T16:06:27Z

Eliminates some duplication across test and prod code for creating GRPC channels.

pcholakov · 2025-01-06T16:08:33Z

crates/cli-util/src/opts.rs

+    }
+
+    fn keep_alive_interval(&self) -> Duration {
+        Duration::from_secs(60)


Hard-coding this for the CLI should be okay; wanted to keep the trait as-is to avoid changing prod code.

pcholakov · 2025-01-06T16:09:00Z

crates/cli-util/src/opts.rs

@@ -63,7 +67,7 @@ pub(crate) struct ConfirmMode {
 }

 #[derive(Args, Clone, Default)]
-pub(crate) struct NetworkOpts {
+pub struct NetworkOpts {


We now need to export this struct to make this work.

pcholakov · 2025-01-06T16:09:42Z

server/tests/trim_gap_handling.rs

+            connect_timeout: 1000,
+            request_timeout: 5000,


Seemed reasonable for tests.

github-actions · 2025-01-06T16:24:47Z

Test Results

7 files ±0 7 suites ±0 4m 24s ⏱️ +4s
47 tests ±0 46 ✅ ±0 1 💤 ±0 0 ❌ ±0
182 runs ±0 179 ✅ ±0 3 💤 ±0 0 ❌ ±0

Results for commit d661cd7. ± Comparison against base commit 483dcd6.

♻️ This comment has been updated with latest results.

tillrohrmann

Thanks for creating this PR @pcholakov. This is a good clean up! The one question I have is whether we want to enable create_tonic_channel_from_advertised_address to also accept a request timeout so that we don't have to change the semantics of restatectl?

tillrohrmann · 2025-01-07T10:48:35Z

crates/cli-util/src/opts.rs

+    fn keep_alive_timeout(&self) -> Duration {
+        Duration::from_millis(self.request_timeout)
+    }


Isn't this a different meaning than before? Before request_timeout defined a timeout for the overall request.

Thank you for pointing this out, I was very sloppy here! I totally mixed up request timeout and connect timeout. Fixed now by introducing a dedicated optional request_timeout() method on the config trait.

I'm still defaulting keep-alive timeout to the connect_timeout with a very large fixed interval for the CLI. Previously we didn't set it but I think this is benign because:

a keep-alive should be cheap for the peer to process; we should receive a response much faster than a typical request as it requires effectively zero "service time"

CLI connections are in any event unlikely to live longer than the (hard-coded to 60s) keep-alive interval

It feels kludgey but I didn't want to make these optional in the common config trait.

tillrohrmann · 2025-01-07T10:49:24Z

server/tests/trim_gap_handling.rs

+    fn keep_alive_timeout(&self) -> Duration {
+        Duration::from_millis(self.request_timeout)
+    }


Same question here. Before we seem to have set Channel::builder().timeout(request_timeout) which is no longer the case with this change.

Fixed now; I was being blind - I've added a dedicated request_timeout property. I messed up when merging the logic of the cli/test-only and production code endpoint building.

tillrohrmann · 2025-01-07T10:51:21Z

tools/restatectl/src/util.rs

-        AdvertisedAddress::Http(uri) => {
-            Channel::builder(uri)
-                .connect_timeout(ctx.connect_timeout())
-                .timeout(ctx.request_timeout())


How are we setting the overall timeout now? Or is this no longer needed?

Added back! My mistake!

pcholakov · 2025-01-08T08:54:04Z

crates/cli-util/src/opts.rs

+    fn keep_alive_timeout(&self) -> Duration {
+        Duration::from_millis(self.request_timeout)
+    }


Thank you for pointing this out, I was very sloppy here! I totally mixed up request timeout and connect timeout. Fixed now by introducing a dedicated optional request_timeout() method on the config trait.

I'm still defaulting keep-alive timeout to the connect_timeout with a very large fixed interval for the CLI. Previously we didn't set it but I think this is benign because:

a keep-alive should be cheap for the peer to process; we should receive a response much faster than a typical request as it requires effectively zero "service time"

CLI connections are in any event unlikely to live longer than the (hard-coded to 60s) keep-alive interval

It feels kludgey but I didn't want to make these optional in the common config trait.

pcholakov · 2025-01-08T10:30:58Z

server/tests/trim_gap_handling.rs

+    fn keep_alive_timeout(&self) -> Duration {
+        Duration::from_millis(self.request_timeout)
+    }


Fixed now; I was being blind - I've added a dedicated request_timeout property. I messed up when merging the logic of the cli/test-only and production code endpoint building.

pcholakov · 2025-01-08T10:34:02Z

tools/restatectl/src/util.rs

-        AdvertisedAddress::Http(uri) => {
-            Channel::builder(uri)
-                .connect_timeout(ctx.connect_timeout())
-                .timeout(ctx.request_timeout())


Added back! My mistake!

pcholakov · 2025-01-08T10:36:36Z

crates/core/src/network/net_util.rs

@@ -30,36 +30,47 @@ use restate_types::net::{AdvertisedAddress, BindAddress};

 use crate::{cancellation_watcher, ShutdownError, TaskCenter, TaskKind};

-pub fn create_tonic_channel_from_advertised_address<T: CommonClientConnectionOptions>(
+pub fn create_tonic_channel<T: CommonClientConnectionOptions>(


An approach where we accept a closure to customize the endpoint might be a nicer alternative if CommonClientConnectionOptions gets any more complex, but I'm punting on that for now.

tillrohrmann

Nice work @pcholakov. This is a good improvement and clean up :-) LGTM. +1 for merging.

pcholakov changed the base branch from main to feat/trim-gap-e2e-test January 6, 2025 16:07

pcholakov commented Jan 6, 2025

View reviewed changes

pcholakov requested a review from tillrohrmann January 6, 2025 16:09

pcholakov marked this pull request as ready for review January 6, 2025 16:10

pcholakov mentioned this pull request Jan 6, 2025

Add an end-to-end test for trim gap handling using snapshots #2463

Merged

tillrohrmann reviewed Jan 7, 2025

View reviewed changes

pcholakov force-pushed the feat/trim-gap-e2e-test branch 6 times, most recently from 5200c2a to 81d3b2e Compare January 8, 2025 08:42

pcholakov force-pushed the refactor/consolidate-create-tonic-channel-copies branch 2 times, most recently from 3aca5aa to df3d0bd Compare January 8, 2025 10:34

pcholakov commented Jan 8, 2025

View reviewed changes

pcholakov requested a review from tillrohrmann January 8, 2025 10:37

pcholakov force-pushed the feat/trim-gap-e2e-test branch from 81d3b2e to 9592d6a Compare January 8, 2025 13:48

Base automatically changed from feat/trim-gap-e2e-test to main January 8, 2025 16:07

tillrohrmann approved these changes Jan 10, 2025

View reviewed changes

pcholakov added 2 commits January 10, 2025 16:36

Use shared create_tonic_channel_from_advertised_address utility

55c24ef

Properly set [optional] request timeout in the test context

6ef9a2a

pcholakov force-pushed the refactor/consolidate-create-tonic-channel-copies branch from df3d0bd to 6ef9a2a Compare January 10, 2025 14:36

Fixup issue post-rebase, rename grpc_connect to grpc_channel

d661cd7

pcholakov merged commit a5e1503 into main Jan 10, 2025
15 checks passed

pcholakov deleted the refactor/consolidate-create-tonic-channel-copies branch January 10, 2025 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactoring] Consolidate multiple create Tonic channel functions #2469

[Refactoring] Consolidate multiple create Tonic channel functions #2469

pcholakov commented Jan 6, 2025 •

edited

Loading

pcholakov Jan 6, 2025

pcholakov Jan 6, 2025

pcholakov Jan 6, 2025

github-actions bot commented Jan 6, 2025 •

edited

Loading

tillrohrmann left a comment

tillrohrmann Jan 7, 2025

pcholakov Jan 8, 2025

tillrohrmann Jan 7, 2025

pcholakov Jan 8, 2025

tillrohrmann Jan 7, 2025

pcholakov Jan 8, 2025

pcholakov Jan 8, 2025

pcholakov Jan 8, 2025

pcholakov Jan 8, 2025

pcholakov Jan 8, 2025

tillrohrmann left a comment

[Refactoring] Consolidate multiple create Tonic channel functions #2469

[Refactoring] Consolidate multiple create Tonic channel functions #2469

Conversation

pcholakov commented Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 6, 2025 • edited Loading

Test Results

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann left a comment

Choose a reason for hiding this comment

pcholakov commented Jan 6, 2025 •

edited

Loading

github-actions bot commented Jan 6, 2025 •

edited

Loading