Skip to content

[Feature] New Retry Behavior #3836

Open
kai-ion wants to merge 8 commits into
mainfrom
new-retry
Open

[Feature] New Retry Behavior #3836
kai-ion wants to merge 8 commits into
mainfrom
new-retry

Conversation

@kai-ion
Copy link
Copy Markdown
Collaborator

@kai-ion kai-ion commented Jun 1, 2026

Issue #, if available:

Description of changes:
Retry Behavior 2.1 core logic behind AWS_NEW_RETRIES_2026 flag

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

so can m_retryCost AND m_throttlingRetryCost ever be set and used at the same time? judging from RetryCostClassification you use that enum value to switch between cost implementations, when in reality you should be using a concept like poly morphism to swap implementations. i.e. how you have it now

enum class choice { A, B };

class implementation_choice {
public:
  implementation_choice(int a_impl_value, int b_impl_value, choice choice)
      : a_impl_value_(a_impl_value), b_impl_value_(b_impl_value), choice_(choice) {}
  void do_something() {
    if (choice_ == choice::A) {
      std::cout << a_impl_value_ << "\n";
    } else {
      std::cout << b_impl_value_ << "\n";
    }
  }
private:
  int a_impl_value_;
  int b_impl_value_;
  choice choice_;
};

this is storing both the variant of A and B on the object and making a runtime decesion. something much cleaner would be

class cost_applier {
public:
  virtual ~cost_applier() = default;
  virtual void apply_cost() = 0;
};

class a_cost_applier: public cost_applier {
 public:
  ~a_cost_applier() override;
  void apply_cost() override {
    std::cout << "applied a cost\n";
  }
};

class b_cost_applier: public cost_applier {
 public:
  ~b_cost_applier() override;
  void apply_cost() override {
    std::cout << "applied b cost\n";
  }
};

class implementation_choice {
public:
  implementation_choice(std::unique_ptr<cost_applier> cost_applier)
      : cost_applier_(std::move(cost_applier)) {}
  void do_something() {
    cost_applier_->apply_cost();
  }
private:
  std::unique_ptr<cost_applier> cost_applier_;
};

when we are looking for branching implementations we should lean on polymorphism instead of branch statments

Comment thread src/aws-cpp-sdk-core/source/client/RetryStrategy.cpp Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please remove AI generated comments

Comment thread src/aws-cpp-sdk-core/include/aws/core/client/RetryStrategy.h Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

formatting here is really off, should prefer to change only the code, not formatting so there is a clear dif

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why are we adding a new constructor specifically for the hardcoded value of 0.05, that seems like it should be a implementation detail

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this really doesnt belong in ClientConfiguration.cpp this should be included somewhere in the core retry directory. however you should mark the class as AWS_CORE_LOCAL this is local linkage within the core package, this should be in a header that is not installed with the SDK however it should be available to build the SDK, making the symbol internal to the SDK.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

so you are still using a bool instead of a proper design pattern for this we want to change the implementation based on the value of the environment variable. for example consider a class that uses pimpl.

class Widget {
public:
    Widget();
    ~Widget();

    struct Impl;
private:
    std::unique_ptr<Impl> impl;
};

we want the Impl type to change based on the environment variables. i,e in widget.cpp

struct Widget::Impl {
    virtual ~Impl() = default;
};

namespace {

struct ImplA : Widget::Impl {};

struct ImplB : Widget::Impl {};
}

Widget::Widget() {
    const char* mode = std::getenv("WIDGET_MODE");
    std::string m = mode ? mode : "";

    if (m == "fast") {
        impl = std::make_unique<ImplA>();
    } else {
        impl = std::make_unique<ImplB>();
    }
}

where in this case ImplA is the old standard retry strategy defintion and ImplB is the new retry strategy definition

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants