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.

Sign up for GitHub

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

Jump to bottom

[Divulgence pruning] Added prune_all_divulged_contracts to PruneRequest [DPP-534] #10635

Merged
merged 3 commits into from
Aug 23, 2021
Merged

[Divulgence pruning] Added prune_all_divulged_contracts to PruneRequest [DPP-534] #10635

merged 3 commits into from
Aug 23, 2021

Conversation

tudor-da
Copy link
Contributor

@tudor-da tudor-da commented Aug 20, 2021 •
edited
Loading

This PR enriches the PruningRequest with an additional flag prune_all_divulged_contracts.

  • The flag is passed to the WriteParticipantPruningService (v1 and v2)

CHANGELOG_BEGIN
CHANGELOG_END

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.

@tudor-da tudor-da changed the title Added prune_all_divulged_contracts to ParticipantPruningService.Prune [Divulgence pruning] Added prune_all_divulged_contracts to PruneRequest Aug 20, 2021
@tudor-da tudor-da force-pushed the tudor/dpp-534-add-prune_all_divulged_contracts-to-Ledger-API branch from d35fd23 to 6907b7d Compare August 20, 2021 06:42
@tudor-da tudor-da changed the title [Divulgence pruning] Added prune_all_divulged_contracts to PruneRequest [Divulgence pruning] Added prune_all_divulged_contracts to PruneRequest [DPP-534 Aug 20, 2021
@tudor-da tudor-da changed the title [Divulgence pruning] Added prune_all_divulged_contracts to PruneRequest [DPP-534 [Divulgence pruning] Added prune_all_divulged_contracts to PruneRequest [DPP-534] Aug 20, 2021
@tudor-da tudor-da force-pushed the tudor/dpp-534-add-prune_all_divulged_contracts-to-Ledger-API branch from 6907b7d to 6bd297a Compare August 20, 2021 07:19
@tudor-da tudor-da marked this pull request as ready for review August 20, 2021 07:23
@tudor-da tudor-da requested review from a team as code owners August 20, 2021 07:23
meiersi-da
meiersi-da reviewed Aug 20, 2021
View reviewed changes
Copy link
Contributor

@meiersi-da meiersi-da left a comment

Choose a reason for hiding this comment

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

Looks good up to two concerns:

  1. The sandbox-classic codebase must return NOT_IMPLEMENTED when the prune_all_divulged_contracts flag is set. I'd expect this PR to make that change, or is this the wrong place?
  2. Do the KV ledgers support prune_all_divulged_contracts as of this PR? If not, I'd suggest for them to return NOT_IMPLEMENTED when the prune_all_divulged_contracts flag is set.

This way this PR leaves the codebase in a consistent state, and later changes can replace the NOT_IMPLEMENTED errors with their actual implementation.

What do you think?

@tudor-da
Copy link
Contributor Author

tudor-da commented Aug 22, 2021 •
edited
Loading

Looks good up to two concerns:

1. The `sandbox-classic` codebase must return `NOT_IMPLEMENTED` when the `prune_all_divulged_contracts` flag is set. I'd expect this PR to make that change, or is this the wrong place?

2. Do the KV ledgers support `prune_all_divulged_contracts` as of this PR? If not, I'd suggest for them to return  `NOT_IMPLEMENTED` when the `prune_all_divulged_contracts` flag is set.

This way this PR leaves the codebase in a consistent state, and later changes can replace the NOT_IMPLEMENTED errors with their actual implementation.

What do you think?

Afaict, sandbox-classic already returns UNIMPLEMENTED (see here). For KV ledgers, I didn't see an issue if the interface is slightly inconsistent between the commits of this epic. However, the change is small, I'll amend.

@tudor-da tudor-da requested a review from meiersi-da August 22, 2021 17:51
@tudor-da tudor-da force-pushed the tudor/dpp-534-add-prune_all_divulged_contracts-to-Ledger-API branch 3 times, most recently from df9cb86 to 8081318 Compare August 23, 2021 04:49
meiersi-da
meiersi-da approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@meiersi-da meiersi-da left a comment

Choose a reason for hiding this comment

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

Thx @tudor-da . Sorry for overlooking the Status.UNIMPLEMENTED for sandbox-classic.

LGTM.

@tudor-da
Added prune_all_divulged_contracts to ParticipantPruningService.Prune
305dc40 
* KeyValueParticipantStateWriter.prune returns UNIMPLEMENTED if called with `pruneAllDivulgedContracts` set

CHANGELOG_BEGIN
CHANGELOG_END
@tudor-da tudor-da force-pushed the tudor/dpp-534-add-prune_all_divulged_contracts-to-Ledger-API branch from 8081318 to c6f68db Compare August 23, 2021 07:29
tudor-da
tudor-da commented Aug 23, 2021
View reviewed changes
...ate/src/main/scala/com/daml/ledger/participant/state/v2/WriteParticipantPruningService.scala
*/
def prune(
pruneUpToInclusive: Offset,
submissionId: Ref.SubmissionId,
pruneAllDivulgedContracts: Boolean,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC @oliverse-da Coming your way

Copy link
Contributor

Choose a reason for hiding this comment

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

fyi @matthiasS-da - merged now so will be in this week's daml snapshot upgrade.

@tudor-da tudor-da force-pushed the tudor/dpp-534-add-prune_all_divulged_contracts-to-Ledger-API branch from c6f68db to 51a5c1d Compare August 23, 2021 10:10
fabiotudone-da
fabiotudone-da approved these changes Aug 23, 2021
View reviewed changes
...n/scala/com/daml/ledger/api/testtool/infrastructure/participant/ParticipantTestContext.scala
def prune(
pruneUpTo: LedgerOffset,
attempts: Int = 10,
// TODO Divulgence pruning: Change default to `true` once all divulgence pruning is implemented
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is tracked, I think you can remove this and other occurrences.

@mergify mergify bot merged commit 29c546c into main Aug 23, 2021
@mergify mergify bot deleted the tudor/dpp-534-add-prune_all_divulged_contracts-to-Ledger-API branch August 23, 2021 20:52
azure-pipelines bot pushed a commit that referenced this pull request Aug 25, 2021
release 1.17.0-snapshot.20210824.7647.0.640fb683
b289daf 
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
mergify bot pushed a commit that referenced this pull request Aug 25, 2021
@azure-pipelines
release 1.17.0-snapshot.20210824.7647.0.640fb683 (#10659)
430c1cc 
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 <[email protected]>
@meiersi-da meiersi-da mentioned this pull request Sep 8, 2021
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverse-da oliverse-da oliverse-da left review comments

@meiersi-da meiersi-da meiersi-da approved these changes

@mziolekda mziolekda mziolekda approved these changes

@fabiotudone-da fabiotudone-da fabiotudone-da approved these changes

@hubert-da hubert-da hubert-da approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tudor-da @meiersi-da @mziolekda @oliverse-da @fabiotudone-da @hubert-da