KV ledgers: disable participant side command de-duplication [KVL-1083] #11095

fabiotudone-da · 2021-09-30T10:25:01Z

Description:

Disables participant side command deduplication in KeyValueParticipantStateWriter.
Informs KVCommandDeduplicationIT and AppendOnlyKVCommandDeduplicationIT about the above.
Leaves nevertheless participant side command deduplication enabled for sandbox-next when using static time (as this combination is currently unsupported by KV).
Fixes an issue in CommandDeduplicationBase when triggering async deduplication.
Adapts KVDeduplicatioBase so that it doesn't rely on participant side command deduplication.
Disable cross-compatibility checks between this ledger API test tool and previous platform versions.

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.

CHANGELOG_BEGIN CHANGELOG_END

…d-dedup

…oken

… on static time

… it's broken" This reverts commit 3dac41a

fabiotudone-da · 2021-10-04T09:36:50Z

...ala/com/daml/ledger/api/testtool/infrastructure/deduplication/CommandDeduplicationBase.scala

+      _ <- submitRequestAndAssertDeduplication(ledger)(aliceRequest, alice)

      // Submit another command that uses same commandId, but is submitted by Bob
      _ <- submitRequestAndAssertCompletionAccepted(ledger)(bobRequest, bob)
-      _ <- submitRequestAndAssertDeduplication(ledger)(bobRequest)
+      _ <- submitRequestAndAssertDeduplication(ledger)(bobRequest, bob)


This fix is akin to #11083.

compatibility/bazel_tools/testing.bzl

ghost

Looks good!

compatibility/bazel_tools/testing.bzl

fabiotudone-da · 2021-10-04T15:24:47Z

How can I avoid checking against platform-0.0.0?

garyverhaegen-da · 2021-10-04T17:34:26Z

How can I avoid checking against platform-0.0.0?

As HEAD (0.0.0) is considered "after" all other versions, if you don't specify an end to your exclusion range, it will include HEAD. Though given that you currently only have an end, that would mean the test is always skipped after if test-tool > 1.18.0-snapshot.20210928.7948.1, and at that point you might as well just delete the tests, no?

fabiotudone-da · 2021-10-05T06:50:27Z

As HEAD (0.0.0) is considered "after" all other versions, if you don't specify an end to your exclusion range, it will include HEAD.
Though given that you currently only have an end, that would mean the test is always skipped after if test-tool > 1.18.0-snapshot.20210928.7948.1, and at that point you might as well just delete the tests, no?

I think my understanding of the cross-test exclusions is still flawed somewhere. This change updates KV ledgers in such a way that previous ledger API test tools would fail on them; it also updates the ledger API test tool so that it would fail on previous KV ledgers. Which exclusion should I use in such a case?

garyverhaegen-da · 2021-10-05T10:22:35Z

As HEAD (0.0.0) is considered "after" all other versions, if you don't specify an end to your exclusion range, it will include HEAD.
Though given that you currently only have an end, that would mean the test is always skipped after if test-tool > 1.18.0-snapshot.20210928.7948.1, and at that point you might as well just delete the tests, no?

I think my understanding of the cross-test exclusions is still flawed somewhere. This change updates KV ledgers in such a way that previous ledger API test tools would fail on them; it also updates the ledger API test tool so that it would fail on previous KV ledgers. Which exclusion should I use in such a case?

The logic (as far as I understand it, at least 😅) is that if the api test tool version is in the "external" (JSON-ly speaking) range, and the ledger version is in the "internal" range, then the exclusions are passed as CLI parameters.

It looks like what you want to say in this case is to add the exclusions if either:

The test tool is higher than [last current release] (not included) and the ledger is before [last current release] (included), or
The test tool is lower than [last current release] (included) and the ledger is higher than [last current release] (excluded).

Does that sound right? If so, you'll need two exclusion blocks to express that.

fabiotudone-da · 2021-10-05T10:47:20Z

As HEAD (0.0.0) is considered "after" all other versions, if you don't specify an end to your exclusion range, it will include HEAD.
Though given that you currently only have an end, that would mean the test is always skipped after if test-tool > 1.18.0-snapshot.20210928.7948.1, and at that point you might as well just delete the tests, no?

I think my understanding of the cross-test exclusions is still flawed somewhere. This change updates KV ledgers in such a way that previous ledger API test tools would fail on them; it also updates the ledger API test tool so that it would fail on previous KV ledgers. Which exclusion should I use in such a case?

The logic (as far as I understand it, at least sweat_smile) is that if the api test tool version is in the "external" (JSON-ly speaking) range, and the ledger version is in the "internal" range, then the exclusions are passed as CLI parameters.

It looks like what you want to say in this case is to add the exclusions if either:

The test tool is higher than [last current release] (not included) and the ledger is before [last current release] (included), or

The test tool is lower than [last current release] (included) and the ledger is higher than [last current release] (excluded).

Does that sound right? If so, you'll need two exclusion blocks to express that.

bf5a815 expresses what I want at a PR level (i.e., HEAD ledger API tool is incompatible with all released platforms and HEAD platform is incompatible with all released ledger API tools) and it works and it would also work for the nightly checks, but it will need to change into a proper release as soon as a new snapshot is tagged. AFAIU, since HEAD is greater than anything, I can't use e.g. 1.18.0-snapshot.20210928.7948.1 (> last current release) in its stead because HEAD would still be included. Do you see less inconvenient alternatives?

fabiotudone-da · 2021-10-05T10:55:00Z

bf5a815 expresses what I want at a PR level (i.e., HEAD ledger API tool is incompatible with all released platforms and HEAD platform is incompatible with all released ledger API tools) and it works and it would also work for the nightly checks, but it will need to change into a proper release as soon as a new snapshot is tagged. AFAIU, since HEAD is greater than anything, I can't use e.g. 1.18.0-snapshot.20210928.7948.1 (> last current release) in its stead because HEAD would still be included. Do you see less inconvenient alternatives?

@garyverhaegen-da On second thought, I think your suggestion is right and should work (because HEAD is excluded at the top of the interval).

fabiotudone-da · 2021-10-05T11:31:37Z

Thanks @garyverhaegen-da!

garyverhaegen-da · 2021-10-05T11:40:06Z

LGTM, and I don't think you need to change to "real" releases once we do the next one. I, at least, don't mind if the bounds we use in that file are not real releases.

garyverhaegen-da · 2021-10-05T11:43:46Z

Nitpick, though: if this commit introduces a break in the compat tests, does that mean it introduces a breaking change that our users should be informed of? It seems a bit weird to me to see a PR that needs to add these kinds of exclusions while not having any CHANGELOG entry.

fabiotudone-da · 2021-10-05T11:44:37Z

I don't think you need to change to "real" releases once we do the next one. I, at least, don't mind if the bounds we use in that file are not real releases.

Yes, your solution has this advantage.

fabiotudone-da · 2021-10-05T11:50:58Z

Nitpick, though: if this commit introduces a break in the compat tests, does that mean it introduces a breaking change that our users should be informed of? It seems a bit weird to me to see a PR that needs to add these kinds of exclusions while not having any CHANGELOG entry.

It's a good point but this change is under the hood, i.e. it only switches the provider of a ledger API behavior (command deduplication) from the participant to the KV driver. Since the ledger API doesn't specify whether deduplication responses are sync or async, this is invisible to the user [that doesn't rely on unspecified API behavior]. @miklos-da @andreaslochbihler-da do you agree?

EDIT: note that the ledger API test tool had to be changed as well because it also contains ledger implementation-specific (KV-specific in this case) tests.

andreaslochbihler-da · 2021-10-05T12:32:26Z

The new exclusions refer to kv-specific tests and therefore are actually not conformance tests. So no changelog entry for those needed IMO.

@adriaanm-da

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. @adriaanm-da is in charge of this release. Commit log: ``` cfcdc13 Add deprecation output for sandbox classic (#11119) ac5c52d DPP-609 Add StorageBackendTestsDeduplication (#11088) 55af7ad HttpServiceTestFixture provides a sandbox-classic ledger used for performance tests (#11128) 63ab3f3 Fix change ID references in services.rst (#11130) 31db15d Port error code changes (#11113) 9fd8182 move reusable functions from ContractsFetch to new fetch-contracts library (#11057) c2084f6 Try and improve the explanation of compatibility test exclusions (#11114) 4df9b7c Rename command completion dedup time to duration in participant code [KVL-1057] (#10901) 11bc22d LF: builtins to create, signatory, and obersver on interface payload. (#11120) d4cb1f9 KV ledgers: disable participant side command de-duplication [KVL-1083] (#11095) b9a5a83 Minor: fix trace logging in `TimeBasedWriteSetSelector` (#11117) 018e908 [JSON-API] Make key_hash idx non unique (#11102) 429f437 Remove HA indexer integration test on H2 (#11104) 9cffa1f LF: check LF transaction protobuf aginst local snapshots (#11064) d7ee278 Optional table prefix for trigger service (#11047) 268b2d3 Command deduplication - test case that uses CommandService and CommandSubmissionService [KVL-1106] (#11098) fac05f6 bypass value enrichment for the performance test (#11112) 4860271 Port file was not written to... where? (#11108) a8d703b Increase akka server request-timeout for HttpServiceIntegrationTests (#11109) 37d94aa Add unit for daily performance tests (#11107) eb4ac8a Increase timeouts for resetting database (#11103) db75f7d interface methods: Scala Typechecker (#11097) d3e6f16 [JSON-API] Migrating tests to use sandbox next (#11034) ffc8d68 Data dependencies deserialize imports (#11037) 4b31bf0 update compat versions for 1.17.0 (#11075) 9fd6326 interfaces: consuming/non-consuming iface choices (#11009) 229ce47 Data dependencies serialize imports (#11038) 5d9ec65 Add test case for parallel command deduplication using mixed clients [KVL-1090] (#11093) d64d965 kvutils: Print state updates before and after normalization. (#11096) be216aa update NOTICES file (#11089) 517e866 Extract common code for command dedup conformance tests [KVL-1090] (#11092) 7a4963b add `--max-inbound-message-size` flag to `daml ledger export` (#11087) 26d10b2 Drop NodeId type parameter (#10921) 3cbd922 More NonEmpty functions (#10930) bcd4686 interface methods: Speedy (#11076) 85adaab [DPP-417][DPP-595] Error codes switching - follow-up (#11074) d7b280b Remove debug print in daml-lf/interpreter build (#11091) 1ed6428 LF: move archive snapshots in a separate directory (#11081) 5e424f8 Command dedup conformance suites readme (#11051) 8290347 DPP-496 HA indexer integration test (#11033) 735c309 Switch from InputStream to Byte Array at the binary content JDBC transport (#11072) 6ae3afa fix perf reporting (#11073) bf801a6 Fix SimpleDeduplicationBasic when participant deduplication is disabled (#11083) 62234dc [Short] Remove unused code (#11079) c1d1521 interface methods: Scala AST (#11070) 7dd9c2d Remove expectations for internal failures from parallel command dedup tests (#11061) 5112599 Explicit discard in daml-lf/interpreter (#11067) 554b36c rotate release duty after 1.18.0-snapshot.20210928.7948.0.b4d00317 (#11059) d156964 Release SDK 1.17.0 (#11062) 6d8cf70 Handle interface methods in LF conversion (#11054) 87ecf1f daml-ledger: better errors for non-json responses. (#11055) ec2d26f [Divulgence pruning] Fixes divulgence pruning offset update query (#11046) f13c6d6 release 1.18.0-snapshot.20210928.7948.0.b4d00317 (#11058) d1805a3 More dependabot fun (#11063) 07b273a update NOTICES file (#11060) ``` Changelog: ``` - [JSON-API] make key_hash indexes non-unique, this fixes a bug where a duplicate key conflict was raised on the query store when the same contract was being witnessed twice by two separate parties * [Trigger Service] Enable the new `tablePrefix` setting in the `--jdbc` flag to add a prefix to all tables used by the trigger service to avoid collisions with other components using the same db-schema. * [Trigger Service] Enable the new ``--allow-existing-schema`` flag to initialize the trigger service on a database with a pre-existing schema. - [Daml Assistant] Add support for `--max-inbound-message-size` flag to the Ledger Export tool. Add cons, find, delete and deleteBy functions to NonEmpty module. ``` CHANGELOG_BEGIN CHANGELOG_END

@adriaanm-da

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. @adriaanm-da is in charge of this release. Commit log: ``` cfcdc13 Add deprecation output for sandbox classic (#11119) ac5c52d DPP-609 Add StorageBackendTestsDeduplication (#11088) 55af7ad HttpServiceTestFixture provides a sandbox-classic ledger used for performance tests (#11128) 63ab3f3 Fix change ID references in services.rst (#11130) 31db15d Port error code changes (#11113) 9fd8182 move reusable functions from ContractsFetch to new fetch-contracts library (#11057) c2084f6 Try and improve the explanation of compatibility test exclusions (#11114) 4df9b7c Rename command completion dedup time to duration in participant code [KVL-1057] (#10901) 11bc22d LF: builtins to create, signatory, and obersver on interface payload. (#11120) d4cb1f9 KV ledgers: disable participant side command de-duplication [KVL-1083] (#11095) b9a5a83 Minor: fix trace logging in `TimeBasedWriteSetSelector` (#11117) 018e908 [JSON-API] Make key_hash idx non unique (#11102) 429f437 Remove HA indexer integration test on H2 (#11104) 9cffa1f LF: check LF transaction protobuf aginst local snapshots (#11064) d7ee278 Optional table prefix for trigger service (#11047) 268b2d3 Command deduplication - test case that uses CommandService and CommandSubmissionService [KVL-1106] (#11098) fac05f6 bypass value enrichment for the performance test (#11112) 4860271 Port file was not written to... where? (#11108) a8d703b Increase akka server request-timeout for HttpServiceIntegrationTests (#11109) 37d94aa Add unit for daily performance tests (#11107) eb4ac8a Increase timeouts for resetting database (#11103) db75f7d interface methods: Scala Typechecker (#11097) d3e6f16 [JSON-API] Migrating tests to use sandbox next (#11034) ffc8d68 Data dependencies deserialize imports (#11037) 4b31bf0 update compat versions for 1.17.0 (#11075) 9fd6326 interfaces: consuming/non-consuming iface choices (#11009) 229ce47 Data dependencies serialize imports (#11038) 5d9ec65 Add test case for parallel command deduplication using mixed clients [KVL-1090] (#11093) d64d965 kvutils: Print state updates before and after normalization. (#11096) be216aa update NOTICES file (#11089) 517e866 Extract common code for command dedup conformance tests [KVL-1090] (#11092) 7a4963b add `--max-inbound-message-size` flag to `daml ledger export` (#11087) 26d10b2 Drop NodeId type parameter (#10921) 3cbd922 More NonEmpty functions (#10930) bcd4686 interface methods: Speedy (#11076) 85adaab [DPP-417][DPP-595] Error codes switching - follow-up (#11074) d7b280b Remove debug print in daml-lf/interpreter build (#11091) 1ed6428 LF: move archive snapshots in a separate directory (#11081) 5e424f8 Command dedup conformance suites readme (#11051) 8290347 DPP-496 HA indexer integration test (#11033) 735c309 Switch from InputStream to Byte Array at the binary content JDBC transport (#11072) 6ae3afa fix perf reporting (#11073) bf801a6 Fix SimpleDeduplicationBasic when participant deduplication is disabled (#11083) 62234dc [Short] Remove unused code (#11079) c1d1521 interface methods: Scala AST (#11070) 7dd9c2d Remove expectations for internal failures from parallel command dedup tests (#11061) 5112599 Explicit discard in daml-lf/interpreter (#11067) 554b36c rotate release duty after 1.18.0-snapshot.20210928.7948.0.b4d00317 (#11059) d156964 Release SDK 1.17.0 (#11062) 6d8cf70 Handle interface methods in LF conversion (#11054) 87ecf1f daml-ledger: better errors for non-json responses. (#11055) ec2d26f [Divulgence pruning] Fixes divulgence pruning offset update query (#11046) f13c6d6 release 1.18.0-snapshot.20210928.7948.0.b4d00317 (#11058) d1805a3 More dependabot fun (#11063) 07b273a update NOTICES file (#11060) ``` Changelog: ``` - [JSON-API] make key_hash indexes non-unique, this fixes a bug where a duplicate key conflict was raised on the query store when the same contract was being witnessed twice by two separate parties * [Trigger Service] Enable the new `tablePrefix` setting in the `--jdbc` flag to add a prefix to all tables used by the trigger service to avoid collisions with other components using the same db-schema. * [Trigger Service] Enable the new ``--allow-existing-schema`` flag to initialize the trigger service on a database with a pre-existing schema. - [Daml Assistant] Add support for `--max-inbound-message-size` flag to the Ledger Export tool. Add cons, find, delete and deleteBy functions to NonEmpty module. ``` CHANGELOG_BEGIN CHANGELOG_END Co-authored-by: Azure Pipelines DAML Build <[email protected]>

CHANGELOG_BEGIN CHANGELOG_END

…est tool CHANGELOG_BEGIN CHANGELOG_END

CHANGELOG_BEGIN CHANGELOG_END

nicu-da and others added 4 commits September 30, 2021 09:33

Extract common code for command dedup conformance tests

27e0ded

CHANGELOG_BEGIN CHANGELOG_END

Revert changes for parallel tests

4cd1ed9

Fix compile warnings

b25ab62

Disable participant deduplication for KV

4ba684d

CHANGELOG_BEGIN CHANGELOG_END

fabiotudone-da changed the base branch from main to nicuda/kvl-1106/cleanup September 30, 2021 10:25

Merge branch 'main' into fabiotudone-da/kv/disable-participant-comman…

07d75d0

…d-dedup

Base automatically changed from nicuda/kvl-1106/cleanup to main September 30, 2021 11:20

fabiotudone-da added 3 commits September 30, 2021 13:53

Always call submitRequestAndAssertDeduplication with parties

b60234d

Merge branch 'main' into fabiotudone-da/kv/disable-participant-comman…

f4bb438

…d-dedup

Fix party in CommandDeduplicationBase

51563af

fabiotudone-da changed the title ~~KV ledgers: disable participant command de-duplication [KVL-1083]~~ KV ledgers: disable participant side command de-duplication [KVL-1083] Sep 30, 2021

fabiotudone-da added 4 commits October 4, 2021 09:54

Don't test KV-based command deduplication with static time as it's br…

3dac41a

…oken

Don't disable participant-side command deduplication for sandbox-next…

f041e27

… on static time

Revert "Don't test KV-based command deduplication with static time as…

a50bc6e

… it's broken" This reverts commit 3dac41a

Add missing dependency

344f426

fabiotudone-da commented Oct 4, 2021

View reviewed changes

compatibility/bazel_tools/testing.bzl Outdated Show resolved Hide resolved

fabiotudone-da marked this pull request as ready for review October 4, 2021 12:23

fabiotudone-da requested review from aherrmann-da, cocreature, garyverhaegen-da and a team as code owners October 4, 2021 12:23

Don't cross-test this ledger API test tool with existing versions

a3730e9

fabiotudone-da force-pushed the fabiotudone-da/kv/disable-participant-command-dedup branch from 5d27fa9 to a3730e9 Compare October 4, 2021 12:48

ghost approved these changes Oct 4, 2021

View reviewed changes

compatibility/bazel_tools/testing.bzl Outdated Show resolved Hide resolved

fabiotudone-da requested review from hubert-da and oliverse-da October 4, 2021 12:49

hubert-da approved these changes Oct 4, 2021

View reviewed changes

Add the current PR to the list of PRs that resulted in exclusions

95ed817

nicu-da approved these changes Oct 4, 2021

View reviewed changes

fabiotudone-da force-pushed the fabiotudone-da/kv/disable-participant-command-dedup branch from 1c34c58 to 95ed817 Compare October 4, 2021 15:45

Apply @garyverhaegen-da's suggestion for compatibility exclusions

f16424b

fabiotudone-da merged commit d4cb1f9 into main Oct 5, 2021

fabiotudone-da deleted the fabiotudone-da/kv/disable-participant-command-dedup branch October 5, 2021 11:31

fabiotudone-da added a commit that referenced this pull request Oct 6, 2021

Fix compatibility exclusions introduced by #11095

b6dfce8

CHANGELOG_BEGIN CHANGELOG_END

fabiotudone-da mentioned this pull request Oct 6, 2021

Fix compatibility exclusions introduced by #11095 [KVL-1083] #11141

Merged

6 tasks

fabiotudone-da added a commit that referenced this pull request Oct 6, 2021

Exclude CommandDeduplicationIT fixed in #11095 for older ledger API t…

ca9b224

…est tool CHANGELOG_BEGIN CHANGELOG_END

garyverhaegen-da pushed a commit that referenced this pull request Oct 6, 2021

Fix compatibility exclusions introduced by #11095 [KVL-1083] (#11141)

4295ecd

CHANGELOG_BEGIN CHANGELOG_END

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KV ledgers: disable participant side command de-duplication [KVL-1083] #11095

KV ledgers: disable participant side command de-duplication [KVL-1083] #11095

fabiotudone-da commented Sep 30, 2021 •

edited

Loading

fabiotudone-da Oct 4, 2021 •

edited

Loading

ghost left a comment

fabiotudone-da commented Oct 4, 2021

garyverhaegen-da commented Oct 4, 2021

fabiotudone-da commented Oct 5, 2021

garyverhaegen-da commented Oct 5, 2021 •

edited

Loading

fabiotudone-da commented Oct 5, 2021 •

edited

Loading

fabiotudone-da commented Oct 5, 2021

fabiotudone-da commented Oct 5, 2021

garyverhaegen-da commented Oct 5, 2021

garyverhaegen-da commented Oct 5, 2021

fabiotudone-da commented Oct 5, 2021

fabiotudone-da commented Oct 5, 2021 •

edited

Loading

andreaslochbihler-da commented Oct 5, 2021

KV ledgers: disable participant side command de-duplication [KVL-1083] #11095

KV ledgers: disable participant side command de-duplication [KVL-1083] #11095

Conversation

fabiotudone-da commented Sep 30, 2021 • edited Loading

Description:

Pull Request Checklist

fabiotudone-da Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

fabiotudone-da commented Oct 4, 2021

garyverhaegen-da commented Oct 4, 2021

fabiotudone-da commented Oct 5, 2021

garyverhaegen-da commented Oct 5, 2021 • edited Loading

fabiotudone-da commented Oct 5, 2021 • edited Loading

fabiotudone-da commented Oct 5, 2021

fabiotudone-da commented Oct 5, 2021

garyverhaegen-da commented Oct 5, 2021

garyverhaegen-da commented Oct 5, 2021

fabiotudone-da commented Oct 5, 2021

fabiotudone-da commented Oct 5, 2021 • edited Loading

andreaslochbihler-da commented Oct 5, 2021

fabiotudone-da commented Sep 30, 2021 •

edited

Loading

fabiotudone-da Oct 4, 2021 •

edited

Loading

garyverhaegen-da commented Oct 5, 2021 •

edited

Loading

fabiotudone-da commented Oct 5, 2021 •

edited

Loading

fabiotudone-da commented Oct 5, 2021 •

edited

Loading