feat!: Create retry modules #710

jbelkins · 2024-05-03T05:42:55Z

Issue #

awslabs/aws-sdk-swift#1386

Description of changes

Modularize retries per Smithy reference architecture.
SmithyRetriesAPI module created with retry-related protocols and data types
SmithyRetries module created with default implementations for retry-related types
WeatherSDK generated code updated to use modular retry

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.

jbelkins · 2024-05-06T15:04:01Z

Package.swift

+        .testTarget(
+            name: "SmithyRetriesTests",
+            dependencies: ["ClientRuntime", "SmithyRetriesAPI", "SmithyRetries"]
+        ),


Create Swift targets for SmithyRetriesAPI and SmithyRetries (these are both defined in SRA).

SmithyRetriesAPI has no tests because it consists only of interfaces & types intended solely for holding data, such as configuration types.

…wift into jbe/modular_retries

jbelkins · 2024-05-09T19:13:08Z

.swiftlint.yml

-  - Tests/SmithyXMLTests/*
-  - Tests/SmithyTimestampsTests/*
-  - Tests/WeatherSDKTests/*
+  - Tests/*


No need to list these test exceptions to lint rules 1 by 1

jbelkins · 2024-05-09T19:14:04Z

Sources/ClientRuntime/Config/DefaultClientConfiguration.swift

@@ -5,6 +5,8 @@
 // SPDX-License-Identifier: Apache-2.0
 //

+import struct SmithyRetriesAPI.RetryStrategyOptions
+


Separate imports have to be added in many files for the retry types extracted into modules.
I won't comment in these files every time those are added.

jbelkins · 2024-05-09T19:16:16Z

Sources/ClientRuntime/Config/DefaultSDKRuntimeConfiguration.swift

-    static var defaultRetryStrategyOptions: RetryStrategyOptions { RetryStrategyOptions() }
+    static var defaultRetryStrategyOptions: RetryStrategyOptions {
+        RetryStrategyOptions(backoffStrategy: ExponentialBackoffStrategy())
+    }


Since RetryStrategyOptions (in SmithyRetriesAPI) has been split from ExponentialBackoffStrategy (an implementation in SmithyRetries) the backoff strategy must be provided to the retry strategy options when it is created by the caller.

jbelkins · 2024-05-09T19:17:00Z

Sources/SmithyRetries/DefaultRetryStrategy/DefaultRetryStrategy.swift

-enum RetryError: Error {
-    case maxAttemptsReached
-    case insufficientQuota
-}


This error was made public and moved to its own file in SmithyRetriesAPI.

jbelkins · 2024-05-09T19:18:40Z

Sources/SmithyRetries/RetryErrorType+CRT.swift

+import enum SmithyRetriesAPI.RetryErrorType
+import AwsCommonRuntimeKit
+
+public extension RetryErrorType {


This extension was split from the declaration of RetryErrorType, which was placed in SmithyRetriesAPI.

This extension goes into the implementation module SmithyRetries since it is dependent on CRT.

jbelkins · 2024-05-09T19:19:48Z

Sources/SmithyRetriesAPI/RetryError.swift

+public enum RetryError: Error {
+    case maxAttemptsReached
+    case insufficientQuota
+}


This type was originally declared along with the DefaultRetryStrategy and was internal.

Now, it's public and in the SmithyRetriesAPI module.

Is this actually used by/part of SmithyRetriesAPI? I don't see mention of the 'quota' concept anywhere in API, but it is here (unless I've missed something). I also don't see it used anywhere, presumably its meant to be the type of error that could be thrown by refreshTokenForRetry?

Also, might be a good time to add some more swift docs on what each of these types are, what they do, etc. (in general, not just for RetryError). Like RetryErrorInfo, RetryErrorType, RetryStrategy.

jbelkins · 2024-05-09T19:20:28Z

Sources/SmithyRetriesAPI/RetryErrorType.swift

-        case .clientError: return .clientError
-        }
-    }
-}


This CRT-dependent extension was split off to its own file in SmithyRetries (see above.)

jbelkins · 2024-05-09T19:21:55Z

Sources/SmithyRetriesAPI/RetryStrategyOptions.swift

@@ -50,7 +50,7 @@ public struct RetryStrategyOptions {
    ///   - availableCapacity: The number of available tokens in a retry quota.  Defaults to 500.
    ///   - maxCapacity: The max number of tokens in a retry quota.  Defaults to 500.
    public init(
-        backoffStrategy: RetryBackoffStrategy = ExponentialBackoffStrategy(),
+        backoffStrategy: RetryBackoffStrategy,


ExponentialBackoffStrategy was moved off to the implementation module, so this default value can't be set anymore - backoff strategy must be passed in when RetryStrategyOptions is created.

jbelkins · 2024-05-09T19:23:19Z

Sources/WeatherSDK/models/Baz.swift

@@ -3,7 +3,7 @@
 import ClientRuntime

 extension WeatherClientTypes {
-    public struct Baz: Swift.Equatable {
+    public struct Baz {


Not commenting on individual WeatherSDK files.

Some of the changes seen in WeatherSDK generated code are from features prior to this PR.

jbelkins · 2024-05-09T19:23:59Z

Tests/ClientRuntimeTests/OrchestratorTests/OrchestratorTests.swift

@@ -8,6 +8,8 @@
 import XCTest

 @testable import ClientRuntime
+import SmithyRetriesAPI
+import SmithyRetries

 class OrchestratorTests: XCTestCase {


Just a few changes related to the relocation of retry types.

jbelkins · 2024-05-09T19:24:46Z

smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/ClientRuntimeTypes.kt

@@ -120,8 +120,6 @@ object ClientRuntimeTypes {
        val SDKLogLevel = runtimeSymbol("SDKLogLevel")
        val ClientLogMode = runtimeSymbol("ClientLogMode")
        val IdempotencyTokenGenerator = runtimeSymbol("IdempotencyTokenGenerator")
-        val DefaultRetryStrategy = runtimeSymbol("DefaultRetryStrategy")
-        val RetryStrategyOptions = runtimeSymbol("RetryStrategyOptions")


Symbols moved to a separate file for retry module types

jbelkins · 2024-05-09T19:25:19Z

smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/SwiftDependency.kt

+        "https://github.com/smithy-lang/smithy-swift",
+        Resources.computeAbsolutePath("smithy-swift", "", "SMITHY_SWIFT_CI_DIR"),
+        "smithy-swift"
+    ),


Our two new modules defined as Swift dependencies

jbelkins · 2024-05-09T19:26:07Z

...rc/main/kotlin/software/amazon/smithy/swift/codegen/integration/HttpProtocolServiceClient.kt

@@ -124,6 +124,7 @@ open class HttpProtocolServiceClient(
            val properties: List<ConfigProperty> = ctx.integrations
                .flatMap { it.clientConfigurations(ctx).flatMap { it.getProperties(ctx) } }
                .let { overrideConfigProperties(it) }
+            properties.forEach { writer.addImport(it.type) }


Making sure imports are added as needed

jbelkins · 2024-05-09T19:29:16Z

smithy-swift-codegen/src/test/kotlin/ContentMd5MiddlewareTests.kt

@@ -7,35 +7,7 @@ class ContentMd5MiddlewareTests {
        val context = setupTests("Isolated/contentmd5checksum.smithy", "aws.protocoltests.restxml#RestXml")
        val contents = getFileContents(context.manifest, "/RestXml/RestXmlProtocolClient.swift")
        val expectedContents = """
-    public func idempotencyTokenWithStructure(input: IdempotencyTokenWithStructureInput) async throws -> IdempotencyTokenWithStructureOutput {


This test is ridiculously overbroad for "generates ContentMD5 middleware" so I cut out the expected Swift code unrelated to that test assertion.

jbelkins · 2024-05-09T19:29:52Z

.../main/kotlin/software/amazon/smithy/swift/codegen/integration/middlewares/RetryMiddleware.kt

 import software.amazon.smithy.swift.codegen.SwiftWriter
 import software.amazon.smithy.swift.codegen.integration.ProtocolGenerator
 import software.amazon.smithy.swift.codegen.integration.middlewares.handlers.MiddlewareShapeUtils
 import software.amazon.smithy.swift.codegen.middleware.MiddlewarePosition
 import software.amazon.smithy.swift.codegen.middleware.MiddlewareRenderable
 import software.amazon.smithy.swift.codegen.middleware.MiddlewareStep
+import software.amazon.smithy.swift.codegen.swiftmodules.SmithyRetriesTypes

 class RetryMiddleware(


Adjust imports and a little code cleanup.

jbelkins · 2024-05-09T19:31:18Z

smithy-swift-codegen/src/test/kotlin/IdempotencyTokenTraitTests.kt

@@ -6,34 +6,7 @@ class IdempotencyTokenTraitTests {
        val context = setupTests("Isolated/idempotencyToken.smithy", "aws.protocoltests.restxml#RestXml")
        val contents = getFileContents(context.manifest, "/RestXml/RestXmlProtocolClient.swift")
        val expectedContents = """
-    public func idempotencyTokenWithStructure(input: IdempotencyTokenWithStructureInput) async throws -> IdempotencyTokenWithStructureOutput {


Like the test above, the expectation is way way broader than "generates idempotent middleware".

milesziemer · 2024-05-09T20:22:43Z

Sources/SmithyRetriesAPI/RetryError.swift

+public enum RetryError: Error {
+    case maxAttemptsReached
+    case insufficientQuota
+}


Is this actually used by/part of SmithyRetriesAPI? I don't see mention of the 'quota' concept anywhere in API, but it is here (unless I've missed something). I also don't see it used anywhere, presumably its meant to be the type of error that could be thrown by refreshTokenForRetry?

milesziemer · 2024-05-09T20:26:51Z

Sources/SmithyRetriesAPI/RetryError.swift

+public enum RetryError: Error {
+    case maxAttemptsReached
+    case insufficientQuota
+}


Also, might be a good time to add some more swift docs on what each of these types are, what they do, etc. (in general, not just for RetryError). Like RetryErrorInfo, RetryErrorType, RetryStrategy.

smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/ClientRuntimeTypes.kt

milesziemer · 2024-05-09T20:35:00Z

smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/ClientRuntimeTypes.kt

Probably would add a lot of noise to this PR, but in the future maybe we consider moving ClientRuntimeTypes into swiftmodules

True. I have many more of these modularization PRs to tackle, I can do it in a future one.

jbelkins · 2024-05-10T20:29:51Z

@milesziemer

Is this actually used by/part of SmithyRetriesAPI? I don't see mention of the 'quota' concept anywhere in API, but it is here (unless I've missed something). I also don't see it used anywhere, presumably its meant to be the type of error that could be thrown by refreshTokenForRetry?

Agreed. .maxAttemptsReached is implied by the retry configuration in SmithyRetriesAPI so it should stay here, but the . insufficientQuota error is really tied to the "adaptive" implementation. I'll move that error back in with the default implementation.

Also no matter where these go, if these errors are public they should have doc comments. I'll add them.

…dd doc comments

jbelkins · 2024-05-14T21:59:08Z

@milesziemer Per discussion above:

The general-case max retries error was split from the adaptive-related quota error, and the quota error was relocated to SmithyRetries with the retry strategy implementation
Doc comments were added to both errors

(Also: fixed a couple unrelated build warnings)

Please re-review.

Sources/SmithyRetriesAPI/RetryErrorType.swift

…ddleware to not force-unwrap

jbelkins and others added 10 commits May 2, 2024 17:06

Created SmithyRetryAPI and SmithyRetry impl modules

0296c29

Publish SmithyRetriesAPI & SmithyRetries

3933cca

Use SmithyRetriesAPI module in codegen

8dde8fc

Fix codegen & codegen tests

5389c3a

Update WeatherSDK

4b6f781

Fix WeatherSDK

0689ecb

Fix swiftlint

07140b0

Add SmithyTestUtil back to WeatherSDK

41e42aa

Merge remote-tracking branch 'origin/main' into jbe/modular_retries

94faf27

Merge branch 'main' into jbe/modular_retries

2ee40f1

jbelkins commented May 6, 2024

View reviewed changes

jbelkins and others added 6 commits May 6, 2024 13:22

Merge branch 'main' into jbe/modular_retries

846046d

Merge branch 'main' into jbe/modular_retries

7a5707c

Fix codegen tests

104e462

Fix build after merging interceptors int this branch

750152a

Merge branch 'jbe/modular_retries' of github.com:smithy-lang/smithy-s…

9543c67

…wift into jbe/modular_retries

Fix protocol tests

c6273ac

jbelkins commented May 9, 2024

View reviewed changes

Fix ktlint

045d75b

jbelkins commented May 9, 2024

View reviewed changes

jbelkins mentioned this pull request May 9, 2024

feat!: Create retry modules awslabs/aws-sdk-swift#1473

Merged

jbelkins marked this pull request as ready for review May 9, 2024 19:42

jbelkins requested review from sichanyoo and dayaffe May 9, 2024 19:42

milesziemer requested changes May 9, 2024

View reviewed changes

jbelkins added 3 commits May 10, 2024 14:41

Merge branch 'main' into jbe/modular_retries

93c5567

Fix tests & regen WeatherSDK

4e1d8a9

Fix swiftlint.yml file

ac0129a

jbelkins added 2 commits May 14, 2024 16:17

Merge branch 'main' into jbe/modular_retries

990d3b0

Split adaptive-related retry error from generic max attempts error; a…

af83cff

…dd doc comments

Fix tests

457d932

milesziemer requested changes May 15, 2024

View reviewed changes

Sources/SmithyRetriesAPI/RetryErrorType.swift Show resolved Hide resolved

jbelkins added 6 commits May 17, 2024 14:06

Merge branch 'main' into jbe/modular_retries

db8c984

Fix codegen tests after merge

7af28e6

Merge branch 'main' into jbe/modular_retries

6ce9ab8

Merge remote-tracking branch 'origin/main' into jbe/modular_retries

2236bc1

Add doc comments to RetryErrorInfo & RetryErrorType, refactor RetryMi…

f4f8342

…ddleware to not force-unwrap

Merge remote-tracking branch 'origin/main' into jbe/modular_retries

cdbfcb8

milesziemer approved these changes May 24, 2024

View reviewed changes

jbelkins merged commit fdc7a1b into main May 24, 2024
17 checks passed

jbelkins deleted the jbe/modular_retries branch May 24, 2024 17:48

jbelkins mentioned this pull request May 24, 2024

chore: Update codegen after retry modularization awslabs/aws-sdk-swift#1527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Create retry modules #710

feat!: Create retry modules #710

jbelkins commented May 3, 2024 •

edited

Loading

jbelkins May 6, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

milesziemer May 9, 2024

milesziemer May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

jbelkins May 9, 2024

milesziemer May 9, 2024

milesziemer May 9, 2024

milesziemer May 9, 2024

jbelkins May 10, 2024

jbelkins commented May 10, 2024

jbelkins commented May 14, 2024 •

edited

Loading

feat!: Create retry modules #710

feat!: Create retry modules #710

Conversation

jbelkins commented May 3, 2024 • edited Loading

Issue #

Description of changes

Scope

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

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

jbelkins commented May 10, 2024

jbelkins commented May 14, 2024 • edited Loading

jbelkins commented May 3, 2024 •

edited

Loading

jbelkins commented May 14, 2024 •

edited

Loading