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
