-
Notifications
You must be signed in to change notification settings - Fork 206
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
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
88cb641
to
7f113d2
Compare
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? :) |
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. |
5802c13
to
d3598ed
Compare
a2710c4
to
03a39b9
Compare
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"] |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
ledger/sandbox-on-x/src/app/scala/com/daml/ledger/sandbox/MainHocon.scala
Outdated
Show resolved
Hide resolved
ledger/sandbox-on-x/src/app/scala/com/daml/ledger/sandbox/MainHocon.scala
Outdated
Show resolved
Hide resolved
ledger/sandbox-on-x/src/main/scala/com/daml/ledger/sandbox/SandboxOnXRunner.scala
Outdated
Show resolved
Hide resolved
ledger/ledger-runner-common/src/main/scala/com/daml/ledger/runner/common/ConfigProvider.scala
Outdated
Show resolved
Hide resolved
ledger/ledger-runner-common/src/main/scala/com/daml/ledger/runner/common/ConfigRenderer.scala
Outdated
Show resolved
Hide resolved
ledger/ledger-runner-common/src/main/scala/com/daml/ledger/runner/common/MetricsConfig.scala
Show resolved
Hide resolved
ledger/participant-integration-api/src/main/scala/platform/apiserver/ApiServerConfig.scala
Show resolved
Hide resolved
ledger/participant-integration-api/src/main/scala/platform/apiserver/AuthServiceConfig.scala
Show resolved
Hide resolved
|
||
import scala.util.Try | ||
|
||
object AuthServiceConfigCli { |
There was a problem hiding this comment.
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(), |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
ledger/sandbox-on-x/BUILD.bazel
Outdated
) | ||
|
||
#Duplicated tests using new hocon-style configuration | ||
conformance_test( |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LegacyBridgeConfigProvider maybe ?
...er/ledger-runner-common/src/main/scala/com/daml/ledger/runner/common/ParticipantConfig.scala
Outdated
Show resolved
Hide resolved
...er/ledger-runner-common/src/main/scala/com/daml/ledger/runner/common/CliConfigProvider.scala
Outdated
Show resolved
Hide resolved
ledger/participant-integration-api/src/main/scala/platform/store/backend/StorageBackend.scala
Show resolved
Hide resolved
6c4b7fe
to
0c08862
Compare
This PR revises the configuration of Sandbox-on-X and underlying infrastructure:
CHANGELOG_BEGIN
CHANGELOG_END