Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-28860

Refactoring the configuration service of the Qserv Replication/Ingest system

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: Qserv
    • Labels:
      None

      Description

      Motivations

      The present design of the Configuration service is quite complex. There are 5 classes (and a few hundred methods) in the core of the service:

                        ==================
        +-------------> ConfigurationIFace
        |               ==================
        |                    ^    ^
        |                    |    |
        | has     +----------+    +----------+
        |         |                          |
        |   =============            =================
        +-- Configuration            ConfigurationBase
            =============            =================
             : public API                ^       ^
                                         |       |
                                  +------+       +-------+
                                  |                      |
                          ================       ==================
                          ConfigurationMap       ConfigurationMySQL
                          ================       ==================
                           : unit testing            : used at NCSA
                                                     : and Kubernetes
      

      The second issue is a distributed state of the transient configuration that's spread in between static data members of class Configuration and member data of class ConfigurationBase.

      The third issue is related to how the default values of the parameters are stored and used. In the present implementation there are ~50 static members of class ConfigurationBase representing the defaults. The defaults are separate from the target variables (where the current state of the corresponding configuration parameters are stored).

      All together, this makes it quite cumbersome to add new configuration parameters, or modify existing ones.

      There is also a new requirement in the Replication/Ingest system that would be hard to address within the present paradigm of the service - serializing the state of the configuration and sending it over-the-wire protocol from the Master Controller to the Replication/Ingest workers.

      The proposed migration plan

      The proposal is modeled after a similar effort made in a context of DM-26754.
      To address the above stated issues, the following refactoring is proposed:

      • Flatten the hierarchy of the existing classes into a single class Configuration.
      • Keep the named methods as they are now to avoid modifying the dependent code.
      • Implement the transient state as a single object of class nlohmann::json::object.
      • Set defaults as initial value of the transient object. The defaults will be amended when the corresponding "external" source of the configuration will be used to set up the new state.
      • Implements a few forms of the configuration construction from the following sources:
        • MySQL (will require a connection string mysql://<user>:<password>@<host>:<port>/<database>).
        • Transient JSON object.
        • A JSON-formatted text file.

      This approach will benefit from the existing features of the nlohmann::json library, such as: serialization/de-serialization into/from a string, json path capability.

      Besides, the new implementation of the service won't affect existing code relying upon the configuration. Existing unit tests should also work. Though, they will need to be migrated from relying upon the "key-values" maps for setting the tested object state to use a transient JSON object.

        Attachments

          Issue Links

            Activity

            Hide
            salnikov Andy Salnikov added a comment -

            I have reviewed the PR but I did not approve it, so I 'm not marking this ticket as reviewed yet. We should probably talk about what needs to be done for schema migration and few other issues that I commeneted on.

            Show
            salnikov Andy Salnikov added a comment - I have reviewed the PR but I did not approve it, so I 'm not marking this ticket as reviewed yet. We should probably talk about what needs to be done for schema migration and few other issues that I commeneted on.
            Hide
            gapon Igor Gaponenko added a comment - - edited

            Andy Salnikov Please, have another look at the PR. I've reviewed all suggestions, answered your questions on the design decisions, and implemented most of what was requested. Specifically, the following major changes were made to the code:

            • The built-in schema management was killed in anticipation that an appropriate schema migration mechanism will be implemented in the new Qserv containers. When it'll be ready, the code of the module will quickly adopt the mechanism as soon as we will switch to using the new containers and the build system. In the meantime, two SQL files were reintroduced into the package: the schema definition file and the initialization file for the default values of the general parameters. I should say I don't like the second file since what it does duplicates what's done in the JSON schema definition of class ConfigurationSchema. However, with no access to the schema from the transient code, this duplication is unavoidable. With my original attempt to put all schema operations into the code, I was hoping to avoid this and other related problems. I still think the built-in "smart" schema management approach is better than what's presently done in Qserv with the "external" schema management.
            • The AKA "integration" test (better be called the "diagnostic" test for the MySQL backend) moved into a separate application qserv-replica-config-test.
            • The use of JSON has been reduced to the general parameters only. Complex (object) definitions of workers, database families, and databases are now stored as traditional statically types structures/objects within class Configuration.
            • Parsing of parameters from JSON or MySQL sources has been made scalable (nearly everything is driven by the transient JSON schema in class ConfigurationSchema). After that new general parameters would only need to be added to the schema definition (and, unfortunately, since I killed the built-in MySQL schema support, within an external SQL file) and into the unit tests.
            • The class Configuration has been reduced by moving classes WorkerInfo, DatabaseFamilyInfo, and DatabaseInfo into dedicated files.
            • The parser classes moved from file Configuration.cc into dedicated files: ConfigParserJSON and ConfigParserSQL. They were also refactored to avoid explicitly mentioning the names of the general parameters. Parsing of those is now completely driven by the transient schema definition.

            Typos and spellings were corrected. I'm not sure why my VSCode setup didn't see them, even though I have the spelling checker installed. It sees some, but not others.

            For sure more improvements could be made to the code to make it "perfect". However (and honestly) this won't worth additional efforts. What I have here covers all (but the one I killed) my needs.

            UPDATED: I've realized that with the JSON-based transient schema (that also provides default values) there is no longer any need in preloading the defaults from an external SQL file. So, I'm removing the one from the module:

            core/modules/replica/schema/replication_default_config.sql
            

            Should any site/deployment-specific customization be needed, those would/should be done using a specialized configuration, that would presumably be in a separate Git package.

            Thank you!

            Show
            gapon Igor Gaponenko added a comment - - edited Andy Salnikov Please, have another look at the PR. I've reviewed all suggestions, answered your questions on the design decisions, and implemented most of what was requested. Specifically, the following major changes were made to the code: The built-in schema management was killed in anticipation that an appropriate schema migration mechanism will be implemented in the new Qserv containers. When it'll be ready, the code of the module will quickly adopt the mechanism as soon as we will switch to using the new containers and the build system. In the meantime, two SQL files were reintroduced into the package: the schema definition file and the initialization file for the default values of the general parameters. I should say I don't like the second file since what it does duplicates what's done in the JSON schema definition of class ConfigurationSchema . However, with no access to the schema from the transient code, this duplication is unavoidable. With my original attempt to put all schema operations into the code, I was hoping to avoid this and other related problems. I still think the built-in "smart" schema management approach is better than what's presently done in Qserv with the "external" schema management. The AKA "integration" test (better be called the "diagnostic" test for the MySQL backend) moved into a separate application qserv-replica-config-test . The use of JSON has been reduced to the general parameters only. Complex (object) definitions of workers, database families, and databases are now stored as traditional statically types structures/objects within class Configuration . Parsing of parameters from JSON or MySQL sources has been made scalable (nearly everything is driven by the transient JSON schema in class ConfigurationSchema ). After that new general parameters would only need to be added to the schema definition (and, unfortunately, since I killed the built-in MySQL schema support, within an external SQL file) and into the unit tests. The class Configuration has been reduced by moving classes WorkerInfo , DatabaseFamilyInfo , and DatabaseInfo into dedicated files. The parser classes moved from file Configuration.cc into dedicated files: ConfigParserJSON and ConfigParserSQL . They were also refactored to avoid explicitly mentioning the names of the general parameters. Parsing of those is now completely driven by the transient schema definition. Typos and spellings were corrected. I'm not sure why my VSCode setup didn't see them, even though I have the spelling checker installed. It sees some, but not others. For sure more improvements could be made to the code to make it "perfect". However (and honestly) this won't worth additional efforts. What I have here covers all (but the one I killed) my needs. UPDATED : I've realized that with the JSON-based transient schema (that also provides default values) there is no longer any need in preloading the defaults from an external SQL file. So, I'm removing the one from the module: core/modules/replica/schema/replication_default_config.sql Should any site/deployment-specific customization be needed, those would/should be done using a specialized configuration, that would presumably be in a separate Git package. Thank you!
            Hide
            salnikov Andy Salnikov added a comment -

            Looks OK, few comments on PR.

            Show
            salnikov Andy Salnikov added a comment - Looks OK, few comments on PR.

              People

              Assignee:
              gapon Igor Gaponenko
              Reporter:
              gapon Igor Gaponenko
              Reviewers:
              Andy Salnikov
              Watchers:
              Andy Salnikov, Fritz Mueller, Igor Gaponenko, Nate Pease
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.