Repository navigation
accept references to payloads in put requests #1218
Description
Activity
Thanks for the issue.
I don't think builders are a very idiomatic way of doing a lot of this in the rust ecosystem, although it seems to be the AWS way.
I'd argue that builders are fairly common in Rust but that's neither here nor there. The use of builders in the Rust SDK stems in part from dealing with model evolutions and being able to provide strong backwards compatibility guarantees.
I don't think there is much we can do here but I would ask why you are implementing retries on top of the SDK which already retries requests?
Unfortunately, the
PutRecordsbatch call cannot be retried automatically, because it supports partial success... so the retry you're @aajtodd talking about probably only works up until a partial success. I would imagine all endpoints with partial success are therefore susceptible to this same problem.In this particular case, I think what is happening is that within the batch, the puts to shards that are being rescaled are failing, but the puts to shards that are fine are succeeding. A more efficient retry user-level logic would retry only the puts that failed the first time, but it's sufficient to retry everything. Regardless, to be able to avoid data loss, one must copy all the data and throw it away in most cases, because the builder consumes the input. The total payload can be up to 5MB (see PutRecords), broken into as many as 500 individual blobs, so it's not an insignificant cost for a high throughput application. (And remember this is all happening in parallel, consider how much data that is for a stream with 1,000 shards, albeit across many producers...)
There probably is something that can be done here, though... adding the request payloads to the failed responses would for sure solve this particular issue. That way there's no need to clone every 5MB upload before sending it, we know we will get back the requests that failed. That would be a
PutRecordsResultEntryand adding an optional fieldfailed_request: Option<PutRecordsRequestEntry>would be great, only populated on failure.One option to consider is registering a custom retry classifier. This would have the advantage of retrying the already serialized request without the need to clone it yourself (or implement a custom retry loop).
It would look something like this:
This example takes advantage of per operation config and assumes the retry classifier is only used for the operation in question (otherwise you'll need to modify the downcast appropriately).
use aws_config::BehaviorVersion; use aws_sdk_kinesis::config::Intercept; use aws_sdk_kinesis::config::retry::{ClassifyRetry, RetryAction}; #[tokio::main] async fn main() { tracing_subscriber::fmt::init(); let sdk_config = aws_config::defaults(BehaviorVersion::latest()) .load() .await; let kinesis = aws_sdk_kinesis::Client::new(&sdk_config); let custom_retry_config = kinesis.config().to_builder().retry_classifier(RetryPartialSuccessClassifier); kinesis.put_records() // ... normal operation inputs // add our custom config override with our retry classifier .customize() .config_override(custom_retry_config) .send() .await; println!("resp: {:?}", resp); } #[derive(Debug)] struct RetryPartialSuccessClassifier; impl ClassifyRetry for RetryPartialSuccessClassifier { fn name(&self) -> &'static str { "RetryPartialSuccessClassifier" } fn classify_retry(&self, ctx: &aws_sdk_kinesis::config::interceptors::InterceptorContext) -> aws_sdk_kinesis::config::retry::RetryAction { let output = match ctx.output_or_error() { Some(Ok(erased)) => erased.downcast_ref::<aws_sdk_kinesis::operation::put_records::PutRecordsOutput>().expect("correct type"), _ => return RetryAction::NoActionIndicated, }; let failed = output.failed_record_count().unwrap_or(0); if failed > 0 { tracing::debug!("retrying partial success failed: {}", failed); RetryAction::transient_error() }else { RetryAction::NoActionIndicated } } }
Reacted by Sam Hallidayexcellent, thanks @aajtodd I think that should workaround the issue so I'm happy to close it for now... although I haven't written any code at this point to check it. Assuming that works, it should unblock the use of Rust as a viable alternative to JVM based solutions for high throughput writing to kinesis.
Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.Sure thing, let us know if it doesn't work or you have questions. Also wanted to point out this strategy will retry the whole batch as it's retrying the whole HTTP request after serialization but it sounded like that was an ok tradeoff. Just wanted to make sure that was clear though.
Describe the feature
many builders require their input to be owned, which means that in order to handle a possible retry under failure, the caller must make a copy that is most likely going to be thrown away. This is very wasteful, especially in high throughput applications and when writing large payloads.
Use Case
here's an example of writing records to kinesis. The
batchhere has to be fully owned, containing the payloads. Which means we need tocloneit just incase we need to retry it....
Proposed Solution
I don't think builders are a very idiomatic way of doing a lot of this in the rust ecosystem, although it seems to be the AWS way. If there were no builders, or struct objects, we would be able to say that the methods could simply take referencse rather than owned objects. However, I doubt we'll see builders done away with... so it would be good enough if select responses returned the input parameter instead of completely consuming. That behaves effectively the same as taking a reference.
Or take
Rcwrappers of everything in the builders... or push that to the smithy layer and useRcwrapped bytes in theBlob.Other Information
No response
Acknowledgements
A note for the community
Community Note