feat: add codegen wrappers for retries #490

ianbotsf · 2021-10-05T21:06:08Z

Issue #

Closes #224

Description of changes

Implements the codegen support and wrappers for retrying all operations. Runtime components were implemented in #487.

This PR includes the first example of replaceable integrations which may be controversial. I chose that to allow a default strategy/policy in smithy-kotlin but allow overriding for specific codegen use cases (like aws-sdk-kotlin). Comments on this approach are welcome.

Scope

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

kggilmer

Yes regarding the introduction of this new extension capability of replacing integrations..it's not clear to me from these PRs why defining/overriding a section would not suffice. Can you briefly outline why you choose this approach over sections?

kggilmer · 2021-10-05T21:28:02Z

smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/CodegenVisitor.kt

+
+        val replaceIntegrations = integrationsBeforeReplacement
+            .flatMap { it.replacesIntegrations }
+            .onEach { integration -> LOGGER.info("Replacing KotlinIntegration: ${integration.javaClass.name}") }


suggestion

for future debugging i think it would be helpful to print both the source and target here.

kggilmer · 2021-10-05T22:09:20Z

smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/CodegenVisitor.kt

            .filter { integration -> integration.enabledForService(context.model, settings) }
-            .also { integration -> LOGGER.info("Enabled KotlinIntegration: ${integration.javaClass.name}") }
+            .onEach { integration -> LOGGER.info("Enabled KotlinIntegration: ${integration.javaClass.name}") }


correctness

I think this log message is misleading as any particular integration may be removed in the subsequent code. Maybe "loaded" and then "enabled" in the final stanza? Generally I think it's noisy if we are saying anything about an unused integration except that it's unused in logs.

aajtodd

Overall it looks good. I too am not quite convinced that we need the new "replace" capability though.

In fact we already have a section in the ServiceGenerator to override the config properties if needed. We have precedence for default config properties. Both the idempotency token provider and http client engine are registered as default properties.

We also have a way of specifying default middleware. (Alternatively you could insert it into getHttpMiddleware() such that it is always present and a customization would have to remove it if they really wanted it gone).

I almost want to just say let's always render a retry strategy and a retry middleware and if you want to not have retries then pass in a "no retry" strategy.

aajtodd · 2021-10-06T13:07:56Z

...hy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/RuntimeTypes.kt

@@ -48,6 +48,10 @@ object RuntimeTypes {
            val HttpResponse = runtimeSymbol("HttpResponse", KotlinDependency.HTTP, "response")
        }

+        object Retries {
+            val StandardRetryFeature = runtimeSymbol("StandardRetryFeature", KotlinDependency.HTTP, "retries")


nit

I should have caught this before but this probably should have lived in aws.smithy.kotlin.runtime.http.middleware which is an existing package. Also since it takes a strategy and a policy nothing makes it "Standard". It could probably just be RetryMiddleware

Well you couldn't have caught this before since it's new in this PR. 😃 I can relocate it.

aajtodd · 2021-10-06T13:18:39Z

...en/src/main/kotlin/software/amazon/smithy/kotlin/codegen/retries/StandardRetryIntegration.kt

+                symbol = buildSymbol {
+                    name = "RetryStrategy"
+                    namespace(KotlinDependency.CORE, "retries")
+                    nullable = false


question

This implies we wouldn't be able to set the strategy to null someday correct? To get "no retry" behavior someone would have to explicitly pass retryStrategy = RetryStrategy.None or something. I think that's probably ok (and more clear) but just clarifying our stance.

Yes, exactly. No nulls, always some RetryStrategy even if just a trivial None implementation.

ianbotsf · 2021-10-06T22:05:00Z

I almost want to just say let's always render a retry strategy and a retry middleware and if you want to not have retries then pass in a "no retry" strategy.

OK, let me see what I can work up.

aajtodd

I think I may have jumped the gun, will wait for the rest of the changes before continuing to review. Let me know when it's ready

aajtodd · 2021-10-11T13:51:28Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/request/HttpRequestBuilder.kt

     */
-    var method: HttpMethod = HttpMethod.GET
+    val isRetryable: Boolean


nit

seems like this can be localized to the retry middleware as an extension

aajtodd · 2021-10-11T13:53:47Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/util/StringValuesMap.kt

-        if (caseInsensitiveName) CaseInsensitiveMap<List<String>>().apply { putAll(initialValues) } else values.toMap()
+    protected val values: Map<String, List<String>> = run {
+        // Make a defensive copy so modifications to the initialValues don't mutate our internal copy
+        val copiedValues = initialValues.deepCopy()


question

is this really necessary? The input initialValues is an immutable map with an immutable key and value.

I'm afraid it is necessary. The type of the parameter may be the immutable interface type Map but the actual argument supplied is a mutable implementation. Modifications to that map in the builder will cause already-built instances to mutate unless copied.

Moreover, the map values passed in are mutable lists. They too need to be defensively copied to prevent post hoc mutation.

Which argument where?

The argument supplied from the builder is a mutable map.

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/middleware/RetryFeature.kt

aajtodd · 2021-10-11T13:59:37Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/operation/OperationRequest.kt

+ * transient and is not part of the copy. The subject itself, however, is deeply copied.
+ * @return A new [OperationRequest] with the same context and a deeply-copied subject.
+ */
+fun <T : CanDeepCopy<T>> OperationRequest<T>.deepCopy(): OperationRequest<T> =


suggestion

Make internal?

aajtodd · 2021-10-11T14:06:09Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/QueryParameters.kt

+    override fun build(): QueryParameters = QueryParametersImpl(values)
+
+    override fun deepCopy(): QueryParametersBuilder =
+        QueryParametersBuilder().apply { values.putAll([email protected]()) }


style

I found this hard to read/follow due to this both referring to the same type QueryParametersBuilder. I guess it's fine as is if it works but it took me staring at this for a second to verify it was correct.

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/middleware/RetryFeature.kt

ianbotsf · 2021-10-11T17:38:27Z

...n-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/WaiterGenerator.kt

+import software.amazon.smithy.waiters.WaitableTrait
+import software.amazon.smithy.waiters.Waiter
+
+class WaiterGenerator(


Comment: This shouldn't be here and I'll pull it out shortly...ignore it for now!

aajtodd

Couple questions on correctness. I'm also not sure about HttpBody changing to support CanDeepCopy. My inclination is to really isolate this as a retry concern and not have it supported in the type hierarchy.

aajtodd · 2021-10-11T18:53:49Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/HttpBody.kt

 import aws.smithy.kotlin.runtime.io.SdkByteReadChannel

 /**
 * HTTP payload to be sent to a peer
 */
-sealed class HttpBody {
+sealed class HttpBody : CanDeepCopy<HttpBody> {


question

Does this really need to support deep copy? Payload bytes aren't modified by signing (and they really shouldn't be modified by any middleware)

aajtodd · 2021-10-11T18:56:11Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/HttpBody.kt

+    /**
+     * Flag indicating the body can be consumed multiple times.
+     */
+    open val isReplayable: Boolean = true


question/style

why move this? The only type we expect to need to "replay" is streaming right?

aajtodd · 2021-10-11T18:58:19Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/Url.kt

@@ -130,6 +131,17 @@ class UrlBuilder {
        forceQuery
    )

+    override fun deepCopy(): UrlBuilder = UrlBuilder().apply {
+        scheme = [email protected]


style

another ambiguous this. If it works as expected I suppose it's fine but I find something like this more readable:

override fun deepCopy(): UrlBuilder { val builder = this return UrlBuilder().apply { scheme = builder.scheme ... } }

aajtodd · 2021-10-11T19:00:31Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/util/CanDeepCopy.kt

+
+package aws.smithy.kotlin.runtime.http.util
+
+interface CanDeepCopy<T> {


style

out T

aajtodd · 2021-10-11T19:02:12Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/util/StringValuesMap.kt

-        if (caseInsensitiveName) CaseInsensitiveMap<List<String>>().apply { putAll(initialValues) } else values.toMap()
+    protected val values: Map<String, List<String>> = run {
+        // Make a defensive copy so modifications to the initialValues don't mutate our internal copy
+        val copiedValues = initialValues.deepCopy()


Which argument where?

aajtodd · 2021-10-11T19:11:35Z

...en/src/main/kotlin/software/amazon/smithy/kotlin/codegen/retries/StandardRetryIntegration.kt

+                }
+                name = "retryStrategy"
+                documentation = """
+                    The [RetryStrategy] implementation to use for service calls. All API calls will be wrapped by the


correctness

This won't resolve in documentation I don't think. I believe you'll need to give the full import path [aws.smithy.kotlin.*.RetryStrategy]. Ignore if I'm wrong.

Hmm, looks like the documentation isn't used at all right now because we only write KDocs on the DslBuilder and we only put mutable properties in the DslBuilder.

If and when we do make this mutable, I believe it should work as-is because the symbol type of RetryStrategy gets imported (which I can verify by manually inspecting a generated client).

aajtodd · 2021-10-11T19:24:26Z

...en/src/main/kotlin/software/amazon/smithy/kotlin/codegen/retries/StandardRetryIntegration.kt

+        override val name: String = "RetryFeature"
+
+        override fun renderConfigure(writer: KotlinWriter) {
+            writer.declareSection(RetryConfigSection) {


this isn't quite what I had in mind..

There's two things here. The (1) config property and the (2) middleware.

For (1) just add the config property to the list of default properties if we are taking a stance that all SDK clients should be generated with a retry strategy (and I think we should). We have a way to override this if needed already to remove default props. No need for this to be in an integration.

For (2) you don't need a Section. Since it's a middleware just wholesale replace the thing in aws-sdk-kotlin. You can make it re-usable if you want or just duplicate it to a new HttpFeatureMiddleware type there. Since it's not a lot of code I'd probably just duplicate it personally.

I'm not going to die on this hill, if you think they belong as a KotlinIntegration I'll live with it. It just seems at odds with what we've done before.

aajtodd · 2021-10-11T19:27:34Z

...en/src/main/kotlin/software/amazon/smithy/kotlin/codegen/retries/StandardRetryIntegration.kt

+            }
+        )
+
+    override fun customizeMiddleware(


correctness

Should probably only customize the middleware if it's an HTTP protocol. Another reason to maybe move this to a set of default HTTP middleware instead of an integration.

aajtodd · 2021-10-11T19:30:37Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/HttpBody.kt

@@ -35,6 +44,14 @@ sealed class HttpBody {
         * Provides [ByteArray] to be sent to peer
         */
        abstract fun bytes(): ByteArray
+
+        override fun deepCopy(): Bytes = object : Bytes() {


comment

I don't love that we are going to deep copy a payload. I'm not convinced this type should implement CanDeepCopy

aajtodd · 2021-10-11T19:34:01Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/middleware/RetryFeature.kt

+                    val reqCopy = req.deepCopy()
+
+                    // Reset bodies back to beginning (mainly for streaming bodies)
+                    reqCopy.subject.body.reset()


comment

I don't know that I love how much of retry has seeped into HttpBody. It seems like a lot of these concerns are localized to retry middleware.

kggilmer

Very clean, nice work!

feat: add codegen wrappers for retries

dfd0a6b

ianbotsf requested review from aajtodd and kggilmer October 5, 2021 21:06

kggilmer approved these changes Oct 5, 2021

View reviewed changes

aajtodd reviewed Oct 6, 2021

View reviewed changes

ianbotsf and others added 2 commits October 8, 2021 17:28

fix broken protocol tests; rework codegen hierarchy

cb8cfad

Merge branch 'main' into retry-codegen

e07a541

aajtodd reviewed Oct 11, 2021

View reviewed changes

ianbotsf commented Oct 11, 2021

View reviewed changes

remove erroneous WaiterGenerator; add body resets during retry

86975a2

aajtodd reviewed Oct 11, 2021

View reviewed changes

ianbotsf and others added 3 commits October 11, 2021 23:04

replace integration with middlware and statically-specified config

fcf4bfe

fixing missing KAML version spec

3f16d51

Merge branch 'main' into retry-codegen

5cf9224

aajtodd approved these changes Oct 12, 2021

View reviewed changes

Merge branch 'main' into retry-codegen

6c3f563

kggilmer approved these changes Oct 12, 2021

View reviewed changes

ianbotsf merged commit d887259 into main Oct 12, 2021

ianbotsf deleted the retry-codegen branch October 12, 2021 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add codegen wrappers for retries #490

feat: add codegen wrappers for retries #490

ianbotsf commented Oct 5, 2021

kggilmer left a comment

kggilmer Oct 5, 2021

kggilmer Oct 5, 2021

aajtodd left a comment

aajtodd Oct 6, 2021

ianbotsf Oct 6, 2021

aajtodd Oct 6, 2021

ianbotsf Oct 6, 2021

ianbotsf commented Oct 6, 2021

aajtodd left a comment

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

ianbotsf Oct 11, 2021

aajtodd Oct 11, 2021

ianbotsf Oct 11, 2021

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

ianbotsf Oct 11, 2021

aajtodd left a comment

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

ianbotsf Oct 11, 2021

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

aajtodd Oct 11, 2021

kggilmer left a comment


		package aws.smithy.kotlin.runtime.http.util

		interface CanDeepCopy<T> {

feat: add codegen wrappers for retries #490

feat: add codegen wrappers for retries #490

Conversation

ianbotsf commented Oct 5, 2021

Issue #

Description of changes

Scope

kggilmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aajtodd 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

ianbotsf commented Oct 6, 2021

aajtodd 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

aajtodd 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

Choose a reason for hiding this comment

kggilmer left a comment

Choose a reason for hiding this comment