-
Notifications
You must be signed in to change notification settings - Fork 133
Add support for gRPC binary metadata values #1070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7509e71
14539b3
21a453e
5001e92
7872b1e
b9977a6
47661cf
edb2fad
0a9916b
fa9b1b2
54487f5
a905e76
8b1dd3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
+214 −20 | client/src/lib.rs | |
+1 −1 | core-c-bridge/src/client.rs | |
+1 −1 | core-c-bridge/src/worker.rs | |
+159 −0 | core/src/worker/tuner.rs | |
+1 −1 | tests/integ_tests/client_tests.rs |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,7 +8,9 @@ use temporal_client::{ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ConfiguredClient, HealthService, HttpConnectProxyOptions, RetryClient, RetryConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TemporalServiceClientWithMetrics, TestService, TlsConfig, WorkflowService, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use tonic::metadata::MetadataKey; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use tonic::metadata::{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AsciiMetadataKey, AsciiMetadataValue, BinaryMetadataKey, BinaryMetadataValue, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use url::Url; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use crate::runtime; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -28,7 +30,7 @@ pub struct ClientConfig { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
target_url: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
client_name: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
client_version: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
metadata: HashMap<String, String>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
metadata: HashMap<String, RpcMetadataValue>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
api_key: Option<String>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
identity: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
tls_config: Option<ClientTlsConfig>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -72,10 +74,18 @@ struct RpcCall { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rpc: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req: Vec<u8>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
retry: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
metadata: HashMap<String, String>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
metadata: HashMap<String, RpcMetadataValue>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
timeout_millis: Option<u64>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#[derive(FromPyObject)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
enum RpcMetadataValue { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#[pyo3(transparent, annotation = "str")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Str(String), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#[pyo3(transparent, annotation = "bytes")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bytes(Vec<u8>), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+81
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the tests in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 And not too worried about error quality for this rare situation |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub fn connect_client<'a>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
py: Python<'a>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
runtime_ref: &runtime::RuntimeRef, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -116,8 +126,19 @@ macro_rules! rpc_call_on_trait { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#[pymethods] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
impl ClientRef { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fn update_metadata(&self, headers: HashMap<String, String>) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.retry_client.get_client().set_headers(headers); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fn update_metadata(&self, headers: HashMap<String, RpcMetadataValue>) -> PyResult<()> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let (ascii_headers, binary_headers) = partition_headers(headers); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.retry_client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.get_client() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.set_headers(ascii_headers) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|err| PyValueError::new_err(err.to_string()))?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.retry_client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.get_client() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.set_binary_headers(binary_headers) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|err| PyValueError::new_err(err.to_string()))?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+132
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are both fallible and non-atomic 😕. Right now there's a bit of a risk that the client is left in an inconsistent state (in a way that other code could observe) if one of them fails. I suspect you'll run into this in the other SDKs, too. I made a test to reproduce/document the issue: sdk-python/tests/api/test_grpc_stub.py Lines 235 to 265 in 54487f5
Let me know if we should fix this. I think another PR to SDK core that either:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not that concerned, though arguably we could have changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
FWIW the reason I didn't do something like that originally is that it would have broken semver on the Core SDK client crate (although maybe I could have done some
Ack, I can make that change.
Heh, fair. Yeah I think if the ASCII is invalid (and setting it fails), this same issue doesn't exist. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the docs in 8b1dd3e: ![]() |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fn update_api_key(&self, api_key: Option<String>) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -536,12 +557,32 @@ fn rpc_req<P: prost::Message + Default>(call: RpcCall) -> PyResult<tonic::Reques | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|err| PyValueError::new_err(format!("Invalid proto: {err}")))?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut req = tonic::Request::new(proto); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for (k, v) in call.metadata { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req.metadata_mut().insert( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
MetadataKey::from_str(k.as_str()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|err| PyValueError::new_err(format!("Invalid metadata key: {err}")))?, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
v.parse() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|err| PyValueError::new_err(format!("Invalid metadata value: {err}")))?, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if let Ok(binary_key) = BinaryMetadataKey::from_str(&k) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let RpcMetadataValue::Bytes(bytes) = v else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Err(PyValueError::new_err(format!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"Invalid metadata value for binary key {k}: expected bytes" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req.metadata_mut() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.insert_bin(binary_key, BinaryMetadataValue::from_bytes(&bytes)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let ascii_key = AsciiMetadataKey::from_str(&k) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|err| PyValueError::new_err(format!("Invalid metadata key: {err}")))?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let RpcMetadataValue::Str(string) = v else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Err(PyValueError::new_err(format!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"Invalid metadata value for ASCII key {k}: expected str" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req.metadata_mut().insert( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ascii_key, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AsciiMetadataValue::from_str(&string).map_err(|err| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
PyValueError::new_err(format!("Invalid metadata value: {err}")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
})?, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if let Some(timeout_millis) = call.timeout_millis { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req.set_timeout(Duration::from_millis(timeout_millis)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -568,11 +609,41 @@ where | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fn partition_headers( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
headers: HashMap<String, RpcMetadataValue>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) -> (HashMap<String, String>, HashMap<String, Vec<u8>>) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let (ascii_enum_headers, binary_enum_headers): (HashMap<_, _>, HashMap<_, _>) = headers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.into_iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.partition(|(_, v)| matches!(v, RpcMetadataValue::Str(_))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let ascii_headers = ascii_enum_headers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.into_iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map(|(k, v)| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let RpcMetadataValue::Str(s) = v else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
unreachable!(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
(k, s) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let binary_headers = binary_enum_headers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.into_iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map(|(k, v)| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let RpcMetadataValue::Bytes(b) = v else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
unreachable!(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
(k, b) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
(ascii_headers, binary_headers) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
impl TryFrom<ClientConfig> for ClientOptions { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type Error = PyErr; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fn try_from(opts: ClientConfig) -> PyResult<Self> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut gateway_opts = ClientOptionsBuilder::default(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let (ascii_headers, binary_headers) = partition_headers(opts.metadata); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
gateway_opts | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.target_url( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Url::parse(&opts.target_url) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -587,7 +658,8 @@ impl TryFrom<ClientConfig> for ClientOptions { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.keep_alive(opts.keep_alive_config.map(Into::into)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.http_connect_proxy(opts.http_connect_proxy_config.map(Into::into)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.headers(Some(opts.metadata)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.headers(Some(ascii_headers)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.binary_headers(Some(binary_headers)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.api_key(opts.api_key); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Builder does not allow us to set option here, so we have to make | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// a conditional to even call it | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to use
str | bytes
luckily ran into a CI issue about Python 3.9 incompatibility (which I hadn't realized). These all useUnion[str, bytes]
as a result.