Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise the config to use a hierarchical structure [DPP-1029] #13629

Merged
merged 46 commits into from
May 30, 2022

Conversation

skisel-da
Copy link
Contributor

@skisel-da skisel-da commented Apr 19, 2022

This PR revises the configuration of Sandbox-on-X and underlying infrastructure:

  • Remove of unused configuration parameters.
  • Create smaller config objects with cohesive parametrisation.
  • Removal of redundant transformation layer.
  • Preparation for the more generic configuration objects which are not cli-driven.
  • Preparation for the future HOCON-driven configuration.

CHANGELOG_BEGIN
CHANGELOG_END

@garyverhaegen-da
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@skisel-da skisel-da force-pushed the skisel-da/cli-config-layer branch 3 times, most recently from 88cb641 to 7f113d2 Compare April 20, 2022 15:44
@skisel-da skisel-da changed the title Layer between config and cli-driven config HOCON configuration Apr 21, 2022
@rgugliel-da
Copy link
Contributor

Thanks for the pointer on this PR :) Would you mind giving a small summary of what you are changing and how it will propagate on Canton side a few days before merging? :)
@phoebe-da FYI

@skisel-da
Copy link
Contributor Author

sure, this is currently in the PoC stage (with reusable parts which we will decide to take). I will definitely involve you at the time it will be ready for breaking changes.

@skisel-da skisel-da force-pushed the skisel-da/cli-config-layer branch 3 times, most recently from 5802c13 to d3598ed Compare April 28, 2022 10:10
@skisel-da skisel-da force-pushed the skisel-da/cli-config-layer branch from a2710c4 to 03a39b9 Compare May 3, 2022 14:48
for lf_version in lf_versions_aggregate(lf_versions):
extra_server_args = ["--daml-lf-dev-mode-unsafe"] if lf_version == lf_version_configuration.get("preview") or lf_version == lf_version_configuration.get("dev") else []
daml_lf_dev_mode_args = ["-C ledger.engine.allowed-language-versions=daml-lf-dev-mode-unsafe"] if hocon else ["--daml-lf-dev-mode-unsafe"]
Copy link
Contributor

@simonmaxen-da simonmaxen-da May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that we are using a -C flag here for hocon. Isn't this deprecated ?

databaseConnectionPoolSize = participantConfig.apiServerDatabaseConnectionPoolSize,
databaseConnectionTimeout = FiniteDuration(
participantConfig.apiServerDatabaseConnectionTimeout.toMillis,
def extraConfigParser(parser: OptionParser[LegacyCliConfig[ExtraConfig]]): Unit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is returning Unit so clearly doing some 'nasty' mutation of the parser. A comment as to its intended use may help.


import scala.util.Try

object AuthServiceConfigCli {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LegacyAuthServiceConfigCli maybe ?

jdbcUrl: String,
connectionPool: ConnectionPoolConfig,
postgres: PostgresDataSourceConfig = PostgresDataSourceConfig(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to work on postgres and oracle so it looks strange to have a field called postgres.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This object is mapped by pureconfig library as is currently, so it produces the following config in hocon format:

        database {
          connection-pool {
            connection-timeout = "250ms"
            max-pool-size = 17
            minimum-idle = 17
          }
          jdbc-url = "jdbc:h2:mem:default;db_close_delay=-1;db_close_on_exit=false"
          postgres {
            synchronous-commit = off
            tcp-keepalives-count = 5
            tcp-keepalives-idle = 10
            tcp-keepalives-interval = 1
          }
        }

How about making it Option[PostgresDataSourceConfig] so it can be undefined?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it should be dataSourceconfig: DataSourceConfig with variants for Postgres/Oracle. I don't understand why we would need specific config for one DB but not another (in the general case).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to not use polymorphic objects within configuration as it adds complexity on parsing, deriving and understanding of the concrete type used to map for this path - this is why it is within postgres path - you know exactly what are the possible values and their types. In case of generic "data-source-config" path and interface used to map for this path - how would parser know which config type to use here? Usually type hint is used in this case, but not sure I would like something like:

database {
  jdbc-url = "jdbc:h2:mem:default;db_close_delay=-1;db_close_on_exit=false"
  datasource-config {
    type = postgres
    synchronous-commit = off
    tcp-keepalives-count = 5
    tcp-keepalives-idle = 10
    tcp-keepalives-interval = 1
  }
}

Let me investigate further if there is a better way for that. I might come with another proposal.

)

#Duplicated tests using new hocon-style configuration
conformance_test(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many of these conformance tests get run on each PR to main. How long does it add to the build ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added them to make sure HOCON configuration works just like old CLI. In fact did not plan them to be in parallel and they should have been replacing old cli implementation. Let's agree first on the sequence of work merged to main and deprecation policy, it would be then clear what to do with the tests.


import java.time.Duration

class BridgeConfigProvider extends ConfigProvider[BridgeConfig] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LegacyBridgeConfigProvider maybe ?

@skisel-da skisel-da force-pushed the skisel-da/cli-config-layer branch from 6c4b7fe to 0c08862 Compare May 30, 2022 07:11
@skisel-da skisel-da merged commit 168f943 into main May 30, 2022
@skisel-da skisel-da deleted the skisel-da/cli-config-layer branch May 30, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants