Migrate SDK comms to gRPC #8838
Conversation
Instead, move dataplatform-related stuff into `redap` and feature gate that behind a separate feature flag. Message proxy client is now always available.
|
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
|
As a treat... @rerun-bot full-check |
|
Started a full build: https://github.com/rerun-io/rerun/actions/runs/13034883233 |
|
if possible we need some better error messages / warnings when spawning old viewers. This usually works fine but we're doing a hard break here that we should intercept and warn about. Right now it does this: |
|
|
|
We no longer pick up running viewers correctly it seems: Open one command line with -> the snippet will try to spawn a new viewer which in my case brings up a 0.20 viewer since this is the last one I system-installed. It then fails as before |
This is macOS thing (rust-windowing/winit#3931) 😔 |
|
Latest documentation preview deployed successfully.
Note: This comment is updated whenever you push a commit. |
- `.rrd` suffix now required for rrd-over-http - any other `http` url is considered a connection to message proxy
Also bring back `default_server_addr` and `default_flush_timeout`
| } | ||
|
|
||
| assert!(!failed, "one or more test cases failed"); |
There was a problem hiding this comment.
Nit: What's the benefit of just having a single assert here at the end? Won't that make it harder to debug potential problems?
| Ok(Self(url.to_owned())) | ||
| } | ||
| // TODO(#8761): URL prefix | ||
| else if let Some(url) = url.strip_prefix("temp") { |
There was a problem hiding this comment.
Nit: Maybe put "temp" in a const TEMP_URL_PREFIX to make refactoring later easier.
| macro_rules! test_parse_url { | ||
| ($name:ident, $url:literal, error) => { | ||
| #[test] | ||
| fn $name() { | ||
| assert!(MessageProxyUrl::parse($url).is_err()); | ||
| } | ||
| }; | ||
|
|
||
| ($name:ident, $url:literal, expected: $expected_http:literal) => { | ||
| #[test] | ||
| fn $name() { | ||
| assert_eq!( | ||
| MessageProxyUrl::parse($url).map(|v| v.to_http()), | ||
| Ok($expected_http.to_owned()) | ||
| ); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Nit: I think having explicit test cases might make it easier to see what's going on a at a glance.
Related
What
The goal of this PR is to completely delete
re_sdk_commsandre_ws_comms. The prerequisite to this is to first migrate all of their usages over to our temporary gRPC server ("message proxy").Step-by-step:
gRPCvariants ofspawn/connectfrom all SDKsre_ws_commsserver and connect to it by defaultre_sdk_commsandre_ws_commsre_sdk_commsandre_ws_commsI don't really know how to split this up, to be honest! There are tons of subtle but very strong dependencies between different parts of our SDKs, the Viewer, and the "server". Breaking it apart seems too difficult, and it'd also temporarily make some parts of rerun unusable, and thus untestable. So instead I'll be trying to keep individual commits somewhat sensible and reviewable.
Testing checklist
spawnservererun(default args) + SDKconnectrerun --web-viewer+ SDKconnectMigration guide
TODO
Unresolved questions
http://ortemp://prefix be necessary?