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

Disable configuration management until the ledger has a configuration. [KVL-1058] #10478

Merged
merged 9 commits into from
Aug 4, 2021

Conversation

ghost
Copy link

@ghost ghost commented Aug 4, 2021

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 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.

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

Comment on lines 56 to 64
.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())
}
Copy link
Contributor

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.

Copy link
Author

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(...)).

Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

val expectedGeneration = 3L
val expectedConfiguration = Configuration(
generation = expectedGeneration,
timeModel = LedgerTimeModel(
Copy link
Contributor

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()
Copy link
Contributor

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:

Suggested change
val lid = ledgerId()
val aLedgerId = ledgerId()

Copy link
Author

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.

@ghost ghost added the automerge label Aug 4, 2021
@ghost ghost force-pushed the samir/participant-integration-api/config-not-found branch from ab93d14 to 4fb79da Compare August 4, 2021 15:44
@rautenrieth-da
Copy link
Contributor

From the changelog:

In this case, the ledger will also not respond to transaction submissions.

Technically this should be "... the participant will also not respond to command submissions."

@ghost ghost removed the automerge label Aug 4, 2021
SamirTalwar and others added 5 commits August 4, 2021 20:42
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
Co-authored-by: Robert Autenrieth <31539813+rautenrieth-da@users.noreply.github.com>
Co-authored-by: fabiotudone-da <fabio.tudone@digitalasset.com>
@ghost ghost force-pushed the samir/participant-integration-api/config-not-found branch from 4fb79da to 21acfb5 Compare August 4, 2021 18:42
@ghost ghost added the automerge label Aug 4, 2021
@mergify mergify bot merged commit 12599e7 into main Aug 4, 2021
@mergify mergify bot deleted the samir/participant-integration-api/config-not-found branch August 4, 2021 20:34
azure-pipelines bot pushed a commit that referenced this pull request Aug 11, 2021
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
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.

6 participants