ENG-15036 add c-client to voltdb-client-cpp #57

kaikaizi · 2018-12-07T21:54:14Z

Innovation project follow-up: a simple C-wrapper for the C++ client.

Together included are:

Current C++ client does not build on my box with latest compiler (Ubuntu-18.4; GCC-7.3.0), because some warnings are now treated as errors; and a ton of deprecated, pre-C++00 usage are treated as warnings. I suppressed those error/warnings.
Cleaned/rewrote build script.
Removed dependency of boost library. We mostly use smart pointers from boost, which are now available in C++11 standard. Use the standard library to replace most boost usage. For a single usage of Boost::Property_Tree, I copied relevant headers to include/ directory. Therefore, building the client no longer needs Boost library.

…rray

boost::atomic and boost::memory_order_*, mutex and locks.

By using C++11 features, and copying 2 boost header files over. Clean up make file and README.

And proof-read/formated README.md

n3world · 2018-12-10T15:51:24Z

include/CAPI.h

+#ifdef __cplusplus
+extern "C" {
+#endif
+   struct c_client;


Wouldn't a prefix of voltdb or something like that be better for name spacing and readability than c_

I would like a c_ prefix to differentiate from C++ world. Yes, I think voltdb is necessary in the name in case some other DB client is linked to the executable. So I'll change it to c_voltdb_client and the same for the rest.

As I have explained previously, I will vote against the c_ prefix. Everything in C++ used the camel case and was under the voltdb namespace, which I think was different enough from C functions. I would recommend not to complicate the naming.

n3world · 2018-12-10T15:53:58Z

include/ClientImpl.h

@@ -163,7 +162,7 @@ class ClientImpl {
    /*
     * Initiate connection based on pending connection instance
     */
-    void initiateConnection(boost::shared_ptr<PendingConnection> &pc) throw (ConnectException, LibEventException, SSLException);
+    void initiateConnection(std::shared_ptr<PendingConnection> &pc);


Doesn't this change break backwards compatibility with the previous public client API? When is it OK to break backwards compatibility?

It does. But I think using C++ STL is the preferred way over 3rd party dependencies.
I'll note this in the README.build file, and poll other's opinion on this.

n3world · 2018-12-10T16:04:05Z

src/CAPI.cpp

+      /*voltdb::ClientAuthHashScheme schema,*/ bool enableAbandon, bool enableTimeout,
+      int timeoutInSec, bool useSSL) {
+   voltdb::ClientConfig* client_config = new voltdb::ClientConfig(
+         usrname, pwd, voltdb::HASH_SHA1/*schema*/, enableAbandon,


I thought SHA1 was deprecated for hashing in volt

I need to change it back to an argument. To be consistent with C++ method argument list. Thanks for the reminder.

n3world · 2018-12-10T16:10:20Z

include/CAPI.h

+   int c_status_code(struct c_invocation_response*);
+   void c_destroy_response(struct c_invocation_response*);
+
+   struct c_stringified_table {


It looks like the client has no way of know what data type each column is and just would have to deal with the string representation of the column which is not very flexible

Right. Supporting all column types would be some extra work, especially with memory management for the C++ world (and all the typings).
I created a follow-up ticket ENG-15044 to track this.

n3world · 2018-12-10T16:11:37Z

include/CAPI.h

+      c_stringified_table* tables;
+   };
+
+   c_stringified_tables* c_exec_result(struct c_invocation_response*);


This isn't really any execution it is more of a conversion to strings

Right, it's just getting result tuples from invocation response. Maybe a better name would be c_voltdb_get_execution_result.

No, too long.

n3world · 2018-12-10T16:13:51Z

src/CAPI.cpp

+   voltdb::ClientConfig* client_config = new voltdb::ClientConfig(
+         usrname, pwd, voltdb::HASH_SHA1/*schema*/, enableAbandon,
+         enableTimeout, timeoutInSec, useSSL);
+   voltdb::Client* client = new voltdb::Client(new voltdb::ClientImpl(*client_config));


What happens if this or any other C++ call throws an exception a C program or anything using the C wrapper wouldn't handle the exception and I think it would be ignored.

Good point, which I chose to ignore but shouldn't.
I'll use NULL with errno to address this.

Why do you have to expose the ClientImpl class?
It was used to be voltdb::Client client = voltdb::Client::create(config);

ethanyzhang · 2018-12-10T15:57:50Z

README.build

@@ -46,7 +23,7 @@ version of libevent - simply replace the libevent tarball.
 Running the tests

 The tests require CPPUnit. You can install cppunit on ubuntu with the
-command: `apt get install libcppunit-dev`
+command: `apt-get install libcppunit-dev`


More likely people need to add sudo to use this command.
Plus, apt get without the dash is a legitimate way to call.
This file was not written using MarkDown. If we do not intend to do so, we shouldn't use `

ethanyzhang · 2018-12-10T15:58:28Z

README.md

@@ -16,14 +16,26 @@ The Linux binary was compiled with GCC 4.4.7 on Centos 6.7.
 The OSX binary was compiled with Xcode 7.2 on OSX 10.11.


ethanyzhang · 2018-12-10T16:00:53Z

README.md

+
+New Features in 8.4
+==================
+- Removed dependency on Boost library (by taking advantage of C++11 features)


Suggested change

- Removed dependency on Boost library (by taking advantage of C++11 features)

- Removed the dependency on the Boost library (by taking advantage of C++11 features)

ethanyzhang · 2018-12-10T16:02:57Z

README.md

+==================
+- Removed dependency on Boost library (by taking advantage of C++11 features)
+
+- Added a C wrapper for a few functions to support synchronous execution of Ad-Hoc query.


Suggested change

- Added a C wrapper for a few functions to support synchronous execution of Ad-Hoc query.

- Added a C wrapper for a few functions to support synchronous execution of Ad-Hoc queries.

ethanyzhang · 2018-12-10T16:04:36Z

README.md

-needs to be instantiated using client config (voltdb::ClientConfig) with the ssl field (m_useSSL) set to true. Code
-snippet:
+
+- Add SSL support to C++ client to enable communication between VoltDB server


Suggested change

- Add SSL support to C++ client to enable communication between VoltDB server

- Added the SSL support to enable communications with a VoltDB server

ethanyzhang · 2018-12-11T19:05:51Z

src/CAPI.cpp

+
+c_procedure* c_create_procedure(const char* name,
+      int nparams, voltdb::Parameter* params) {
+   c_procedure* proc = new c_procedure;


Suggested change

c_procedure* proc = new c_procedure;

ProcCall* proc = new ProcCall;

ethanyzhang · 2018-12-11T19:06:13Z

src/CAPI.cpp

+         fprintf(stderr, "Client is null\n");
+      }
+      if (proc == nullptr) {
+         fprintf(stderr, "Procedure is null\n");


Suggested change

fprintf(stderr, "Procedure is null\n");

fprintf(stderr, "Procedure call is null\n");

ethanyzhang · 2018-12-11T19:06:59Z

src/CAPI.cpp

+}
+
+c_invocation_response* c_exec_adhoc(struct c_client* client, struct c_procedure* proc, const char* param) {
+   const char* params[] = {param, nullptr};


Why is the nullptr here?

ethanyzhang · 2018-12-11T19:07:46Z

src/Client.cpp

@@ -26,7 +26,7 @@

 namespace voltdb {

-Client Client::create(ClientConfig config) throw (voltdb::Exception, voltdb::LibEventException) {
+Client Client::create(ClientConfig config) {


Why didn't you use this factory method but exposed the ClientImpl? What was the benefit?

ethanyzhang · 2018-12-11T19:13:37Z

src/Distributer.cpp

@@ -194,7 +190,7 @@ void Distributer::updateAffinityTopology(const std::vector<voltdb::Table>& topoT
    const std::string hashMode = hashRow.getString(0);
    //Check if token ring is correct
    if (hashMode.compare("ELASTIC") == 0 ) {
-        boost::scoped_array<char> tokens(new char[realsize]);
+       std::unique_ptr<char[]> tokens(new char[realsize]);


kaikaizi added 7 commits December 3, 2018 13:45

Suppress modern GCC compiler warnings (and errors)

84ef1dd

Use std::shared_ptr in place of boost::shared_ptr and boost::shared_a…

52f4e0d

…rray

Get rid of boost::scoped_ptr and boost FOR_EACH,

f34a6d3

boost::atomic and boost::memory_order_*, mutex and locks.

Removed Boost dependency

0d19704

By using C++11 features, and copying 2 boost header files over. Clean up make file and README.

Bring examples/* up to date

755640e

And proof-read/formated README.md

Added a C wrapper

d87cdba

Made CAPI c-compatible

779d4a5

kaikaizi requested review from Chaozz, wweiss-voltdb and ethanyzhang December 7, 2018 21:54

n3world reviewed Dec 10, 2018

View reviewed changes

ethanyzhang suggested changes Dec 11, 2018

View reviewed changes

		@@ -16,14 +16,26 @@ The Linux binary was compiled with GCC 4.4.7 on Centos 6.7.
		The OSX binary was compiled with Xcode 7.2 on OSX 10.11.

	- Removed dependency on Boost library (by taking advantage of C++11 features)
	- Removed the dependency on the Boost library (by taking advantage of C++11 features)

	- Added a C wrapper for a few functions to support synchronous execution of Ad-Hoc query.
	- Added a C wrapper for a few functions to support synchronous execution of Ad-Hoc queries.

	- Add SSL support to C++ client to enable communication between VoltDB server
	- Added the SSL support to enable communications with a VoltDB server

	c_procedure* proc = new c_procedure;
	ProcCall* proc = new ProcCall;

	fprintf(stderr, "Procedure is null\n");
	fprintf(stderr, "Procedure call is null\n");

ENG-15036 add c-client to voltdb-client-cpp #57

Are you sure you want to change the base?

ENG-15036 add c-client to voltdb-client-cpp #57

Uh oh!

Conversation

kaikaizi commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!