Thanks Hans and Anders for your good comments. The updated patch is just sent out. Please help to have a look. Thanks a lot!
Regards, Vu > -----Original Message----- > From: Hans Nordebäck [mailto:hans.nordeb...@ericsson.com] > Sent: Thursday, November 30, 2017 1:07 PM > To: Anders Widell <anders.wid...@ericsson.com>; Vu Minh Nguyen > <vu.m.ngu...@dektech.com.au> > Cc: opensaf-devel@lists.sourceforge.net > Subject: RE: [PATCH 1/1] base: create generic try-again handling decorator for > AIS APIs [#2702] > > agree, constexpr should be fine. /HansN > > -----Original Message----- > From: Anders Widell > Sent: den 30 november 2017 13:05 > To: Hans Nordebäck <hans.nordeb...@ericsson.com>; Vu Minh Nguyen > <vu.m.ngu...@dektech.com.au> > Cc: opensaf-devel@lists.sourceforge.net > Subject: Re: [PATCH 1/1] base: create generic try-again handling decorator for > AIS APIs [#2702] > > I have two comments: > > 1) I think this should be put under src/ais (but not src/ais/include) instead > of > putting it in src/base. > > 2) Could you think about if it is possible to make the retry time and interval > constexpr? > > regards, > > Anders Widell > > > On 11/30/2017 01:01 PM, Hans Nordebäck wrote: > > Hi Vu, > > > > please see some comments inlined below/Regards HansN > > > > > > On 11/28/2017 02:13 PM, Vu Minh Nguyen wrote: > >> Make generic C++ decorator for handling SA_AIS_ERR_TRY_AGAIN return > >> code of AIS APIs. > >> --- > >> src/base/Makefile.am | 5 +- > >> src/base/tests/try_again_decorator_test.cc | 48 +++++++++++++ > >> src/base/try_again_decorator.h | 109 > >> +++++++++++++++++++++++++++++ > >> 3 files changed, 161 insertions(+), 1 deletion(-) > >> create mode 100644 src/base/tests/try_again_decorator_test.cc > >> create mode 100644 src/base/try_again_decorator.h > >> > >> diff --git a/src/base/Makefile.am b/src/base/Makefile.am index > >> 956cce6..f11b738 100644 > >> --- a/src/base/Makefile.am > >> +++ b/src/base/Makefile.am > >> @@ -150,6 +150,7 @@ noinst_HEADERS += \ > >> src/base/unix_client_socket.h \ > >> src/base/unix_server_socket.h \ > >> src/base/unix_socket.h \ > >> + src/base/try_again_decorator.h \ > >> src/base/usrbuf.h > >> TESTS += bin/testleap bin/libbase_test bin/core_common_test @@ > >> -208,7 +209,9 @@ bin_libbase_test_SOURCES = \ > >> src/base/tests/time_compare_test.cc \ > >> src/base/tests/time_convert_test.cc \ > >> src/base/tests/time_subtract_test.cc \ > >> - src/base/tests/unix_socket_test.cc > >> + src/base/tests/unix_socket_test.cc \ > >> + src/base/tests/try_again_decorator_test.cc > >> + > >> bin_libbase_test_LDADD = \ > >> $(GTEST_DIR)/lib/libgtest.la \ > >> diff --git a/src/base/tests/try_again_decorator_test.cc > >> b/src/base/tests/try_again_decorator_test.cc > >> new file mode 100644 > >> index 0000000..ac8595d > >> --- /dev/null > >> +++ b/src/base/tests/try_again_decorator_test.cc > >> @@ -0,0 +1,48 @@ > >> +/* -*- OpenSAF -*- > >> + * > >> + * (C) Copyright 2017 The OpenSAF Foundation > >> + * > >> + * This program is distributed in the hope that it will be useful, > >> +but > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > >> MERCHANTABILITY > >> + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are > >> licensed > >> + * under the GNU Lesser General Public License Version 2.1, February > >> 1999. > >> + * The complete license can be accessed from the following location: > >> + * http://opensource.org/licenses/lgpl-license.php > >> + * See the Copying file included with the OpenSAF distribution for > >> + full > >> + * licensing terms. > >> + * > >> + * Author(s): Ericsson AB > >> + * > >> + */ > >> + > >> +#include "base/try_again_decorator.h" > >> +#include "gtest/gtest.h" > >> + > >> +static unsigned counter = 0; > > [HansN] use an unnamed namespace instead of static: > > namespace { > > unsigned counter = 0; > > } > > > >> + > >> +SaAisErrorT TestMethod() { > >> + ++counter; > >> + return SA_AIS_ERR_TRY_AGAIN; > >> +} > > [HansN] shouldn't this function be declared extern "C" ? > > > >> + > >> +TEST(TryAgainDecorator, DefaultRetryControl) { > >> + // default interval = 40ms, timeout = 10 * 1000ms > >> + using DefaultDecorator = base::TryAgainDecorator<>; > >> + auto DecorTestMethod = > >> DefaultDecorator::DecorateFunction(TestMethod); > >> + > >> + DecorTestMethod(); > > > > [HansN] the return code from the DecorTestMethod should be validated > >> + EXPECT_GE(counter, 200); > >> + EXPECT_LE(counter, 250); > >> + counter = 0; > >> +} > >> + > >> +TEST(TryAgainDecorator, GivenRetryControl) { > >> + // interval = 10ms, timeout = 1000ms > >> + using TryAgainDecorator = base::TryAgainDecorator<10, 1 * 1000>; > >> + auto DecorTestMethod = > >> TryAgainDecorator::DecorateFunction(TestMethod); > >> + > >> + DecorTestMethod(); > > > > [HansN] the return code from the DecorTestMethod should be validated > >> + EXPECT_GE(counter, 50); > >> + EXPECT_LE(counter, 100); > >> + counter = 0; > >> +} > >> diff --git a/src/base/try_again_decorator.h > >> b/src/base/try_again_decorator.h new file mode 100644 index > >> 0000000..e676697 > >> --- /dev/null > >> +++ b/src/base/try_again_decorator.h > >> @@ -0,0 +1,109 @@ > >> +/* -*- OpenSAF -*- > >> + * > >> + * Copyright Ericsson AB 2017 - All Rights Reserved. > >> + * > >> + * This program is distributed in the hope that it will be useful, > >> +but > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > >> MERCHANTABILITY > >> + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are > >> licensed > >> + * under the GNU Lesser General Public License Version 2.1, February > >> 1999. > >> + * The complete license can be accessed from the following location: > >> + * http://opensource.org/licenses/lgpl-license.php > >> + * See the Copying file included with the OpenSAF distribution for > >> + full > >> + * licensing terms. > >> + * > >> + */ > >> + > >> +#ifndef BASE_TRY_AGAIN_DECORATOR_H_ > >> +#define BASE_TRY_AGAIN_DECORATOR_H_ > >> + > >> +#include <iostream> > >> +#include <functional> > >> +#include <saAis.h> > >> +#include "base/time.h" > >> + > >> +namespace base { > >> + > > [HansN] perhaps we can use a strategy/policy like pattern instead? > > What do you say about something like the following: > > > > > > struct RetryStrategy { > > > > virtual int nTries() = 0; > > > > virtual int retryInterval() = 0; > > > > }; > > > > struct DefaultRetryStrategy : public RetryStrategy { > > > > int nTries() {return 10;} > > > > int retryInterval() {return 10;} > > > > }; > > > > struct AnotherRetryStrategy : public RetryStrategy { > > > > int nTries() {return 100;} > > > > int retryInterval() {return 100;} > > > > }; > > > > DefaultRetryStrategy DfltStrategy; > > > > AnotherRetryStrategy AnotherStrategy; > > > > int SA_AIS_ERR_TRY_AGAIN = 0; > > > > // > > > > template <typename> class Decorator; > > > > template <typename T, typename... Args> > > > > class Decorator<T(Args ...)> { > > > > public: > > > > explicit Decorator(std::function<T(Args...)> f, RetryStrategy > > &strategy) > > > > : f_(f), strategy_(strategy) {} > > > > T operator()(Args... args) { > > > > std::cout << "Call the decorated function" << std::endl; > > > > int rc = f_(args...); > > > > int n_tries = 1; > > > > while (rc == SA_AIS_ERR_TRY_AGAIN && > > > > n_tries < strategy_.nTries()) { > > > > base::Sleep(strategy_.retryInterval()); > > > > rc = f_(args...); > > > > n_tries++; > > > > } > > > > std::cout << "return code is " << rc << std::endl; > > > > std::cout << "add all common retry logic here, etc " << std::endl; > > > > return rc; > > > > } > > > > RetryStrategy &strategy_; > > > > std::function<T(Args ...)> f_; > > > > }; > > > > template<typename T, typename... Args> > > > > Decorator<T(Args...)> make_decorator(T (*f)(Args...), RetryStrategy > > &strategy=DfltStrategy) { > > > > return Decorator<T(Args...)>(std::function<T(Args...)>(f), > > strategy); > > > > } > > > > int main() { > > > > int rc = 0; > > > > auto saImmOmInitialize_d = make_decorator(saImmOmInitialize); > > > > std::cout << typeid(saImmOmInitialize_d).name() << std::endl; > > > > rc = saImmOmInitialize_d(4711); > > > > std::cout << "rc: " << rc << std::endl; > > > > auto saImmOmSelectionObjectGet_d = > > make_decorator(saImmOmSelectionObjectGet, AnotherStrategy); > > > > rc = saImmOmSelectionObjectGet_d(4712, "hello world!"); > > > > : > > > > > >> +//> > >> +// C++ decorator which escapsulates try again handling. > >> +// > >> +// E.g: > >> +// 1) If user wants to call saClmInitialize() which has try again // > >> +handling inside using this decorator, do this: > >> +// > >> +// using TryAgainDecorator = base::TryAgainDecorator<>; // auto > >> +saClmInitialize = TryAgainDecorator(::saClmInitialize); > >> +// if (saClmInitialize(handle, cbs, version) != SA_AIS_OK) { // > >> +// error handling // } // // 2) If user wants other retry control > >> +than default ones, // such as interval = 10ms, timeout = 10second, > >> +do this: > >> +// > >> +// using TryAgainDecorator = base::TryAgainDecorator< // > >> +{0, 10 * 1000 * 1000}, // 10 * 1000 > >> +// > // auto saClmInitialize = > >> +TryAgainDecorator(::saClmInitialize); > >> +// if (saClmInitialize(handle, cbs, version) != SA_AIS_OK) { // > >> +// error handling // } // //< > >> + > >> +// Default sleep time b/w retries {second, ns} static const timespec > >> +kInterval = {0, 40 * 1000 * 1000}; // Default maximum time for retry > >> +loop (ms) static const uint64_t kTimeout = 10 * 1000; > >> + > >> +struct RetryControl { > >> + RetryControl() : interval(kInterval), timeout(kTimeout) {} > >> + RetryControl(timespec i, uint64_t t) : interval(i), timeout(t) {} > >> + RetryControl(const RetryControl& ctrl) > >> + : interval(ctrl.interval), timeout(ctrl.timeout) {} > >> + > >> + // Sleep time b/w retries > >> + timespec interval; > >> + // Maximum time for retry loop > >> + uint64_t timeout; > >> +}; > >> + > >> +template <class> class Decorator; > >> +template <class T, class... Args> > >> +class Decorator<T(Args ...)> { > >> + public: > >> + explicit Decorator(const std::function<T(Args ...)>& f, > >> + const RetryControl& ctrl) > >> + : f_{f}, retry_ctrl_{ctrl} {} > >> + > >> + explicit Decorator(const std::function<T(Args ...)>& f) : f_{f} {} > >> + > >> + T operator()(Args ... args) { > >> + T ais_error; > >> + base::Timer wtime(retry_ctrl_.timeout); > >> + while (wtime.is_timeout() == false) { > >> + ais_error = f_(args...); > >> + if (ais_error != SA_AIS_ERR_TRY_AGAIN) break; > >> + base::Sleep(retry_ctrl_.interval); > >> + } > >> + return ais_error; > >> + } > >> + > >> + private: > >> + const std::function<T(Args ...)> f_; > >> + RetryControl retry_ctrl_; > >> +}; > >> + > >> +template <uint64_t interval_ms = 40, uint64_t timeout_ms = 10 * > >> +1000> class TryAgainDecorator { > >> + public: > >> + template<class T, class... Args> > >> + static Decorator<T(Args...)> DecorateFunction(T (*f)(Args ...)) { > >> + RetryControl ctrl({0, interval_ms * 1000 * 1000}, timeout_ms); > >> + return Decorator<T(Args...)> > >> + (std::function<T(Args...)>(f), ctrl); > >> + } > >> +}; > >> + > >> +} // namespace base > >> + > >> +#endif //< BASE_TRY_AGAIN_DECORATOR_H_ > > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
