Conversation
There was a problem hiding this comment.
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
There was a problem hiding this comment.
why are you setting these items and they are not part of the initialization list?
There was a problem hiding this comment.
please remove AI generated comments
There was a problem hiding this comment.
RetryImpl is
struct StandardRetryStrategy::RetryImpl
{
bool newRetriesEnabled = false;
double transientBackoffBaseSec = 0.05;
};
why are we using Pimpl for this? typically Pimpl is used to point at a object that is expected to implement methods, this is just data members
There was a problem hiding this comment.
formatting here is really off, should prefer to change only the code, not formatting so there is a clear dif
There was a problem hiding this comment.
why are we adding a new constructor specifically for the hardcoded value of 0.05, that seems like it should be a implementation detail
2 participants
Issue #, if available:
Description of changes:
Retry Behavior 2.1 core logic behind AWS_NEW_RETRIES_2026 flag
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.