Review fixes

programs/library-bridge/HandlerFactory.cpp

@@ -13,7 +13,7 @@ namespace DB
         LOG_DEBUG(log, "Request URI: {}", uri.toString());
 
         if (request.getMethod() == Poco::Net::HTTPRequest::HTTP_GET)
-            return std::make_unique<PingHandler>(keep_alive_timeout, getContext());
+            return std::make_unique<LibraryExistsHandler>(keep_alive_timeout, getContext());
 
         if (request.getMethod() == Poco::Net::HTTPRequest::HTTP_POST)
             return std::make_unique<LibraryRequestHandler>(keep_alive_timeout, getContext());

programs/library-bridge/Handlers.cpp

@@ -17,6 +17,12 @@
 
 namespace DB
 {
+
+namespace ErrorCodes
+{
+    extern const int BAD_REQUEST_PARAMETER;
+}
+
 namespace
 {
     void processError(HTTPServerResponse & response, const std::string & message)
@@ -190,7 +196,7 @@ void LibraryRequestHandler::handleRequest(HTTPServerRequest & request, HTTPServe
         {
             auto library_handler = SharedLibraryHandlerFactory::instance().get(dictionary_id);
             if (!library_handler)
-                throw Exception(ErrorCodes::LOGICAL_ERROR, "Not found dictionary with id: {}", dictionary_id);
+                throw Exception(ErrorCodes::BAD_REQUEST_PARAMETER, "Not found dictionary with id: {}", dictionary_id);
 
             bool res = library_handler->isModified();
             writeStringBinary(std::to_string(res), out);
@@ -199,7 +205,7 @@ void LibraryRequestHandler::handleRequest(HTTPServerRequest & request, HTTPServe
         {
             auto library_handler = SharedLibraryHandlerFactory::instance().get(dictionary_id);
             if (!library_handler)
-                throw Exception(ErrorCodes::LOGICAL_ERROR, "Not found dictionary with id: {}", dictionary_id);
+                throw Exception(ErrorCodes::BAD_REQUEST_PARAMETER, "Not found dictionary with id: {}", dictionary_id);
 
             bool res = library_handler->supportsSelectiveLoad();
             writeStringBinary(std::to_string(res), out);
@@ -208,7 +214,7 @@ void LibraryRequestHandler::handleRequest(HTTPServerRequest & request, HTTPServe
         {
             auto library_handler = SharedLibraryHandlerFactory::instance().get(dictionary_id);
             if (!library_handler)
-                throw Exception(ErrorCodes::LOGICAL_ERROR, "Not found dictionary with id: {}", dictionary_id);
+                throw Exception(ErrorCodes::BAD_REQUEST_PARAMETER, "Not found dictionary with id: {}", dictionary_id);
 
             const auto & sample_block = library_handler->getSampleBlock();
             LOG_DEBUG(log, "Calling loadAll() for dictionary id: {}", dictionary_id);
@@ -226,7 +232,7 @@ void LibraryRequestHandler::handleRequest(HTTPServerRequest & request, HTTPServe
 
             auto library_handler = SharedLibraryHandlerFactory::instance().get(dictionary_id);
             if (!library_handler)
-                throw Exception(ErrorCodes::LOGICAL_ERROR, "Not found dictionary with id: {}", dictionary_id);
+                throw Exception(ErrorCodes::BAD_REQUEST_PARAMETER, "Not found dictionary with id: {}", dictionary_id);
 
             const auto & sample_block = library_handler->getSampleBlock();
             LOG_DEBUG(log, "Calling loadIds() for dictionary id: {}", dictionary_id);
@@ -265,7 +271,7 @@ void LibraryRequestHandler::handleRequest(HTTPServerRequest & request, HTTPServe
 
             auto library_handler = SharedLibraryHandlerFactory::instance().get(dictionary_id);
             if (!library_handler)
-                throw Exception(ErrorCodes::LOGICAL_ERROR, "Not found dictionary with id: {}", dictionary_id);
+                throw Exception(ErrorCodes::BAD_REQUEST_PARAMETER, "Not found dictionary with id: {}", dictionary_id);
 
             const auto & sample_block = library_handler->getSampleBlock();
             LOG_DEBUG(log, "Calling loadKeys() for dictionary id: {}", dictionary_id);
@@ -304,7 +310,7 @@ void LibraryRequestHandler::handleRequest(HTTPServerRequest & request, HTTPServe
 }
 
 
-void PingHandler::handleRequest(HTTPServerRequest & request, HTTPServerResponse & response)
+void LibraryExistsHandler::handleRequest(HTTPServerRequest & request, HTTPServerResponse & response)
 {
     try
     {
@@ -313,7 +319,7 @@ void PingHandler::handleRequest(HTTPServerRequest & request, HTTPServerResponse
 
         if (!params.has("dictionary_id"))
         {
-            processError(response, "No 'dictionary_id in request URL");
+            processError(response, "No 'dictionary_id' in request URL");
             return;
         }
 
@@ -321,9 +327,9 @@ void PingHandler::handleRequest(HTTPServerRequest & request, HTTPServerResponse
         auto library_handler = SharedLibraryHandlerFactory::instance().get(dictionary_id);
         String res;
         if (library_handler)
-            res = "dictionary=1";
+            res = "1";
         else
-            res = "dictionary=0";
+            res = "0";
 
         setResponseDefaultHeaders(response, keep_alive_timeout);
         LOG_TRACE(log, "Senging ping response: {} (dictionary id: {})", res, dictionary_id);

programs/library-bridge/Handlers.h

@@ -39,10 +39,10 @@ private:
 };
 
 
-class PingHandler : public HTTPRequestHandler, WithContext
+class LibraryExistsHandler : public HTTPRequestHandler, WithContext
 {
 public:
-    explicit PingHandler(size_t keep_alive_timeout_, ContextPtr context_)
+    explicit LibraryExistsHandler(size_t keep_alive_timeout_, ContextPtr context_)
         : WithContext(context_)
         , keep_alive_timeout(keep_alive_timeout_)
         , log(&Poco::Logger::get("LibraryRequestHandler"))

programs/library-bridge/SharedLibraryHandlerFactory.cpp

@@ -4,12 +4,6 @@
 namespace DB
 {
 
-namespace ErrorCodes
-{
-    extern const int BAD_ARGUMENTS;
-    extern const int LOGICAL_ERROR;
-}
-
 SharedLibraryHandlerPtr SharedLibraryHandlerFactory::get(const std::string & dictionary_id)
 {
     std::lock_guard lock(mutex);

src/Bridge/IBridgeHelper.cpp

@@ -33,9 +33,9 @@ Poco::URI IBridgeHelper::getPingURI() const
 }
 
 
-void IBridgeHelper::startBridgeSync() const
+void IBridgeHelper::startBridgeSync()
 {
-    if (!checkBridgeIsRunning())
+    if (!bridgeHandShake())
     {
         LOG_TRACE(getLog(), "{} is not running, will try to start it", serviceAlias());
         startBridge(startBridgeCommand());
@@ -49,7 +49,7 @@ void IBridgeHelper::startBridgeSync() const
             ++counter;
             LOG_TRACE(getLog(), "Checking {} is running, try {}", serviceAlias(), counter);
 
-            if (checkBridgeIsRunning())
+            if (bridgeHandShake())
             {
                 started = true;
                 break;
@@ -66,7 +66,7 @@ void IBridgeHelper::startBridgeSync() const
 }
 
 
-std::unique_ptr<ShellCommand> IBridgeHelper::startBridgeCommand() const
+std::unique_ptr<ShellCommand> IBridgeHelper::startBridgeCommand()
 {
     if (startBridgeManually())
         throw Exception(serviceAlias() + " is not running. Please, start it manually", ErrorCodes::EXTERNAL_SERVER_IS_NOT_RESPONDING);

src/Bridge/IBridgeHelper.h

@@ -35,10 +35,11 @@ public:
 
     Poco::URI getPingURI() const;
 
-    void startBridgeSync() const;
+    void startBridgeSync();
 
 protected:
-    virtual bool checkBridgeIsRunning() const = 0;
+    /// Check bridge is running. Can also check something else in the mean time.
+    virtual bool bridgeHandShake() = 0;
 
     /// clickhouse-odbc-bridge, clickhouse-library-bridge
     virtual String serviceAlias() const = 0;
@@ -63,7 +64,7 @@ protected:
 
 
 private:
-    std::unique_ptr<ShellCommand> startBridgeCommand() const;
+    std::unique_ptr<ShellCommand> startBridgeCommand();
 };
 
 }

src/Bridge/LibraryBridgeHelper.cpp

@@ -70,7 +70,7 @@ void LibraryBridgeHelper::startBridge(std::unique_ptr<ShellCommand> cmd) const
 }
 
 
-bool LibraryBridgeHelper::checkBridgeIsRunning() const
+bool LibraryBridgeHelper::bridgeHandShake()
 {
     String result;
     try
@@ -89,13 +89,11 @@ bool LibraryBridgeHelper::checkBridgeIsRunning() const
      * 1. It is dictionary source creation and initialization of library handler on bridge side did not happen yet.
      * 2. Bridge crashed or restarted for some reason while server did not.
     **/
-    static constexpr auto dictionary_check = "dictionary=";
+    if (result.size() != 1)
-    if (result.size() != (std::strlen(dictionary_check) + 1))
+        throw Exception(ErrorCodes::LOGICAL_ERROR, "Unexpected message from library bridge: {}. Check bridge and server have the same version.", result);
-        throw Exception(ErrorCodes::LOGICAL_ERROR, "Unexpected message from library bridge: {}. Check bridge and server have the same version.",
-                        result, std::strlen(dictionary_check));
 
     UInt8 dictionary_id_exists;
-    auto parsed = tryParse<UInt8>(dictionary_id_exists, result.substr(std::strlen(dictionary_check)));
+    auto parsed = tryParse<UInt8>(dictionary_id_exists, result);
     if (!parsed || (dictionary_id_exists != 0 && dictionary_id_exists != 1))
         throw Exception(ErrorCodes::LOGICAL_ERROR, "Unexpected message from library bridge: {} ({}). Check bridge and server have the same version.",
                         result, parsed ? toString(dictionary_id_exists) : "failed to parse");
@@ -106,13 +104,16 @@ bool LibraryBridgeHelper::checkBridgeIsRunning() const
     if (dictionary_id_exists && !library_initialized)
         throw Exception(ErrorCodes::LOGICAL_ERROR, "Library was not initialized, but bridge responded to already have dictionary id: {}", dictionary_id);
 
+    /// Here we want to say bridge to recreate a new library handler for current dictionary,
+    /// because it responded to have lost it, but we know that it has already been created. (It is a direct result of bridge crash).
     if (!dictionary_id_exists && library_initialized)
     {
         LOG_WARNING(log, "Library bridge does not have library handler with dictionaty id: {}. It will be reinitialized.", dictionary_id);
         bool reinitialized = false;
         try
         {
-            reinitialized = initLibrary(false);
+            auto uri = createRequestURI(LIB_NEW_METHOD);
+            reinitialized = executeRequest(uri, getInitLibraryCallback());
         }
         catch (...)
         {
@@ -148,13 +149,12 @@ ReadWriteBufferFromHTTP::OutStreamCallback LibraryBridgeHelper::getInitLibraryCa
 }
 
 
-bool LibraryBridgeHelper::initLibrary(bool check_bridge) const
+bool LibraryBridgeHelper::initLibrary()
 {
-    /// Do not check if we call initLibrary from checkBridgeSync.
+    startBridgeSync();
-    if (check_bridge)
-        startBridgeSync();
     auto uri = createRequestURI(LIB_NEW_METHOD);
-    return executeRequest(uri, getInitLibraryCallback());
+    library_initialized = executeRequest(uri, getInitLibraryCallback());
+    return library_initialized;
 }
 
 
@@ -163,7 +163,10 @@ bool LibraryBridgeHelper::cloneLibrary(const Field & other_dictionary_id)
     startBridgeSync();
     auto uri = createRequestURI(LIB_CLONE_METHOD);
     uri.addQueryParameter("from_dictionary_id", toString(other_dictionary_id));
-    return executeRequest(uri, getInitLibraryCallback());
+    /// We also pass initialization settings in order to create a library handler
+    /// in case from_dictionary_id does not exist in bridge side (possible in case of bridge crash).
+    library_initialized = executeRequest(uri, getInitLibraryCallback());
+    return library_initialized;
 }
 
 
@@ -171,7 +174,7 @@ bool LibraryBridgeHelper::removeLibrary()
 {
     /// Do not force bridge restart if it is not running in case of removeLibrary
     /// because in this case after restart it will not have this dictionaty id in memory anyway.
-    if (checkBridgeIsRunning())
+    if (bridgeHandShake())
     {
         auto uri = createRequestURI(LIB_DELETE_METHOD);
         return executeRequest(uri);

src/Bridge/LibraryBridgeHelper.h

@@ -26,7 +26,7 @@ public:
 
     LibraryBridgeHelper(ContextPtr context_, const Block & sample_block, const Field & dictionary_id_, const LibraryInitData & library_data_);
 
-    bool initLibrary(bool check_bridge = true) const;
+    bool initLibrary();
 
     bool cloneLibrary(const Field & other_dictionary_id);
 
@@ -48,10 +48,8 @@ public:
 
     LibraryInitData getLibraryData() const { return library_data; }
 
-    void setInitialized() { library_initialized = true; }
-
 protected:
-    bool checkBridgeIsRunning() const override;
+    bool bridgeHandShake() override;
 
     void startBridge(std::unique_ptr<ShellCommand> cmd) const override;
 

src/Bridge/XDBCBridgeHelper.h

@@ -74,7 +74,7 @@ public:
     }
 
 protected:
-    bool checkBridgeIsRunning() const override
+    bool bridgeHandShake() override
     {
         try
         {

src/Dictionaries/LibraryDictionarySource.cpp

@@ -41,6 +41,9 @@ LibraryDictionarySource::LibraryDictionarySource(
     , sample_block{sample_block_}
     , context(Context::createCopy(context_))
 {
+    if (fs::path(path).is_relative())
+        path = fs::canonical(path);
+
     if (created_from_ddl && !pathStartsWith(path, context->getDictionariesLibPath()))
         throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "File path {} is not inside {}", path, context->getDictionariesLibPath());
 
@@ -58,10 +61,7 @@ LibraryDictionarySource::LibraryDictionarySource(
 
     bridge_helper = std::make_shared<LibraryBridgeHelper>(context, description.sample_block, dictionary_id, library_data);
 
-    bool initialized = bridge_helper->initLibrary();
+    if (!bridge_helper->initLibrary())
-    if (initialized)
-        bridge_helper->setInitialized();
-    else
         throw Exception(ErrorCodes::EXTERNAL_LIBRARY_ERROR, "Failed to create shared library from path: {}", path);
 }
 
@@ -91,10 +91,7 @@ LibraryDictionarySource::LibraryDictionarySource(const LibraryDictionarySource &
     , description{other.description}
 {
     bridge_helper = std::make_shared<LibraryBridgeHelper>(context, description.sample_block, dictionary_id, other.bridge_helper->getLibraryData());
-    bool cloned = bridge_helper->cloneLibrary(other.dictionary_id);
+    if (!bridge_helper->cloneLibrary(other.dictionary_id))
-    if (cloned)
-        bridge_helper->setInitialized();
-    else
         throw Exception(ErrorCodes::EXTERNAL_LIBRARY_ERROR, "Failed to clone library");
 }
 

src/Dictionaries/LibraryDictionarySource.h

@@ -80,7 +80,7 @@ private:
 
     const DictionaryStructure dict_struct;
     const std::string config_prefix;
-    const std::string path;
+    std::string path;
     const Field dictionary_id;
 
     Block sample_block;

tests/integration/test_library_bridge/configs/config.d/config.xml

@@ -8,5 +8,9 @@
         <count>10</count>
         <stderr>/var/log/clickhouse-server/stderr.log</stderr>
         <stdout>/var/log/clickhouse-server/stdout.log</stdout>
+
+        <library_bridge_log>/var/log/clickhouse-server/clickhouse-library-bridge.log</library_bridge_log>
+        <library_bridge_errlog>/var/log/clickhouse-server/clickhouse-library-bridge.err.log</library_bridge_errlog>
+        <library_bridge_level>trace</library_bridge_level>
     </logger>
 </yandex>