Allow running multiple isolated instances of gaia_db_server #661

simone-gaia · 2021-05-10T21:08:07Z

This change introduces the possibility of running multiple instances of gaia_db_server in complete isolation. Each instance has its own name which is used to create the socket and shm names. Whoever needs to connect to the gaia_db_server (client code, gaiac, gaiat, etc..) has to specify the instance name they want to connect to. If nothing is provided a default instance name is used.

The tests now run each into their own instance of gaia_db_server. The instance name is "randomly" generated at startup.

db

Add session_opts_t struct to pass to begin_session() to allow specifying the instance name to connect to.
- NOTE: this has been hidden from the public API until we ensure there are no global states that can mess across sessions.
Add db_client_config.hpp as internal API to help managing default session_opts_t.
Add server_conf_t class to pass to server_t to configure the server behavior, including the db instance name.
Use the instance name to create the db_server socket and shared memory segments.
Add server_instance_t to help spin up and tear down instances of gaia_db_server.
This is only exposed for testing ATM. It could be used beyond that.

catalog

ddl_executor_t and type_metadata_t store the singleton instance into a thread_local variable since multiple sessions on different instances may be running at the same time.

common

Add API to generate random numbers and strings.

gaiac

Add --instance-name|-n parameter
Remove --destroy-db, it is no longer necessary with --instance-name

gaiat

Add -n parameter

Cmake

Add INSTANCE_NAME parameter to cmake functions
Remove "serialize parallel builds" treick from schemas compilations.
Disable FALTBUFFERS test compilation

Tests

Each test now runs into its own instance of gaia_db_server. This will help running them in parallel in the future.
Explicit call to db_test_base_t::SetUpTestSuite() that initializes the Gaia db instance.
Deprecate the tests that inject the DDL from the environment variable in favor of schema_loader.
test_recovery is now part of ctest.

… all the relevant server settings

…into rondelli-db-server-sessions

…xecutor and type_regoistry thread_local

…into rondelli-db-server-sessions

…rallel

LaurentiuCristofor · 2021-05-11T17:14:19Z

production/db/core/src/db_server_instance.cpp

+    std::vector<const char*> command = get_server_command();
+
+    // TODO this should be wrapped into a if_debug_enabled()
+    std::string command_str;


This looks like Hungarian, except str is a suffix instead of a prefix.

This is very trivial code, functionally, but the naming makes it very mysterious. What exactly is being done here?

Is this meant to be something like:

get_server_command -> get_server_command_line_arguments
command -> command_line_arguments
part -> command_line_argument
command_str -> command_line or just command

I cannot call it get_server_command_line_arguments because it is not just the arguments, but also the command itself. As mentioned in the comment above, I could change to: create_server_stratup_command

You could call it get_server_command_and_arguments then.

Not a big fan of the and within method names, fine this time.

LaurentiuCristofor · 2021-05-11T17:18:39Z

production/db/core/src/db_server_instance.cpp

+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
+        if (-1 == ::execve(command[0], const_cast<char**>(command.data()), nullptr))
+        {
+            ASSERT_UNREACHABLE(fmt::format("Failed to execute '{}'", m_conf.server_exec_path.c_str()));


Punctuation missing in the message.

Again I think this is an abuse of asserts. You should not just turn any fatal error into an assert. There could be any number of reasons why execve() might fail, unrelated to any logical programming error.

What you should be doing here is just using the existing facility throw_system_error() to automatically extract errno and its associated system description. There are many examples of this in the DB code.

Change to system_error.

LaurentiuCristofor · 2021-05-11T17:19:28Z

production/db/core/src/db_server_instance.cpp

+        }
+    }
+
+    gaia_log::sys().debug("Server instance {} started with pid:{}", instance_name(), m_server_pid);


I'd punctuate log messages too, but if this hasn't been done yet, it's probably best left for a separate cleanup change.

We need to come up with guidelines for logging.

They were supposed to be the same as for messages as far as punctuation and capitalization was concerned.
Note that one value is preceded by a colon and the other not - the use of colons should be consistent.

Added punctuation and colon to each message

LaurentiuCristofor · 2021-05-11T17:34:08Z

production/db/core/src/db_server_instance.cpp

+
+            gaia::db::begin_session(session_opts);
+        }
+        catch (gaia::common::system_error& ex)


Weren't we going with just e for exceptions?

LaurentiuCristofor · 2021-05-11T17:38:56Z

production/db/core/tests/test_multiple_server_instances.cpp

+        //  I don't know if this is by design or a bug.
+        //  @chuan to review (if it is a bug I will file a JIRA)
+        std::unique_lock lock(m_gaia_parser_lock);
+        schema_loader_t::instance().load_schema("addr_book.ddl");


I guess @chuan didn't expect to have to load metadata multiple times. This is another reason not to allow clients to access different servers. Our assumption has always been that we deal with 1 server, 1 metadata, etc.

The real problem here, I think, is that a layered design was rejected early on, so now everything (predictably) depends on everything else. Logically, the low-level client API shouldn't depend on anything but the corresponding low-level DB feature set ("dbcore"). In that case, there would be no issue with allowing different sessions in the same process to connect to different servers. The existence of process-global data here is the problem, not the introduction of multiple server instances. Any client state which depends on server state should have session scope.

The existence of process-global data here is the problem, not the introduction of multiple server instances.

The existence of the process-global data is due to the assumption that we only need to care about a single server instance. Obviously, changing that assumption now is a problem. We cannot come and say - "changing the assumption is not a problem, we should not have made it in the first place". We made it and nobody pushed back.

This also has nothing to do with the layered design issues. That is an orthogonal problem. Having a layered design is not incompatible with making the assumption that we work with a single server instance.

The point I was trying to make is that a layered design would have avoided this because the session API would be oblivious to the higher-level features (e.g. the catalog) that are now entangled with the client's process-global state.

"assumption that we only need to care about a single server instance" I think this assumption was a bad assumption to begin with.

I am confused. gaia parser which is just a wrapper around flex/bison generated parser is thread-safe. Executing DDLs in parallel is not multi-thread safe in the sense DDLs do not support txns.

Updated the comment to:

// TODO The gaia parser does not support running // multiple DDL from different threads.

Note that this test is likely to be disabled after the meetings we will have tomorrow about DB sessions.

I think you mean DDL executor. Parser is just a string parsing library.

It's the parser itself that fails, not the DDL executor.

LaurentiuCristofor · 2021-05-11T17:47:43Z

production/inc/gaia_internal/db/db_test_base.hpp

+    {
+        gaia_log::initialize({});
+
+        s_server_instance = server_instance_t{};


Is there a difference between this line and this one:

s_server_instance = server_instance_t();

In this case (no arguments) there is no difference. I always use braces for consistency.

I can see the value of braces at declaration time, but in code it seems confusing to use them when not necessary.

production/system/tests/test_recovery.cpp

LaurentiuCristofor

Finally completed my pass through the entire PR.

My main concern is about introducing the ability for a client to connect to different server instances. I don't think we should support that.

Other than that, the comments I made are just fit&finish type of stuff.

production/catalog/parser/tests/test_ddl_parser.cpp

production/catalog/parser/CMakeLists.txt

chuan · 2021-05-12T17:21:20Z

production/CMakeLists.txt

@@ -146,6 +146,7 @@ endif()
 # Rocksdb.
 add_subdirectory(${ROCKSDB} rocksdb EXCLUDE_FROM_ALL)
 # Flatbuffers.
+set(FLATBUFFERS_BUILD_TESTS OFF)


So that we don't build flatbuffers tests, saving a little bit of build time.

chuan · 2021-05-12T17:28:55Z

production/db/core/src/db_client.cpp

@@ -13,6 +13,7 @@
 #include <thread>

 #include <flatbuffers/flatbuffers.h>
+#include <spdlog/fmt/fmt.h>


If we are going to use fmt, we should use an independent version instead of the embedded or bundled version in spdlog.

Right, this is a temporary work-around. Created a follow-up task: https://gaiaplatform.atlassian.net/browse/GAIAPLAT-938

production/gaia.conf

…into rondelli-db-server-sessions

simone-gaia · 2021-05-18T22:11:04Z

@senderista @LaurentiuCristofor @chuan It was not necessary to customize the reading of the configuration file.What we have is an internal APi that allows setting the default session_options_t:

    // This is not visible to the customer.
    gaia::db::config::session_options_t session_options;
    session_options.db_instance_name = instance_name;
    gaia::db::config::set_default_session_options(session_options);

    // This is visible to the customer.
    gaia::db::begin_session();

When the user call:

    gaia::system::initialize("./gaia.conf", "./gaia_log.conf");

initialize internally call the set_default_session_options using the instance name found in the config file.

That being said, I till intend to make the session_name configurable via Environment variable and also the configuration file modifiable at runtime, in a different PR though.

Reverting change request after meeting.

chuan · 2021-05-19T19:46:46Z

production/db/core/src/db_server_instance.cpp

+// All rights reserved.
+/////////////////////////////////////////////
+
+#include "gaia_internal/db/db_server_instance.hpp"


Should this and db_server_instance.cpp be moved under test since they look like test helper utility?

Yes, you're right, there are other things that should be grouped under a "test" section. I created a jira task for it:

https://gaiaplatform.atlassian.net/browse/GAIAPLAT-954

senderista

Just a couple trivial comments, I don't have time to do a proper review and I trust the general approach.

senderista · 2021-05-19T19:32:43Z

production/catalog/gaiac/src/main.cpp

-        }
+        server_instance_config_t server_conf = server_instance_config_t::get_default();
+        server_conf.instance_name = instance_name;
+        server_conf.server_exec_path = string(path_to_db_server) + "/" + string(c_db_server_exec_name);


Can you normalize this with std::filesystem::path? I want to make this robust since this is user-supplied data.

senderista · 2021-05-19T19:54:46Z

production/db/core/src/db_server_instance.cpp

+
+    if (!fs::exists(db_exec_path))
+    {
+        throw common::gaia_exception("Impossible to find the database server path");


"Database server path does not exist."

simone-gaia added 25 commits April 20, 2021 15:46

Fix some warnings in db_server

4f58dd9

Cleanup unused import in db_client.hpp

3904a5d

Fix warnings in mapped_data

d4558f8

Refactor DB server code to introduce a server_conf_t that encapsulate…

21d564b

… all the relevant server settings

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

67ff0f7

…into rondelli-db-server-sessions

Start stuff

058a1a9

Stuff2

f2ae3f9

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

2c5cef7

…into rondelli-db-server-sessions

Implemented support for multiple server instances

2a6fb61

Add db_client_confoig, add test_multiple_server_instances, make ddl_e…

df74d98

…xecutor and type_regoistry thread_local

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

bd13190

…into rondelli-db-server-sessions

Stuff

bc117bc

Stuff

7cc64ef

Fix persistence test

1ddd494

Fix tests

3adc350

Separate db_server_instance in hpp and cpp

00c4f55

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

beaa0a5

…into rondelli-db-server-sessions

More test fixes

6f5fc08

Add random, fix tests

2b09ce3

Pre-pr fixes

2acd986

Add --instance-name param to gaiac, made the header generation non pa…

5a67f9c

…rallel

Fix airport headers generation

50e6088

Minor formatting

e110bea

Adapt gaiat

3a19fee

Pre PR fixes

5252756

simone-gaia requested review from chuan, senderista, waynelwarren, LaurentiuCristofor and daxhaw May 10, 2021 21:08

LaurentiuCristofor reviewed May 11, 2021

View reviewed changes

production/system/tests/test_recovery.cpp Outdated Show resolved Hide resolved

LaurentiuCristofor previously requested changes May 11, 2021

View reviewed changes

Pr fixes

ef07380

chuan reviewed May 12, 2021

View reviewed changes

production/catalog/parser/tests/test_ddl_parser.cpp Outdated Show resolved Hide resolved

chuan reviewed May 12, 2021

View reviewed changes

production/catalog/parser/CMakeLists.txt Outdated Show resolved Hide resolved

chuan reviewed May 12, 2021

View reviewed changes

simone-gaia added 3 commits May 12, 2021 14:11

Change gaia_parser multi thread comment

99c851d

Fix some clang-tidy warnings

f3ee030

Remove db_instance dependency from test_ddl_parser

d379d56

simone-gaia commented May 18, 2021

View reviewed changes

production/gaia.conf Outdated Show resolved Hide resolved

simone-gaia added 2 commits May 18, 2021 17:36

Hide public API to set sesstion_options_t

ced7716

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

771f1f0

…into rondelli-db-server-sessions

Address PR comments

f1faa1e

LaurentiuCristofor approved these changes May 18, 2021

View reviewed changes

chuan reviewed May 19, 2021

View reviewed changes

Remove thread_local from ddl_executor and type_registry

ca5fbd7

senderista approved these changes May 19, 2021

View reviewed changes

Address PR comments

6eec32a

simone-gaia merged commit f92b0e1 into master May 19, 2021

simone-gaia deleted the rondelli-db-server-sessions branch August 11, 2021 15:06

senderista mentioned this pull request Oct 5, 2021

Remove unnecessary dependency of gaia_db_client on gaia_db_server #977

Merged

Allow running multiple isolated instances of gaia_db_server #661

Allow running multiple isolated instances of gaia_db_server #661

Conversation

simone-gaia commented May 10, 2021 • edited Loading

Choose a reason for hiding this comment

LaurentiuCristofor May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentiuCristofor May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentiuCristofor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-gaia commented May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senderista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-gaia commented May 10, 2021 •

edited

Loading

LaurentiuCristofor May 11, 2021 •

edited

Loading

LaurentiuCristofor May 11, 2021 •

edited

Loading

simone-gaia commented May 18, 2021 •

edited

Loading