Revert commits related to the new RetryPolicy method (#5793)

* Revert "Update the RetryPolicy for the GA release, keeping ShouldRetry extension point hidden. (#5771)"

This reverts commit 9ccd206.

* Revert "Update the RetryPolicy and ShouldRetry customization logic to allow loosening the retry condition. (#5656)"

This reverts commit f1d9552.

* Do not remove changelog entry from a previous beta release

* Revert "Add a virtual ShouldRetry method to the RetryPolicy for customization. (#5584)"

This reverts commit ab90ef6.

---------

Co-authored-by: Anton Kolesnyk <[email protected]>

sdk/core/azure-core/CHANGELOG.md

@@ -9,7 +9,6 @@
 ### Other Changes
 
 - Updated JSON library to 3.11.3.
-- Hide methods on the `RetryPolicy` that are not intended for public use.
 - [[#5622]](https://github.com/Azure/azure-sdk-for-cpp/pull/5622) Documentation fix for building the SDK with specific OpenSSL version.  (A community contribution, courtesy of _[ByteYue](https://github.com/ByteYue)_)
 
 ### Acknowledgments

sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp

@@ -30,14 +30,6 @@
 #include <utility>
 #include <vector>
 
-#if defined(_azure_TESTING_BUILD)
-// Define the classes used from tests
-namespace Azure { namespace Core { namespace Test {
-  class RetryPolicyTest;
-  class RetryLogic;
-}}} // namespace Azure::Core::Test
-#endif
-
 /**
  * A function that should be implemented and linked to the end-user application in order to override
  * an HTTP transport implementation provided by Azure SDK with custom implementation.
@@ -376,11 +368,6 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
         final
 #endif
         : public HttpPolicy {
-#if defined(_azure_TESTING_BUILD)
-      // make tests classes friends
-      friend class Azure::Core::Test::RetryPolicyTest;
-      friend class Azure::Core::Test::RetryLogic;
-#endif
     private:
       RetryOptions m_retryOptions;
 
@@ -415,7 +402,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
        */
       static int32_t GetRetryCount(Context const& context);
 
-    private:
+    protected:
       virtual bool ShouldRetryOnTransportFailure(
           RetryOptions const& retryOptions,
           int32_t attempt,
@@ -428,26 +415,6 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
           int32_t attempt,
           std::chrono::milliseconds& retryAfter,
           double jitterFactor = -1) const;
-
-      /**
-       * @brief Overriding this method customizes the logic of when the RetryPolicy will re-attempt
-       * a request, based on the returned HTTP response.
-       *
-       * @remark A null response pointer means there was no response received from the corresponding
-       * request. Custom implementations of this method that override the retry behavior, should
-       * handle that error case, if that needs to be customized.
-       *
-       * @remark Unless overriden, the default implementation is to always return `false`. The
-       * non-retriable errors, including those specified in the RetryOptions, remain evaluated
-       * before calling ShouldRetry.
-       *
-       * @param response An HTTP response returned corresponding to the request sent by the policy.
-       * @param retryOptions The set of options provided to the RetryPolicy.
-       * @return Whether or not the HTTP request should be sent again through the pipeline.
-       */
-      virtual bool ShouldRetry(
-          std::unique_ptr<RawResponse> const& response,
-          RetryOptions const& retryOptions) const;
     };
 
     /**

sdk/core/azure-core/src/http/retry_policy.cpp

@@ -118,11 +118,6 @@ int32_t RetryPolicy::GetRetryCount(Context const& context)
   return *ptr;
 }
 
-bool RetryPolicy::ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const
-{
-  return false;
-}
-
 std::unique_ptr<RawResponse> RetryPolicy::Send(
     Request& request,
     NextHttpPolicy nextPolicy,
@@ -145,24 +140,9 @@ std::unique_ptr<RawResponse> RetryPolicy::Send(
     {
       auto response = nextPolicy.Send(request, retryContext);
 
-      // Are we out of retry attempts?
-      // Checking this first, before checking the response so that the extension point of
-      // ShouldRetry doesn't have the responsibility of checking the number of retries (again).
-      if (WasLastAttempt(m_retryOptions, attempt))
-      {
-        return response;
-      }
-
-      // If a response is non-retriable (or simply 200 OK, i.e doesn't need to be retried), then
-      // ShouldRetryOnResponse returns false. Service SDKs can inject custom logic to define whether
-      // the request should be retried, based on the response. The default of `ShouldRetry` is
-      // false.
-      // Because of boolean short-circuit evaluation, if ShouldRetryOnResponse returns true, the
-      // overriden ShouldRetry is not called. This is expected, since overriding ShouldRetry enables
-      // loosening the retry conditions (retrying where otherwise the request wouldn't be), but not
-      // strengthening it.
-      if (!ShouldRetryOnResponse(*response.get(), m_retryOptions, attempt, retryAfter)
-          && !ShouldRetry(response, m_retryOptions))
+      // If we are out of retry attempts, if a response is non-retriable (or simply 200 OK, i.e
+      // doesn't need to be retried), then ShouldRetry returns false.
+      if (!ShouldRetryOnResponse(*response.get(), m_retryOptions, attempt, retryAfter))
       {
         // If this is the second attempt and StartTry was called, we need to stop it. Otherwise
         // trying to perform same request would use last retry query/headers
@@ -235,6 +215,12 @@ bool RetryPolicy::ShouldRetryOnResponse(
   using Azure::Core::Diagnostics::Logger;
   using Azure::Core::Diagnostics::_internal::Log;
 
+  // Are we out of retry attempts?
+  if (WasLastAttempt(retryOptions, attempt))
+  {
+    return false;
+  }
+
   // Should we retry on the given response retry code?
   {
     auto const& statusCodes = retryOptions.StatusCodes;