fix(ddl_client): sleep for a while before retry once meta server is busy

cmake_modules/BaseFunctions.cmake

@@ -173,6 +173,7 @@ endfunction(dsn_add_object)
 
 function(dsn_add_test)
   if(${BUILD_TEST})
+    add_definitions(-DPEGASUS_UNIT_TEST)
     add_definitions(-DGTEST_HAS_TR1_TUPLE=0 -DGTEST_USE_OWN_TR1_TUPLE=0)
     set(MY_EXECUTABLE_IS_TEST TRUE)
     dsn_add_executable()

run.sh

@@ -345,6 +345,7 @@ function run_test()
       detect_hotspot_test
       dsn_aio_test
       dsn_block_service_test
+      dsn_client_test
       dsn.failure_detector.tests
       dsn_http_test
       dsn_meta_state_tests

src/client/CMakeLists.txt

@@ -39,3 +39,5 @@ set(MY_PROJ_LIBS "")
 set(MY_BINPLACES "")
 
 dsn_add_static_library()
+
+add_subdirectory(test)

src/client/replication_ddl_client.cpp

@@ -64,10 +64,9 @@ DSN_DEFINE_uint32(ddl_client,
                   "The max count that attempt for failed requests to meta server.");
 
 DSN_DEFINE_uint32(ddl_client,
-                  ddl_client_busy_retry_interval_ms,
+                  ddl_client_retry_interval_ms,
                   10 * 1000,
-                  "The retry interval after receiving ERR_BUSY_CREATING or ERR_BUSY_DROPPING from"
-                  "meta server.");
+                  "The retry interval after receiving error from meta server.");
 
 namespace dsn {
 namespace replication {
@@ -1448,37 +1447,35 @@ bool replication_ddl_client::valid_app_char(int c)
 
 namespace {
 
-bool is_busy(const dsn::error_code &err)
+inline bool is_busy(const dsn::error_code &err)
 {
     return err == dsn::ERR_BUSY_CREATING || err == dsn::ERR_BUSY_DROPPING;
 }
 
 bool need_retry(uint32_t attempt_count, uint32_t busy_attempt_count, const dsn::error_code &err)
 {
+    // For successful request, no need to retry.
     if (err == dsn::ERR_OK) {
         return false;
     }
 
+    // The request, albeit failed, has not reached the max attempt count. Thus just do retry.
     if (attempt_count < FLAGS_ddl_client_max_attempt_count) {
         return true;
     }
 
+    // Do NOT retry, since it has reached FLAGS_ddl_client_max_attempt_count and the error that
+    // led to fail of the last attempt is not busy error.
     if (!is_busy(err)) {
         return false;
     }
 
+    // Though it has reached FLAGS_ddl_client_max_attempt_count, retry is still allowed as long
+    // as the busy attempt count has not reached FLAGS_ddl_client_max_attempt_count.
     return busy_attempt_count < FLAGS_ddl_client_max_attempt_count;
 }
 
-bool should_sleep_before_retry(const dsn::error_code &err, uint32_t &sleep_ms)
-{
-    if (is_busy(err)) {
-        sleep_ms = FLAGS_ddl_client_busy_retry_interval_ms;
-        return true;
-    }
-
-    return false;
-}
+bool should_sleep_before_retry(const dsn::error_code &err) { return is_busy(err); }
 
 } // anonymous namespace
 
@@ -1506,20 +1503,22 @@ void replication_ddl_client::end_meta_request(const rpc_response_task_ptr &callb
         return;
     }
 
-    uint32_t sleep_ms = 0;
-    if (should_sleep_before_retry(err, sleep_ms)) {
+    if (should_sleep_before_retry(err)) {
         LOG_WARNING("sleep {} milliseconds before launch another attempt for {}: err={}",
-                    sleep_ms,
+                    FLAGS_ddl_client_retry_interval_ms,
                     request->local_rpc_code,
                     err);
-        std::this_thread::sleep_for(std::chrono::milliseconds(sleep_ms));
+        std::this_thread::sleep_for(std::chrono::milliseconds(FLAGS_ddl_client_retry_interval_ms));
     }
 
     rpc::call(_meta_server,
               request,
               &_tracker,
               [this, attempt_count, busy_attempt_count, callback](
                   error_code err, dsn::message_ex *request, dsn::message_ex *response) mutable {
+#ifdef PEGASUS_UNIT_TEST
+                  err = get_mock_error();
+#endif
                   end_meta_request(
                       callback, attempt_count, busy_attempt_count, err, request, response);
               });

src/client/replication_ddl_client.h

@@ -26,6 +26,10 @@
 
 #pragma once
 
+#ifdef PEGASUS_UNIT_TEST
+#include <deque>
+#endif
+#include <gtest/gtest_prod.h>
 #include <stdint.h>
 #include <map>
 #include <memory>
@@ -50,6 +54,9 @@
 #include "utils/autoref_ptr.h"
 #include "utils/error_code.h"
 #include "utils/errors.h"
+#ifdef PEGASUS_UNIT_TEST
+#include "utils/fmt_logging.h"
+#endif
 
 namespace dsn {
 class gpid;
@@ -279,6 +286,9 @@ class replication_ddl_client
                   &_tracker,
                   [this, task](
                       error_code err, dsn::message_ex *request, dsn::message_ex *response) mutable {
+#ifdef PEGASUS_UNIT_TEST
+                      err = get_mock_error();
+#endif
                       end_meta_request(std::move(task), 0, 0, err, request, response);
                   });
         return task;
@@ -346,6 +356,22 @@ class replication_ddl_client
     dsn::task_tracker _tracker;
     uint32_t _max_wait_secs = 3600; // Wait at most 1 hour by default.
 
+#ifdef PEGASUS_UNIT_TEST
+    FRIEND_TEST(DDLClientTest, RetryEndMetaRequest);
+    void set_mock_errors(const std::vector<dsn::error_code> &mock_errors)
+    {
+        _mock_errors.assign(mock_errors.begin(), mock_errors.end());
+    }
+    dsn::error_code get_mock_error()
+    {
+        CHECK_FALSE(_mock_errors.empty());
+        auto err = _mock_errors.front();
+        _mock_errors.pop_front();
+        return err;
+    }
+    std::deque<dsn::error_code> _mock_errors;
+#endif
+
     typedef rpc_holder<detect_hotkey_request, detect_hotkey_response> detect_hotkey_rpc;
     typedef rpc_holder<query_disk_info_request, query_disk_info_response> query_disk_info_rpc;
     typedef rpc_holder<add_new_disk_request, add_new_disk_response> add_new_disk_rpc;

src/client/test/CMakeLists.txt

@@ -0,0 +1,37 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+set(MY_PROJ_NAME dsn_client_test)
+
+set(MY_PROJ_SRC "")
+
+set(MY_SRC_SEARCH_MODE "GLOB")
+
+set(MY_PROJ_LIBS
+    dsn_client
+    dsn_replication_common
+    dsn_runtime
+    dsn_utils
+    gtest
+    gtest_main
+    )
+
+set(MY_BINPLACES
+    "${CMAKE_CURRENT_SOURCE_DIR}/run.sh"
+)
+
+dsn_add_test()

src/client/test/ddl_client_test.cpp

@@ -0,0 +1,62 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "client/replication_ddl_client.h"
+#include "common/replication.codes.h"
+#include "meta_admin_types.h"
+#include "utils/flags.h"
+
+DSN_DECLARE_uint32(ddl_client_max_attempt_count);
+DSN_DECLARE_uint32(ddl_client_retry_interval_ms);
+
+namespace dsn {
+namespace replication {
+
+TEST(DDLClientTest, RetryEndMetaRequest)
+{
+    struct test_case
+    {
+        std::vector<error_code> mock_errors;
+    } tests[] = {
+        {{ERR_TIMEOUT, ERR_BUSY_CREATING, ERR_BUSY_CREATING, ERR_BUSY_CREATING}},
+    };
+
+    auto reserved_ddl_client_max_attempt_count = FLAGS_ddl_client_max_attempt_count;
+    FLAGS_ddl_client_max_attempt_count = 3;
+
+    auto reserved_ddl_client_retry_interval_ms = FLAGS_ddl_client_retry_interval_ms;
+    FLAGS_ddl_client_retry_interval_ms = 100;
+
+    std::vector<rpc_address> meta_list = {{"127.0.0.1", 34601}};
+    auto req = std::make_shared<configuration_create_app_request>();
+    for (const auto &test : tests) {
+        auto ddl_client = std::make_unique<replication_ddl_client>(meta_list);
+        auto resp_task =
+            ddl_client->request_meta<configuration_create_app_request>(RPC_CM_CREATE_APP, req);
+        resp_task->wait();
+
+        EXPECT_TRUE(ddl_client->_mock_errors.empty());
+    }
+
+    FLAGS_ddl_client_retry_interval_ms = reserved_ddl_client_retry_interval_ms;
+    FLAGS_ddl_client_max_attempt_count = reserved_ddl_client_max_attempt_count;
+}
+
+} // namespace replication
+} // namespace dsn

src/client/test/run.sh

@@ -0,0 +1,24 @@
+#!/bin/sh
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+# 
+#   http://www.apache.org/licenses/LICENSE-2.0
+# 
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+if [ -z "${REPORT_DIR}" ]; then
+    REPORT_DIR="."
+fi
+
+output_xml="${REPORT_DIR}/dsn_client_test.xml"
+GTEST_OUTPUT="xml:${output_xml}" ./dsn_client_test

src/server/test/CMakeLists.txt

@@ -60,7 +60,6 @@ set(MY_PROJ_LIBS
         gmock
         hashtable
         )
-add_definitions(-DPEGASUS_UNIT_TEST)
 add_definitions(-DENABLE_FAIL)
 
 set(MY_BOOST_LIBS Boost::system Boost::filesystem Boost::regex)