-
Notifications
You must be signed in to change notification settings - Fork 205
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
Disable configuration management until the ledger has a configuration. [KVL-1058] #10478
Disable configuration management until the ledger has a configuration. [KVL-1058] #10478
Conversation
TelemetrySpecBase doesn't need to expose those through inheritance.
.flatMap { | ||
case Some((_, configuration)) => | ||
Future.successful(configurationToResponse(configuration)) | ||
case None => | ||
logger.warn( | ||
"Could not get the current time model. The index does not yet have the ledger configuration." | ||
) | ||
Future.failed(ErrorFactories.missingLedgerConfigOnConfigRequest()) | ||
} |
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.
do we need the flatmap? It seems we're just wrapping withing a future inside.
I would personally actually prefer the fold for readability but it's up to you.
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 it to use Future.failed
here. Can't do that with a .map(_.fold(...))
.
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.
Why not use a throw?
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.
Because I expect .map
to take a successful future and return a successful future, or take a failed future and return a failed future. I appreciate that Future
will catch exceptions within its chained method calls and convert them to failed futures, but IMO this is specifically a backup plan and I don't like to rely on it.
Long story short, I don't like mixing my Scala monads and throw
. I'd rather live in the Scala universe or the Java one, but not both within the same method.
We may disagree here. Happy to debate further.
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 do disagree but I don't have any hard opinions about this so feel free to leave it as is.
As for my reasoning:
I would expect that we would use flatMap
when there's an actual Future context, as this would also make me do a double take when reading the code to make sure I'm aware of what complications
we're merging, and in this case it's an artificial usage.
I do agree that throw is not the best but imo we should return a Future[Either[L, R]]
so that we don't mix exceptions with error handling (and fold the future before returning it), or do accept that scala future just ain't pretty.
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.
Yeah, I think you're right. Bigger change, but perhaps one we need to consider in the long run. I find it really tough to figure out all the places these API service implementations can fail.
...ration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala
Show resolved
Hide resolved
...ration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala
Outdated
Show resolved
Hide resolved
val expectedGeneration = 3L | ||
val expectedConfiguration = Configuration( | ||
generation = expectedGeneration, | ||
timeModel = LedgerTimeModel( |
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.
For readability, you might want to move the two time models required for these tests into a helper variable (e.g., aTimeModel
and aNewerTimeModel
).
with SuiteResourceManagementAroundEach { | ||
|
||
"CommandCompletionService can stream completions from the beginning" in { | ||
val lid = ledgerId() |
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.
'Lid' is a valid English word so I'd suggest to disambiguate to avoid confusion:
val lid = ledgerId() | |
val aLedgerId = ledgerId() |
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 just moved this test, but sure, happy to fix it.
...ration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala
Show resolved
Hide resolved
...ration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala
Outdated
Show resolved
Hide resolved
...ration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala
Outdated
Show resolved
Hide resolved
...i-common/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala
Outdated
Show resolved
Hide resolved
ab93d14
to
4fb79da
Compare
From the changelog:
Technically this should be "... the participant will also not respond to command submissions." |
CHANGELOG_BEGIN - [API Server] The configuration management service previously returned the participant-specified default configuration if none was found on the ledger. This can be misleading, and so the ``GetTimeModel`` endpoint now returns a gRPC ``NOT_FOUND`` error if no configuration has been found yet. Similarly, the ``SetTimeModel`` endpoint returns a gRPC ``UNAVAILABLE`` error. This should only happen if the participant gives up waiting for the ledger to provide a configuration, which is very unlikely in a production setting. In this case, the ledger will also not respond to command submissions. CHANGELOG_END
In ApiConfigManagementServiceSpec.
Co-authored-by: Robert Autenrieth <31539813+rautenrieth-da@users.noreply.github.com>
Co-authored-by: fabiotudone-da <fabio.tudone@digitalasset.com>
4fb79da
to
21acfb5
Compare
This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @nickchapman-da is in charge of this release. Commit log: ``` 56f3f6c factor the complicated bits of contract selection between PG, Oracle (#10528) 3b1d1ac IndexerBenchmark: an option for defining minimum update rate [DPP-541] (#10540) 511f27c H2 Storage backend support for canton jdbc urls with user/password (#10523) 5cc78f5 Handle missing template instances in Daml ledger export (#10526) 5e7f8e6 Handle not quite empty directory digests in cache cleanup (#10536) 34fa5a0 Support running daml navigator without a config file (#10532) 1affbd2 feature: persist script view config in worspace (#10521) 7529c4a Issue: 10383 - Support deadline for command submission (#10522) 881d0ab Upgrade rules_haskell and rules_nixpkgs (#10517) 35f1592 Split the Daml ledger export integration tests (#10506) ebab670 Disable pushing Bazel metrics on PRs from forks (#10520) b00e146 [JSON-API/trigger-service] Refactor db conn (#10497) 23168a8 KV / integrity checker: conflate `Disputed`, `InvalidLedgerTime` and `Inconsistent` rejection reasons [KVL-1059] (#10515) 55ed83c Factor out Daml export integration tests (#10492) 683cccc timer-utils: If `RetryStrategy` eventually fails, explain what happened. (#10511) 2d25797 update compat versions for 1.16.0-snapshot.20210805.7501.0.48050ad7 (#10505) 771f918 update NOTICES file (#10514) 4406e98 Create a proper state enum in LedgerConfigurationSubscriptionFromIndex. [KVL-1046] (#10508) fe1c678 sandbox: Attempt to make the reset service tests less flaky. (#10507) 07da936 participant-integration-api: Always wait for the first config lookup. (#10500) 313110c correct JSON API upper date bound (#10489) a96f3d2 Release another 1.16.x snapshot (#10502) 9da6b04 participant-integration-api: Make the initial ledger config optional. [KVL-1046] (#10498) a7fa7d3 participant-integration-api: Rename and restructure the configuration initialization classes. [KVL-1046] (#10496) 35641b7 Add submission id to the GRPC api [KVL-999] (#10467) df78f9c Replace LedgerConfiguration with InitialLedgerConfiguration or the load timeout. [KVL-1058] (#10487) 8603cd3 [JSON-API] Move database independent tests into a seperate abstract test (#10482) 8f1cde3 Handle exerciseByKey after exercise command (#10490) a8a39f7 Handle tuple values in Daml ledger export (#10483) 73b6596 replace DA ledger with Daml ledger in the ledger model (#10491) 348c6de Add dlint rule to suggest === (#10485) 185f888 move e2e testing to app-dev (#10479) 6e0e46e update NOTICES file (#10488) 12599e7 Disable configuration management until the ledger has a configuration. [KVL-1058] (#10478) 36f3ba8 lsp-types patch for platform independence (#10288) 7c6d3c5 Handle package dependencies without metadata in Daml export (#10481) e5514bc feature: remember checkboxes in script view (#10469) 87c9c51 Use at least LF version 1.7 for Daml ledger export (#10480) 50c7b79 Indexer db MigrateOnEmptySchemaAndStart startup mode and migrateOnly hook (#10457) dc47b10 LF: Log internal errors (#10471) 7bcec46 update NOTICES file (#10476) a60bfb5 json-api: avoid row_number/partition for single-party Oracle contract queries (#10124) c243f82 LF: Clean up of Errors (#10052) 5d5343a LF: remove PartialTransaction argument from Speedy err pretty printer (#10470) 45ed615 participant-integration-api: Use `Scheduler`, and add more tests to configuration initialization. [KVL-1046] (#10461) 55392a5 [Docs] Update to a newer gpg keyserver because the old one is deprecated / not reachable anymore (#10452) 8c55e01 update compat versions for 1.16.0-snapshot.20210802.7499.0.5157ad6d (#10464) 5802f96 [JSON-API] Add option for setting a table prefix (#10444) f1c7548 remove unstable marker for append-only features (#10465) 24d3899 Fix deprecated buf command-line parameterization [KVL-980] (#10447) b4b4a94 Release a new 1.16.0 snapshot (#10460) 8df17d2 update NOTICES file (#10463) a074bb1 Append-only schema h2 database fix (#10462) 1971274 Reactive canton conformance test aginst LF 1.13 (#10458) db7728a participant-integration-api: Split LedgerConfigProvider into two, and add test cases. [KVL-1046] (#10455) ``` Changelog: ``` - [Integration Kit] - indexer-benchmark - added --min-update-rate option for defining required update rate - [Daml export] Daml ledger export now handles templates in packages using LF versions 1.7 or older. These package versions don't include type class instances and Daml ledger export needs to generate replacement instances in the generated script. The generated script uses less type-safe versions of Daml script ledger commands. - [Navigator] Navigator will now start with an empty config if no config file exists and it is run outside of a project. [Daml studio] The script view configuration is remembered for each script in a workspace and does not need to be reconfigured upon closing/reopening or restarting of Daml studio. - [Integration Kit] The initial ledger configuration is now optional; if it is not specified, the participant will not attempt to submit a configuration, but instead wait for something else to provision the ledger with a configuration. This behavior is intended to be used by ledger drivers that have alternative means of setting up the initial ledger configuration. If no configuration is provisioned, the participant server will not be able to submit commands to the ledger. Add optional submission_id to the commands.proto. - [Integration Kit] The ``LedgerConfiguration`` class has been removed in favor of ``InitialLedgerConfiguration``. Its usage has been changed accordingly, with the ``configurationLoadTimeout`` property becoming part of ``ApiServerConfig`` instead. The default options provided by ``LedgerConfiguration`` have been removed; you are now encouraged to come up with sensible values for your own ledger. The ``Configuration.reasonableInitialConfiguration`` value may help. - [API Server] The configuration management service previously returned the participant-specified default configuration if none was found on the ledger. This can be misleading, and so the ``GetTimeModel`` endpoint now returns a gRPC ``NOT_FOUND`` error if no configuration has been found yet. Similarly, the ``SetTimeModel`` endpoint returns a gRPC ``UNAVAILABLE`` error. This should only happen if the participant gives up waiting for the ledger to provide a configuration, which is very unlikely in a production setting. In this case, the ledger will also not respond to command submissions. [daml studio] The script view now remembers checkboxes for detailed disclosure view and archived contracts across script changes. Indexer: The indexer now supports the MigrateOnEmptySchemaAndStart startup mode that only performs migrations on brand-new databases. In addition "real upgrades" are now supported via a new "StandaloneIndexerServer.migrateOnly" entry point. - [Docs] The manual install instructions now use a different gpg keyserver because the old one is deprecated / not reachable anymore - [JSON-API] A table prefix can now be specified in the jdbc config via `tablePrefix=<YourFancyTablePrefix>`. This was added to allow running multiple instances of the json api without having collisions (independent of the chosen database). ``` CHANGELOG_BEGIN CHANGELOG_END
This also adds unit tests for
ApiConfigManagementService
. I had to remove a Sandbox integration test case because the behavior changed, but I think the new behavior is now adequately covered by the new tests.Changelog
[API Server] The configuration management service previously returned the participant-specified default configuration if none was found on the ledger. This can be misleading, and so the
GetTimeModel
endpoint now returns a gRPCNOT_FOUND
error if no configuration has been found yet. Similarly, theSetTimeModel
endpoint returns a gRPCUNAVAILABLE
error.This should only happen if the participant gives up waiting for the ledger to provide a configuration, which is very unlikely in a production setting. In this case, the ledger will also not respond to command submissions.
Pull Request Checklist
CHANGELOG_BEGIN
andCHANGELOG_END
tagsNOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines run
totrigger the build.