participant-integration-api: Store a status gRPC protobuf. [KVL-1005] #10600

ghost · 2021-08-17T13:33:00Z

Instead of storing the status code and message, we store a serialized google.rpc.Status protocol buffers message. This allows us to pass through any additional information reported by the driver ReadService.

The migration is only done for the append-only index, and preserves old data in the existing columns. New data will only be written to the new column. We'll make sure this is OK by only supporting the append-only index with the v2 ReadService.

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.

It's only used here; there's no reason to keep it in the _participant-integration-api_.

...integration-api/src/main/resources/db/migration/oracle-appendonly/V1__Append_only_schema.sql

ghost · 2021-08-17T13:34:11Z

...er/participant-integration-api/src/main/scala/platform/store/CompletionFromTransaction.scala

-  // But for an api server that is part of a distributed ledger network, we might see
-  // transactions that originated from some other api server. These transactions don't contain the submitter information,
-  // and therefore we don't emit CommandAccepted completions for those
-  def apply(


This function was inlined into InMemoryLedger in Sandbox Classic, as it's only used there.

...t-integration-api/src/main/resources/db/migration/h2database/V31__rejection_status_proto.sql

...gration-api/src/main/resources/db/migration/h2database-appendonly/V1__Append_only_schema.sql

...integration-api/src/main/resources/db/migration/oracle-appendonly/V1__Append_only_schema.sql

ledger/participant-integration-api/src/main/resources/db/migration/oracle/V1__Init.sql

.../main/resources/db/migration/postgres-appendonly/V106__add_rejection_status_proto_column.sql

fabiotudone-da · 2021-08-17T13:45:10Z

The migration is only done for the append-only database

How about the "vintage" one?

nicu-da

Nice work!
Do we have any sort of integration tests that we can use to validate the behavior when reading completions that we stored both with the old status and message along with the new rejection_status format?

...integration-api/src/main/resources/db/migration/oracle-appendonly/V1__Append_only_schema.sql

.../main/resources/db/migration/postgres-appendonly/V106__add_rejection_status_proto_column.sql

.../participant-integration-api/src/main/scala/platform/store/dao/CommandCompletionsTable.scala

Instead of storing the status code and message, we store a serialized `google.rpc.Status` protocol buffers message. This allows us to pass through any additional information reported by the driver `ReadService`. The migration is only done for the append-only database, and preserves old data in the existing columns. New data will only be written to the new column. CHANGELOG_BEGIN CHANGELOG_END

Co-authored-by: Fabio Tudone <fabio.tudone@digitalasset.com>

nmarton-da

nice'n'easy 👍 thanks @SamirTalwar (commented only on minor stuff)

...gration-api/src/main/resources/db/migration/h2database-appendonly/V1__Append_only_schema.sql

...integration-api/src/main/resources/db/migration/oracle-appendonly/V1__Append_only_schema.sql

nmarton-da · 2021-08-17T19:03:22Z

...tion-api/src/main/scala/platform/store/backend/common/CompletionStorageBackendTemplate.scala

+    val parserWithCodeAndMessage = sharedColumns ~
+      int("rejection_status_code") ~ str("rejection_status_message") map {
+        case offset ~ recordTime ~ commandId ~ rejectionStatusCode ~ rejectionStatusMessage =>
+          val status = StatusProto.of(rejectionStatusCode, rejectionStatusMessage, Seq.empty)
+          CompletionFromTransaction.rejectedCompletion(recordTime, offset, commandId, status)
+      }


do you think we have tests somewhere to test this part?
normally it would not be tested because on the HEAD we would always populate the rejection_status

@rautenrieth-da did you maybe encountered a test suit which would test this?

We are very bad at testing migrations. I have no idea how we would go about testing this.

The compatibility package tests migrations in a way. It does:

install an old SDK version

push some data into a sandbox ledger, using the ledger API

upgrade to the next SDK version (up to and including the head version)

use the ledger API to check that the ledger content still looks the same

go to step 2

The application that uploads and checks data is a custom client application written in Haskell. There are checks for comparing the ACS and contract visibility due to divulgence. It could probably be extended to compare completions as well.

Yeah but do we also test rejections this way? Never mind me, it will probably work...although we would not necessarily would have here dead code if we follow @miklos-da useful suggestion

I don't think we test rejections, but we could extend the compatibility tests to cover them.

nicu-da · 2021-08-18T07:22:52Z

@SamirTalwar-DA Thinking more about this, I'm not really convinced that we should store the raw google.rpc.Status directly. This creates a direct coupling with the external model which is not under our control at all, and in the long future if there's a breaking change this is gonna really complicate our life.
It's also a really weird coupling between a completion rejection and gRPC response, I don't think we should have those two so tighly bound.
Wouldn't it make more sense to create our own protobuf model to represent completion failure reason?

ghost · 2021-08-18T07:41:38Z

@nicu-da: This is a reasonable concern. Fortunately, google.rpc.Status is a protocol buffers message, which means that (a) it should never change a backwards-incompatible way, and (b) if it does change in a way that we don't want to support, we can just copy it, rename it to daml.Status, and carry on using it, because the message type is not embedded in the message.

The reason we need to store a protobuf (as opposed to just having columns) is because with the new API, the ledger is capable of passing back arbitrary information as part of the details, which then needs to be sent to the ledger API client. The indexer and API server may not necessarily understand this information but they still need to send it on.

fabiotudone-da · 2021-08-18T08:08:51Z

@SamirTalwar-DA could you elaborate on the reason why we only need to do this for the append-only schema?

ghost · 2021-08-18T08:16:25Z

@fabiotudone-da: We have decided that we will only support the v2 participant state API with the append-only index. This means that there will never be any "details" in the rejection_status when using the mutable index, and so we will always be able to fully reconstruct the status from its code and message.

We decided this because adding a new migration to the mutable index causes a whole load of problems right now. This is due to the way migrations are ordered within our codebase: all mutable index migrations must be applied before migrating to the append-only index. If we add a new one, it'll break any existing deployments using the append-only index schema.

We hope that the mutable index will be phased out fairly shortly, so this strange situation should ideally be temporary.

fabiotudone-da

LGTM!

Instead of storing the status code and message, we store a serialized `google.rpc.Status` protocol buffers message. This allows us to pass through any additional information reported by the driver `ReadService`. The migration is done in Scala because constructing protobuf messages in SQL turns out to be very, very unpleasant. CHANGELOG_BEGIN CHANGELOG_END

...gration-api/src/main/resources/db/migration/h2database-appendonly/V1__Append_only_schema.sql

Serializing the details but keeping the code and message columns populated.

miklos-da

Elegant solution!

…#10600) * participant-integration-api: Construct completions in one place. * sandbox-classic: Inline `CompletionFromTransaction#apply`. It's only used here; there's no reason to keep it in the _participant-integration-api_. * participant-integration-api: Store a status gRPC protobuf. Instead of storing the status code and message, we store a serialized `google.rpc.Status` protocol buffers message. This allows us to pass through any additional information reported by the driver `ReadService`. The migration is only done for the append-only database, and preserves old data in the existing columns. New data will only be written to the new column. CHANGELOG_BEGIN CHANGELOG_END * participant-integration-api: Improve comments in migrations. Co-authored-by: Fabio Tudone <fabio.tudone@digitalasset.com> * participant-integration-api: Further improvements to migrations. * participant-integration-api: Store the rejection status as 3 columns. Serializing the details but keeping the code and message columns populated. * participant-integration-api: Publish the indexer protobuf to Maven. Co-authored-by: Fabio Tudone <fabio.tudone@digitalasset.com>

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. @SamirTalwar-DA is in charge of this release. Commit log: ``` 640fb68 Make Index DB enable multiple party additions [DPP-546] (#10623) b22de68 LF: Contract ID suffix check in Preprocessor (#10642) 7b94b06 Map shortened scala test suite names to long names on Windows (#10628) 6e4a24c participant-state: Generate correct gRPC error codes by v2 `WriteService` [KVL-1081] (#10656) 663781a Update curl 7.73.0 --> 7.78.0 (#10655) a471f15 Dpp-558 Fix startexclusive queries on oracle (#10649) e99254f Augment `completion.proto` with deduplication-related info [KVL-1057] (#10619) a00608c participant-integration-api: Accommodate changes to max dedup time. (#10650) 29c546c [Divulgence pruning] Added `prune_all_divulged_contracts` to PruneRequest [DPP-534] (#10635) dea57ca In-memory fan-out optimizations (#10558) 77eb366 [JSON-API] key_hash field to speed up fetchByKey queries (#10631) 5001329 LF: Comparisons fail at runtime if comparing local vs global CIDs (#10630) 055be4b Disable deprecation warnings for data-dependencies (#10647) c155935 clean-up gsg README (#10641) 8501832 DPP-468 StorageBackend tests (#10529) 4e08b47 update NOTICES file (#10645) 733590d ledger-api-health: Use the Scala health status values everywhere. (#10640) 5b837ec Ledger API: add `buf` checks [KVL-1045] (#10625) f77cd0a participant-integration-api: Attempt to fix RecoveringIndexerSpec. (#10639) 9d7f60f participant-integration-api: Fix a flaky test. (#10637) 4a9331c Upgrade Nixpkgs [KVL-1045] (#10624) 01b6e89 update compat versions for 1.17.0-snapshot.20210817.7604.0.0c187853 (#10610) b578b0e Reminder to put an empty line between subject and body (#10638) c0fbad1 participant-integration-api: Remove `limitMaxCommandsInFlight`. (#10626) b4af6d1 Canton testing: Mark one more DeeplyNestedValueIT test flaky (#10636) e807f4a Upgrade to a newer canton version (post 0.27.0 snapshot version) (#10632) c4513f2 Oracle append-only schema: enable contract id index on participant_events_xxxx tables (#10633) 3598e09 LF: Drop contract ID Freshness check (#10620) 37c999e ledger-on-sql: Increase the concurrency for conformance tests. (#10622) 46e8c7d DPP-460 Extract constant for event sequential IDs (#10564) 121047e DPP-460 Parameter storage consolidation (#10472) 569612a Drop broken symlink config (#10616) 6d0109f Support $ in daml-lf identifiers in the parser (#10609) c38703e participant-integration-api: Store a status gRPC protobuf. [KVL-1005] (#10600) 0af5b49 make FinalReason a case class (#10614) 8dd136f bazel-tools: Replace `runner` with either `runner_with_port_check` or `runner_with_port_file`. (#10615) 19c3d28 Remove GenMissingString class because it is not used (#10608) 3227e86 Use the port file and dynamic port generation in client/server tests. (#10604) fb19bcb fix gsg-trigger template (#10611) b207702 rotate release duty after 1.17.0-snapshot.20210817.7604.0.0c187853 (#10606) 975a5fb Move DeduplicationPeriod to ledger-api-domain [KVL-1047] (#10590) f8a1820 release 1.17.0-snapshot.20210817.7604.0.0c187853 (#10605) 64abf8a update NOTICES file (#10607) ``` Changelog: ``` [Integration Kit] Corrected gRPC error codes returned by v2 `WriteService` adaptor. - [Ledger API Server] The API server manages a single command tracker per (application ID × submitters) pair. This tracker would read the current ledger configuration's maximum deduplication time on creation, but never updated it, leading to trackers that might inadvertently reject a submission when it should have been accepted. The tracker now reads the latest ledger configuration. - Update schema version for http-json-api query store with new key_hash field - Improved performance for fetchByKey query which now uses key_hash field participant-state - move `DeduplicationPeriod` to ledger-api-domain dc4629f update NOTICES file (#10580) 8b0a0e7 update NOTICES file (#10578) 4b8b67a Upgrade Scalatest to v3.2.9. (#10576) 41e60f7 Upgrade to Scala 2.12.14 and 2.13.6. (#10573) c447898 Fix display of unhandled exceptions in the scenario service (#10572) 4e1a90d Enable --incompatible_remote_results_ignore_disk (#10571) d183ecc rotate release duty after 1.17.0-snapshot.20210811.7560.0.4f9de4ba (#10556) 76ecb44 update compat versions for 1.17.0-snapshot.20210811.7565.0.f1a55aa4 (#10563) 86a03fa Bump bazel max jvm memory (#10569) 1cc136c update NOTICES file (#10565) c69880c ledger-api-test-tool: Enforce test naming standards. (#10562) ee34d0f Track command - use types for error handling instead of grpc statuses [KVL-1005] (#10503) 93c25f3 Release 1.17 snapshot (#10560) ``` Changelog: ``` - [Ledger API Test Tool] The ``TransactionServiceIT`` test suite has been split into many test suites. If you are including or excluding it, you will need to use the new test suite names, or you can use "TransactionService" as a prefix for all of them. If you are including or excluding individual tests, you will need to update your arguments with the new test suite. You can find the new test suite by running the test tool with the ``--list-all`` flag and looking for the test's short identifier. The short identifiers have not changed, with the exception of ``TXNoContractKey``, which has been renamed to ``CKNoContractKey`` and is now in the ``ContractKeysIT`` test suite. * [Daml export] You can now set the ``--all-parties`` option to generate a ledger export as seen by all known parties. ledger-api-client - Propagate definite_answer as metadata in the GRPC response for submit/submitAndWait [JSON API] Ledger connection errors are now logged at every attempt akka-bindings: `LedgerClientBinding.commands` now returns a flow of `Either[CompletionFailure, CompletionSuccess]` instead of `Completion` for clearer error handling. For backwards compatiblity the new return type can be turned back into a `Completion` using `CompletionResponse.toCompletion` ``` ``` CHANGELOG_BEGIN CHANGELOG_END

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. @SamirTalwar-DA is in charge of this release. Commit log: ``` 640fb68 Make Index DB enable multiple party additions [DPP-546] (#10623) b22de68 LF: Contract ID suffix check in Preprocessor (#10642) 7b94b06 Map shortened scala test suite names to long names on Windows (#10628) 6e4a24c participant-state: Generate correct gRPC error codes by v2 `WriteService` [KVL-1081] (#10656) 663781a Update curl 7.73.0 --> 7.78.0 (#10655) a471f15 Dpp-558 Fix startexclusive queries on oracle (#10649) e99254f Augment `completion.proto` with deduplication-related info [KVL-1057] (#10619) a00608c participant-integration-api: Accommodate changes to max dedup time. (#10650) 29c546c [Divulgence pruning] Added `prune_all_divulged_contracts` to PruneRequest [DPP-534] (#10635) dea57ca In-memory fan-out optimizations (#10558) 77eb366 [JSON-API] key_hash field to speed up fetchByKey queries (#10631) 5001329 LF: Comparisons fail at runtime if comparing local vs global CIDs (#10630) 055be4b Disable deprecation warnings for data-dependencies (#10647) c155935 clean-up gsg README (#10641) 8501832 DPP-468 StorageBackend tests (#10529) 4e08b47 update NOTICES file (#10645) 733590d ledger-api-health: Use the Scala health status values everywhere. (#10640) 5b837ec Ledger API: add `buf` checks [KVL-1045] (#10625) f77cd0a participant-integration-api: Attempt to fix RecoveringIndexerSpec. (#10639) 9d7f60f participant-integration-api: Fix a flaky test. (#10637) 4a9331c Upgrade Nixpkgs [KVL-1045] (#10624) 01b6e89 update compat versions for 1.17.0-snapshot.20210817.7604.0.0c187853 (#10610) b578b0e Reminder to put an empty line between subject and body (#10638) c0fbad1 participant-integration-api: Remove `limitMaxCommandsInFlight`. (#10626) b4af6d1 Canton testing: Mark one more DeeplyNestedValueIT test flaky (#10636) e807f4a Upgrade to a newer canton version (post 0.27.0 snapshot version) (#10632) c4513f2 Oracle append-only schema: enable contract id index on participant_events_xxxx tables (#10633) 3598e09 LF: Drop contract ID Freshness check (#10620) 37c999e ledger-on-sql: Increase the concurrency for conformance tests. (#10622) 46e8c7d DPP-460 Extract constant for event sequential IDs (#10564) 121047e DPP-460 Parameter storage consolidation (#10472) 569612a Drop broken symlink config (#10616) 6d0109f Support $ in daml-lf identifiers in the parser (#10609) c38703e participant-integration-api: Store a status gRPC protobuf. [KVL-1005] (#10600) 0af5b49 make FinalReason a case class (#10614) 8dd136f bazel-tools: Replace `runner` with either `runner_with_port_check` or `runner_with_port_file`. (#10615) 19c3d28 Remove GenMissingString class because it is not used (#10608) 3227e86 Use the port file and dynamic port generation in client/server tests. (#10604) fb19bcb fix gsg-trigger template (#10611) b207702 rotate release duty after 1.17.0-snapshot.20210817.7604.0.0c187853 (#10606) 975a5fb Move DeduplicationPeriod to ledger-api-domain [KVL-1047] (#10590) f8a1820 release 1.17.0-snapshot.20210817.7604.0.0c187853 (#10605) 64abf8a update NOTICES file (#10607) ``` Changelog: ``` [Integration Kit] Corrected gRPC error codes returned by v2 `WriteService` adaptor. - [Ledger API Server] The API server manages a single command tracker per (application ID × submitters) pair. This tracker would read the current ledger configuration's maximum deduplication time on creation, but never updated it, leading to trackers that might inadvertently reject a submission when it should have been accepted. The tracker now reads the latest ledger configuration. - Update schema version for http-json-api query store with new key_hash field - Improved performance for fetchByKey query which now uses key_hash field participant-state - move `DeduplicationPeriod` to ledger-api-domain dc4629f update NOTICES file (#10580) 8b0a0e7 update NOTICES file (#10578) 4b8b67a Upgrade Scalatest to v3.2.9. (#10576) 41e60f7 Upgrade to Scala 2.12.14 and 2.13.6. (#10573) c447898 Fix display of unhandled exceptions in the scenario service (#10572) 4e1a90d Enable --incompatible_remote_results_ignore_disk (#10571) d183ecc rotate release duty after 1.17.0-snapshot.20210811.7560.0.4f9de4ba (#10556) 76ecb44 update compat versions for 1.17.0-snapshot.20210811.7565.0.f1a55aa4 (#10563) 86a03fa Bump bazel max jvm memory (#10569) 1cc136c update NOTICES file (#10565) c69880c ledger-api-test-tool: Enforce test naming standards. (#10562) ee34d0f Track command - use types for error handling instead of grpc statuses [KVL-1005] (#10503) 93c25f3 Release 1.17 snapshot (#10560) ``` Changelog: ``` - [Ledger API Test Tool] The ``TransactionServiceIT`` test suite has been split into many test suites. If you are including or excluding it, you will need to use the new test suite names, or you can use "TransactionService" as a prefix for all of them. If you are including or excluding individual tests, you will need to update your arguments with the new test suite. You can find the new test suite by running the test tool with the ``--list-all`` flag and looking for the test's short identifier. The short identifiers have not changed, with the exception of ``TXNoContractKey``, which has been renamed to ``CKNoContractKey`` and is now in the ``ContractKeysIT`` test suite. * [Daml export] You can now set the ``--all-parties`` option to generate a ledger export as seen by all known parties. ledger-api-client - Propagate definite_answer as metadata in the GRPC response for submit/submitAndWait [JSON API] Ledger connection errors are now logged at every attempt akka-bindings: `LedgerClientBinding.commands` now returns a flow of `Either[CompletionFailure, CompletionSuccess]` instead of `Completion` for clearer error handling. For backwards compatiblity the new return type can be turned back into a `Completion` using `CompletionResponse.toCompletion` ``` ``` CHANGELOG_BEGIN CHANGELOG_END Co-authored-by: Azure Pipelines DAML Build <support@digitalasset.com>

SamirTalwar added 2 commits August 17, 2021 15:03

participant-integration-api: Construct completions in one place.

66e9e93

sandbox-classic: Inline CompletionFromTransaction#apply.

2527ec8

It's only used here; there's no reason to keep it in the _participant-integration-api_.

ghost self-requested a review as a code owner August 17, 2021 13:33

ghost self-requested a review August 17, 2021 13:33

ghost commented Aug 17, 2021

View reviewed changes

fabiotudone-da reviewed Aug 17, 2021

View reviewed changes

nicu-da reviewed Aug 17, 2021

View reviewed changes

ghost force-pushed the samir/indexer/rejections branch from f4d6de7 to 1fd2a37 Compare August 17, 2021 14:09

SamirTalwar and others added 2 commits August 17, 2021 19:28

participant-integration-api: Improve comments in migrations.

5217ed4

Co-authored-by: Fabio Tudone <fabio.tudone@digitalasset.com>

Merge remote-tracking branch 'origin/main' into samir/indexer/rejections

82002a7

nmarton-da approved these changes Aug 17, 2021

View reviewed changes

participant-integration-api: Further improvements to migrations.

c7a74a0

ghost force-pushed the samir/indexer/rejections branch from f9f27b5 to c7a74a0 Compare August 18, 2021 07:43

ghost requested a review from fabiotudone-da August 18, 2021 07:46

fabiotudone-da approved these changes Aug 18, 2021

View reviewed changes

miklos-da reviewed Aug 18, 2021

View reviewed changes

...gration-api/src/main/resources/db/migration/h2database-appendonly/V1__Append_only_schema.sql Outdated Show resolved Hide resolved

...gration-api/src/main/resources/db/migration/h2database-appendonly/V1__Append_only_schema.sql Outdated Show resolved Hide resolved

participant-integration-api: Store the rejection status as 3 columns.

bacc41f

Serializing the details but keeping the code and message columns populated.

ghost requested a review from miklos-da August 18, 2021 14:51

miklos-da approved these changes Aug 18, 2021

View reviewed changes

ghost added the automerge label Aug 18, 2021

participant-integration-api: Publish the indexer protobuf to Maven.

162772b

mergify bot merged commit c38703e into main Aug 18, 2021

mergify bot deleted the samir/indexer/rejections branch August 18, 2021 17:59

fabiotudone-da mentioned this pull request Aug 26, 2021

Command dedup: add columns to completions in append-only schemas [KVL-1057] #10652

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

participant-integration-api: Store a status gRPC protobuf. [KVL-1005] #10600

participant-integration-api: Store a status gRPC protobuf. [KVL-1005] #10600

ghost commented Aug 17, 2021 •

edited by ghost

Loading

ghost Aug 17, 2021

fabiotudone-da commented Aug 17, 2021

nicu-da left a comment

nmarton-da left a comment

nmarton-da Aug 17, 2021

nmarton-da Aug 17, 2021

ghost Aug 18, 2021

rautenrieth-da Aug 18, 2021

nmarton-da Aug 18, 2021

rautenrieth-da Aug 18, 2021

nicu-da commented Aug 18, 2021

ghost commented Aug 18, 2021

fabiotudone-da commented Aug 18, 2021

ghost commented Aug 18, 2021

fabiotudone-da left a comment

miklos-da left a comment

participant-integration-api: Store a status gRPC protobuf. [KVL-1005] #10600

participant-integration-api: Store a status gRPC protobuf. [KVL-1005] #10600

Conversation

ghost commented Aug 17, 2021 • edited by ghost Loading

Pull Request Checklist

ghost Aug 17, 2021

Choose a reason for hiding this comment

fabiotudone-da commented Aug 17, 2021

nicu-da left a comment

Choose a reason for hiding this comment

nmarton-da left a comment

Choose a reason for hiding this comment

nmarton-da Aug 17, 2021

Choose a reason for hiding this comment

nmarton-da Aug 17, 2021

Choose a reason for hiding this comment

ghost Aug 18, 2021

Choose a reason for hiding this comment

rautenrieth-da Aug 18, 2021

Choose a reason for hiding this comment

nmarton-da Aug 18, 2021

Choose a reason for hiding this comment

rautenrieth-da Aug 18, 2021

Choose a reason for hiding this comment

nicu-da commented Aug 18, 2021

ghost commented Aug 18, 2021

fabiotudone-da commented Aug 18, 2021

ghost commented Aug 18, 2021

fabiotudone-da left a comment

Choose a reason for hiding this comment

miklos-da left a comment

Choose a reason for hiding this comment

ghost commented Aug 17, 2021 •

edited by ghost

Loading