Hi Anders, Thanks a lot for your very good comments. I have just sent out the updated patch V2.
Regards, Vu > -----Original Message----- > From: Anders Widell [mailto:[email protected]] > Sent: Monday, November 27, 2017 10:45 PM > To: Vu Minh Nguyen <[email protected]>; > [email protected] > Cc: [email protected] > Subject: Re: [PATCH 1/1] base: create generic try-again handling decorator for > AIS APIs [#2702] > > Hi! > > I think this is a very good and generic approach. I will do some more > reviewing and testing, but here are my initial comments (see below, > marked AndersW>). My major comment is that I don't like the RetryControl > as it is done here. Maybe a better approach would be to provide the > retry control as template parameter(s)? > > regards, > > Anders Widell > > > On 11/24/2017 11:30 AM, Vu Minh Nguyen wrote: > > Make generic C++ python-like decorator handling SA_AIS_ERR_TRY_AGAIN > > return code of AIS APIs. > > --- > > src/base/Makefile.am | 5 +- > > src/base/tests/try_again_decorator_test.cc | 69 +++++++++++++++ > > src/base/try_again_decorator.h | 131 > +++++++++++++++++++++++++++++ > > 3 files changed, 204 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..50ddf15 > > --- /dev/null > > +++ b/src/base/tests/try_again_decorator_test.cc > > @@ -0,0 +1,69 @@ > > +/* -*- 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 > > + * > > + */ > > + > > +#define private public > AndersW> Even though this is just a test case, it is not so good to do > things like this. Not sure exactly why you need it, but if it is only > because you need read access to the retry_ctrl_ variable then why not > add a public getter method for it? > > > +#include "base/try_again_decorator.h" > > +#include "gtest/gtest.h" > > + > > +static unsigned count = 0; > > + > > +SaAisErrorT TestMethod() { > > + ++count; > > + return SA_AIS_ERR_TRY_AGAIN; > > +} > > + > > +TEST(TryAgainDecorator, DecoratorFunction) { > > + auto DecorTestMethod = base::TryAgainDecorator(TestMethod); > > + DecorTestMethod(); > > + EXPECT_GE(count, 200); > > + EXPECT_LE(count, 250); > > + count = 0; > > +} > > + > > +TEST(TryAgainDecorator, DefaultRetryControl) { > > + auto DecorTestMethod = base::TryAgainDecorator(TestMethod); > > + timespec interval = DecorTestMethod.retry_ctrl_->interval; > > + uint64_t timeout = DecorTestMethod.retry_ctrl_->timeout; > > + > > + EXPECT_EQ(interval.tv_sec, 0); > > + EXPECT_EQ(interval.tv_nsec, 40*1000*1000); > > + EXPECT_EQ(timeout, 10*1000); > > +} > > + > > +TEST(TryAgainDecorator, UseLocalRetryControl) { > > + base::RetryControl ctrl({0, 60*1000}, 60); > > + auto DecorTestMethod = base::TryAgainDecorator(TestMethod, ctrl); > > + timespec interval = DecorTestMethod.retry_ctrl_->interval; > > + uint64_t timeout = DecorTestMethod.retry_ctrl_->timeout; > > + > > + EXPECT_EQ(interval.tv_sec, 0); > > + EXPECT_EQ(interval.tv_nsec, 60*1000); > > + EXPECT_EQ(timeout, 60); > > +} > > + > > +TEST(TryAgainDecorator, ChangeGlobalRetryControl) { > > + base::RetryControl ctrl({0, 100*1000}, 200); > > + auto DecorTestMethod = base::TryAgainDecorator(TestMethod); > > + > > + base::ChangeGlobalRetryControl(ctrl); > > + timespec interval = DecorTestMethod.retry_ctrl_->interval; > > + uint64_t timeout = DecorTestMethod.retry_ctrl_->timeout; > > + > > + EXPECT_EQ(interval.tv_sec, 0); > > + EXPECT_EQ(interval.tv_nsec, 100*1000); > > + EXPECT_EQ(timeout, 200); > > +} > > diff --git a/src/base/try_again_decorator.h > b/src/base/try_again_decorator.h > > new file mode 100644 > > index 0000000..1b648f4 > > --- /dev/null > > +++ b/src/base/try_again_decorator.h > > @@ -0,0 +1,131 @@ > > +/* -*- 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 "base/time.h" > > +#include "ais/include/saAis.h" > > AndersW> Please use #include <saAis.h> instead of #include > "ais/include/saAis.h" > > > + > > +namespace base { > > + > > +struct RetryControl { > > + // Sleep time b/w retries > > + timespec interval; > > + // Maximum time for retries (ms) > > + uint64_t timeout; > AndersW> Move these two instance variable declarations to the end of the > class definition (after the methods). > > > + > > + RetryControl() { > > + interval = {0, 40*1000*1000}; // 40 miliseconds > > + timeout = 10*1000; // 10 seconds > > AndersW> Should use a member initalizer list instead of assigning values > to instance variables in the constructor body. Also, missing spaces > around binary operator (multiplication). > > + } > > + > > + explicit RetryControl(timespec i, uint64_t t) { > > + interval = i; > > + timeout = t; > > + } > AndersW> Use member initializer list. Remove the "explicit" keyword. > > + > > + explicit RetryControl(const RetryControl& ctrl) { > > + interval = ctrl.interval; > > + timeout = ctrl.timeout; > > + } > > + > > + RetryControl& operator=(const RetryControl& ctrl) { > > + interval = ctrl.interval; > > + timeout = ctrl.timeout; > > + return *this; > > + } > AndersW> Copy constructor and assignment operator do the same thing as > the default ones, right? So they can be removed, or use = default. > > > +}; > > + > > + > > +//> > > +// Python-like C++ decorator which escapsulates try again handling. > > AndersW> I don't see why this is "Python-like"? Remove that wording and > just call it C++ decorator. > > > +// > > +// E.g: > > +// 1) When user wants to call saClmInitialize() which has try again > > +// handling inside using this decorator, do this: > > +// > > +// auto saClmInitialize = base::TryAgainDecorator(::saClmInitialize); > > +// if (saClmInitialize(handle, cbs, version) != 0) { > AndersW> Shouldn't it be " != SA_AIS_OK" instead of " != 0"? (SA_AIS_OK > has the value 1 by the way. > > > +// // error handling > > +// } > > +// > > +// 2) When user wants other retry control than default ones, pass > > +// your owned retry control information to decorator. > > +// > > +// const base::RetryControl ctrl({0, 60*1000*1000}, 60*1000); > > +// auto saClmInitialize = TryAgainDecorator(::saClmInitialize, ctrl); > > +// if (saClmInitialize(handle, cbs, version) != 0) { > > +// // error handling > > +// } > > +// > > +//< > > + > > +static RetryControl global_retry_ctrl; > > AndersW> A static variable declaration in the header file? So you will > get a separate global_retry_ctrl in each compilation unit that includes > this header file. Seems very confusing to me, and you shouldn't call it > "global" when it's not. > > > + > > +// Change default retry control information to given one. > > +// Any call to decorators after this call will take the given > > +// retry control. > > +inline void ChangeGlobalRetryControl(const RetryControl& ctrl) { > > + global_retry_ctrl = ctrl; > > +} > > + > > +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) > > + : Decorator(f, global_retry_ctrl) {} > > + > > + ~Decorator() { } > AndersW> Is this destructor needed? > > + > > + 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) { > > + base::Sleep(retry_ctrl_->interval); > > + continue; > > + } > > + break; > AndersW> Wouldn't the code above be simpler if you did "if (ais_error != > SA_AIS_ERR_TRY_AGAIN) break;"? > > + } > > + return ais_error; > > + } > > + > > + private: > > + const std::function<T(Args ...)> f_; > > + const RetryControl* retry_ctrl_; > AndersW> I think it is dangerous to keep a pointer to the RetryControl > and let the caller keep ownership of it, especially when it is passed as > const RetryControl& in the constructor above. That makes me assume that > the constructor makes a copy of the RetryControl. > > > +}; > > + > > +template<class T, class... Args> > > +Decorator<T(Args...)> TryAgainDecorator(T (*f)(Args ...)) { > > + return Decorator<T(Args...)>(std::function<T(Args...)>(f)); > > +} > > + > > +template<class T, class... Args> Decorator<T(Args...)> > > +TryAgainDecorator(T (*f)(Args ...), const RetryControl& ctrl) { > > + 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 [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
