Skip to content
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

feat!: Create retry modules #710

Merged
merged 29 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
0296c29
Created SmithyRetryAPI and SmithyRetry impl modules
jbelkins May 2, 2024
3933cca
Publish SmithyRetriesAPI & SmithyRetries
jbelkins May 2, 2024
8dde8fc
Use SmithyRetriesAPI module in codegen
jbelkins May 2, 2024
5389c3a
Fix codegen & codegen tests
jbelkins May 3, 2024
4b6f781
Update WeatherSDK
jbelkins May 3, 2024
0689ecb
Fix WeatherSDK
jbelkins May 3, 2024
07140b0
Fix swiftlint
jbelkins May 3, 2024
41e42aa
Add SmithyTestUtil back to WeatherSDK
jbelkins May 3, 2024
94faf27
Merge remote-tracking branch 'origin/main' into jbe/modular_retries
jbelkins May 3, 2024
2ee40f1
Merge branch 'main' into jbe/modular_retries
jbelkins May 6, 2024
846046d
Merge branch 'main' into jbe/modular_retries
jbelkins May 6, 2024
7a5707c
Merge branch 'main' into jbe/modular_retries
jbelkins May 8, 2024
104e462
Fix codegen tests
jbelkins May 8, 2024
750152a
Fix build after merging interceptors int this branch
jbelkins May 9, 2024
9543c67
Merge branch 'jbe/modular_retries' of github.com:smithy-lang/smithy-s…
jbelkins May 9, 2024
c6273ac
Fix protocol tests
jbelkins May 9, 2024
045d75b
Fix ktlint
jbelkins May 9, 2024
93c5567
Merge branch 'main' into jbe/modular_retries
jbelkins May 10, 2024
4e1d8a9
Fix tests & regen WeatherSDK
jbelkins May 10, 2024
ac0129a
Fix swiftlint.yml file
jbelkins May 10, 2024
990d3b0
Merge branch 'main' into jbe/modular_retries
jbelkins May 14, 2024
af83cff
Split adaptive-related retry error from generic max attempts error; a…
jbelkins May 14, 2024
457d932
Fix tests
jbelkins May 14, 2024
db8c984
Merge branch 'main' into jbe/modular_retries
jbelkins May 17, 2024
7af28e6
Fix codegen tests after merge
jbelkins May 17, 2024
6ce9ab8
Merge branch 'main' into jbe/modular_retries
jbelkins May 21, 2024
2236bc1
Merge remote-tracking branch 'origin/main' into jbe/modular_retries
jbelkins May 23, 2024
f4f8342
Add doc comments to RetryErrorInfo & RetryErrorType, refactor RetryMi…
jbelkins May 23, 2024
cdbfcb8
Merge remote-tracking branch 'origin/main' into jbe/modular_retries
jbelkins May 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ excluded:
- .build
- Sources/SmithyTestUtil/*
- Sources/WeatherSDK/*
- Tests/ClientRuntimeTests/*
- Tests/SmithyTestUtilTests/*
- Tests/SmithyXMLTests/*
- Tests/SmithyTimestampsTests/*
- Tests/WeatherSDKTests/*
- Tests/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- smithy-swift-codegen-test/build/*

analyzer_rules:
Expand Down
21 changes: 19 additions & 2 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ let package = Package(
],
products: [
.library(name: "ClientRuntime", targets: ["ClientRuntime"]),
.library(name: "SmithyRetriesAPI", targets: ["SmithyRetriesAPI"]),
.library(name: "SmithyRetries", targets: ["SmithyRetries"]),
.library(name: "SmithyReadWrite", targets: ["SmithyReadWrite"]),
.library(name: "SmithyXML", targets: ["SmithyXML"]),
.library(name: "SmithyTestUtil", targets: ["SmithyTestUtil"]),
Expand All @@ -41,6 +43,8 @@ let package = Package(
.target(
name: "ClientRuntime",
dependencies: [
"SmithyRetriesAPI",
"SmithyRetries",
"SmithyXML",
.product(name: "AwsCommonRuntimeKit", package: "aws-crt-swift"),
.product(name: "Logging", package: "swift-log"),
Expand All @@ -49,6 +53,19 @@ let package = Package(
.copy("PrivacyInfo.xcprivacy")
]
),
.target(
name: "SmithyRetriesAPI"
),
.target(
name: "SmithyRetries",
dependencies: [
.product(name: "AwsCommonRuntimeKit", package: "aws-crt-swift"),
]
),
.testTarget(
name: "SmithyRetriesTests",
dependencies: ["ClientRuntime", "SmithyRetriesAPI", "SmithyRetries"]
),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.target(name: "SmithyReadWrite"),
.target(
name: "SmithyXML",
Expand Down Expand Up @@ -89,11 +106,11 @@ func addTestServiceTargets() {
package.targets += [
.target(
name: "WeatherSDK",
dependencies: ["SmithyTestUtil", "ClientRuntime"]
dependencies: ["SmithyTestUtil", "ClientRuntime", "SmithyRetriesAPI", "SmithyRetries"]
),
.testTarget(
name: "WeatherSDKTests",
dependencies: ["WeatherSDK"]
dependencies: ["WeatherSDK", "SmithyTestUtil"]
)
]
}
Expand Down
2 changes: 2 additions & 0 deletions Sources/ClientRuntime/Config/DefaultClientConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
// SPDX-License-Identifier: Apache-2.0
//

import struct SmithyRetriesAPI.RetryStrategyOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public protocol DefaultClientConfiguration: ClientConfiguration {
/// The configuration for retry of failed network requests.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
// SPDX-License-Identifier: Apache-2.0
//

import protocol SmithyRetriesAPI.RetryStrategy
import protocol SmithyRetriesAPI.RetryErrorInfoProvider
import struct SmithyRetriesAPI.RetryStrategyOptions
import struct SmithyRetries.ExponentialBackoffStrategy

/// Provides configuration options for a Smithy-based service.
public struct DefaultSDKRuntimeConfiguration<DefaultSDKRuntimeRetryStrategy: RetryStrategy,
DefaultSDKRuntimeRetryErrorInfoProvider: RetryErrorInfoProvider> {
Expand Down Expand Up @@ -119,7 +124,9 @@ public extension DefaultSDKRuntimeConfiguration {
/// The retry strategy options to use when none is provided.
///
/// Defaults to options with the defaults defined in `RetryStrategyOptions`.
static var defaultRetryStrategyOptions: RetryStrategyOptions { RetryStrategyOptions() }
static var defaultRetryStrategyOptions: RetryStrategyOptions {
RetryStrategyOptions(backoffStrategy: ExponentialBackoffStrategy())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// The log mode to use when none is provided
///
Expand Down
3 changes: 3 additions & 0 deletions Sources/ClientRuntime/Middleware/RetryMiddleware.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import struct Foundation.Locale
import struct Foundation.TimeInterval
import struct Foundation.TimeZone
import struct Foundation.UUID
import protocol SmithyRetriesAPI.RetryStrategy
import protocol SmithyRetriesAPI.RetryErrorInfoProvider
import struct SmithyRetriesAPI.RetryStrategyOptions

public struct RetryMiddleware<Strategy: RetryStrategy,
ErrorInfoProvider: RetryErrorInfoProvider,
Expand Down
3 changes: 3 additions & 0 deletions Sources/ClientRuntime/Orchestrator/Orchestrator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
// SPDX-License-Identifier: Apache-2.0
//

import protocol SmithyRetriesAPI.RetryStrategy
import struct SmithyRetriesAPI.RetryErrorInfo

/// Orchestrates operation execution
///
/// Execution performs the following steps in order:
Expand Down
3 changes: 3 additions & 0 deletions Sources/ClientRuntime/Orchestrator/OrchestratorBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
// SPDX-License-Identifier: Apache-2.0
//

import protocol SmithyRetriesAPI.RetryStrategy
import struct SmithyRetriesAPI.RetryErrorInfo

/// Builds an Orchestrator, combining runtime components, interceptors, serializers, and deserializers.
///
/// Note: This is intended to be used within generated code, not directly.
Expand Down
2 changes: 2 additions & 0 deletions Sources/ClientRuntime/Plugins/DefaultClientPlugin.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
// SPDX-License-Identifier: Apache-2.0
//

import struct SmithyRetries.DefaultRetryStrategy

public class DefaultClientPlugin: Plugin {
public init() {}
public func configureClient(clientConfiguration: ClientConfiguration) {
Expand Down
2 changes: 2 additions & 0 deletions Sources/ClientRuntime/Plugins/HttpClientPlugin.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
// SPDX-License-Identifier: Apache-2.0
//

import struct SmithyRetries.DefaultRetryStrategy

public class DefaultHttpClientPlugin: Plugin {

var httpClientConfiguration: HttpClientConfiguration
Expand Down
2 changes: 2 additions & 0 deletions Sources/ClientRuntime/Plugins/RetryPlugin.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
// SPDX-License-Identifier: Apache-2.0
//

import struct SmithyRetriesAPI.RetryStrategyOptions

public class RetryPlugin: Plugin {

private var retryStrategyOptions: RetryStrategyOptions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
//

import struct Foundation.TimeInterval
import struct SmithyRetriesAPI.RetryErrorInfo
import enum SmithyRetriesAPI.RetryErrorType
import protocol SmithyRetriesAPI.RetryErrorInfoProvider

public enum DefaultRetryErrorInfoProvider: RetryErrorInfoProvider {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
//

import struct Foundation.TimeInterval
import protocol SmithyRetriesAPI.RetryStrategy
import struct SmithyRetriesAPI.RetryStrategyOptions
import struct SmithyRetriesAPI.RetryErrorInfo
import enum SmithyRetriesAPI.RetryError

public struct DefaultRetryStrategy: RetryStrategy {
public typealias Token = DefaultRetryToken
Expand Down Expand Up @@ -55,8 +59,3 @@ public struct DefaultRetryStrategy: RetryStrategy {
await token.quota.retryQuotaRelease(isSuccess: true, capacityAmount: token.capacityAmount)
}
}

enum RetryError: Error {
case maxAttemptsReached
case insufficientQuota
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//

import struct Foundation.TimeInterval
import protocol SmithyRetriesAPI.RetryToken

/// A token that is used to track retry of operations.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//

import struct Foundation.TimeInterval
import struct SmithyRetriesAPI.RetryStrategyOptions

/// Keeps the retry quota count for one partition ID.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
// SPDX-License-Identifier: Apache-2.0
//

import struct SmithyRetriesAPI.RetryStrategyOptions

/// Holds multiple quotas, keyed by partition IDs.
actor RetryQuotaRepository {
let options: RetryStrategyOptions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import func Foundation.pow
import struct Foundation.TimeInterval
import protocol SmithyRetriesAPI.RetryBackoffStrategy

public struct ExponentialBackoffStrategy: RetryBackoffStrategy {
let options: ExponentialBackoffStrategyOptions
Expand Down
21 changes: 21 additions & 0 deletions Sources/SmithyRetries/RetryErrorType+CRT.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//
// Copyright Amazon.com Inc. or its affiliates.
// All Rights Reserved.
//
// SPDX-License-Identifier: Apache-2.0
//

import enum SmithyRetriesAPI.RetryErrorType
import AwsCommonRuntimeKit

public extension RetryErrorType {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


func toCRTType() -> AwsCommonRuntimeKit.RetryError {
switch self {
case .transient: return .transient
case .throttling: return .throttling
case .serverError: return .serverError
case .clientError: return .clientError
}
}
}
11 changes: 11 additions & 0 deletions Sources/SmithyRetriesAPI/RetryError.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//
// Copyright Amazon.com Inc. or its affiliates.
// All Rights Reserved.
//
// SPDX-License-Identifier: Apache-2.0
//

public enum RetryError: Error {
case maxAttemptsReached
case insufficientQuota
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

milesziemer marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
//
// SPDX-License-Identifier: Apache-2.0
//
import AwsCommonRuntimeKit

public enum RetryErrorType: Equatable {

Expand All @@ -22,15 +21,3 @@ public enum RetryErrorType: Equatable {
/// Doesn’t count against any budgets. This could be something like a 401 challenge in HTTP.
case clientError
}

public extension RetryErrorType {

func toCRTType() -> AwsCommonRuntimeKit.RetryError {
switch self {
case .transient: return .transient
case .throttling: return .throttling
case .serverError: return .serverError
case .clientError: return .clientError
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ public struct RetryStrategyOptions {
/// Sets the initial available capacity for this retry strategy's quotas.
///
/// Used only during testing, production uses the default values.
let availableCapacity: Int
public let availableCapacity: Int

/// Sets the maximum capacity for this retry strategy's quotas.
///
/// Used only during testing, production uses the default values.
let maxCapacity: Int
public let maxCapacity: Int

/// Creates a new set of retry strategy options
/// - Parameters:
Expand All @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

maxRetriesBase: Int = 2,
availableCapacity: Int = 500,
maxCapacity: Int = 500,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@
// SPDX-License-Identifier: Apache-2.0
//

import protocol SmithyRetriesAPI.RetryStrategy
import protocol SmithyRetriesAPI.RetryToken
import struct SmithyRetriesAPI.RetryStrategyOptions
import struct SmithyRetriesAPI.RetryErrorInfo
import enum SmithyRetriesAPI.RetryError

@testable import ClientRuntime

public struct MockRetryStrategy: RetryStrategy {
Expand Down
Loading
Loading