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

Command deduplication - KV conformance test for usage of max deduplication duration [KVL-1098] #10846

Merged
merged 17 commits into from
Sep 13, 2021
Merged

Command deduplication - KV conformance test for usage of max deduplication duration [KVL-1098] #10846

merged 17 commits into from
Sep 13, 2021

Conversation

nicu-da
Copy link
Contributor

@nicu-da nicu-da commented Sep 10, 2021

Pull Request Checklist

Add a conformance test scenario for the KV ledgers usage of max_deduplication_duration as deduplication period.
Because the Completion only returns the deduplicationPeriod and submissionId if it's an append-only ledger, we had to split the tests once again, adding a more specialized conformance test AppendOnlyKVCommandDeduplicationIT.

I'm not really happy with the way this is designed, it's cumbersome to add tests based on certain ledger features and once we have too many small "divergences" the code gets really difficult to understand, but I couldn't think of a cleaner way of doing this without having more details about the ledger in the tests themselves.

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

nicu-da and others added 12 commits September 9, 2021 10:32
@nicu-da
Always use max_deduplication_duration as deduplication period for com…
2210608 
…mitter side deduplication

Doing this we get the following guarantees:
- Deduplication period is constant for all the submissions therefore this guarantees consistency between "backwards looking" and "forward looking" deduplication

CHANGELOG_BEGIN
kvutils - committer side deduplication always always use max_deduplication_duration + min_skew as a deduplication period for all the requests.
CHANGELOG_END
@nicu-da
Set max_deduplication_duration to 10 seconds for all the conformance …
f4b1a40 
…tests
@nicu-da
Update KV command deduplication conformance tests to account for max …
915edb5 
…deduplication duration used as deduplication period for all the requests
@nicu-da
Merge branch 'main' into nicuda/kvl-1098/use_max_deduplication_duration
2ede50d 
# Conflicts:
#	ledger/ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/suites/KVCommandDeduplicationIT.scala
@nicu-da
Add max deduplication wait for multi participant conformance test
f31b352
@nicu-da
Overwrite the submitter info deduplication period in the transaction …
62f6b31 
…committer so that we always set the max deduplication duration
@nicu-da @miklos-da
Update ledger/participant-state/kvutils/src/test/suite/scala/com/daml…
8471984 
…/ledger/participant/state/kvutils/committer/transaction/TransactionCommitterSpec.scala

Co-authored-by: Miklos <[email protected]>
@nicu-da
Add separate step for overwriting the deduplication period
101ee01
@nicu-da
Code review - reformatting and renames
6b1ddd2
@nicu-da
Add comment for setting the deduplication duration
b6a6434
@nicu-da
Add conformance test for KV committer usage of max deduplication dura…
32e9f9c 
…tion as deduplication duration

CHANGELOG_BEGIN

CHANGELOG_END
@nicu-da
Run AppendOnlyKVCommandDeduplicationIT for KV ledgers that use the ap…
cae69a6 
…pend-only schema
@nicu-da nicu-da requested review from a team as code owners September 10, 2021 12:54
Base automatically changed from nicuda/kvl-1098/use_max_deduplication_duration to main September 10, 2021 16:05
@nicu-da
Merge branch 'main' into nicuda/kvl-1098/conformance_test_max_dedupli…
11c58a2 
…cation

# Conflicts:
#	ledger/ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/infrastructure/deduplication/CommandDeduplicationBase.scala
#	ledger/ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/suites/KVCommandDeduplicationIT.scala
#	ledger/ledger-on-memory/BUILD.bazel
#	ledger/ledger-on-sql/BUILD.bazel
#	ledger/sandbox/BUILD.bazel
hubert-da
hubert-da reviewed Sep 10, 2021
View reviewed changes
...ala/com/daml/ledger/api/testtool/infrastructure/deduplication/CommandDeduplicationBase.scala Outdated Show resolved Hide resolved
...ala/com/daml/ledger/api/testtool/infrastructure/deduplication/CommandDeduplicationBase.scala Outdated
.map(assertGrpcError(_, statusCode, None))
}

protected def requestIsSubmittedAndHasCompletion(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this may become private

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I would consider naming the functions like submitRequestAnd..., for example submitRequestAndAssertCompletion. Or maybe it would be worth splitting up to preserve the SRP(?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would split it but they're almost always used together so it's quite handy. Agree on the name change, will rename

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really assert the completion though, so I would rename it to submitRequestAndFindCompletion

...ala/com/daml/ledger/api/testtool/infrastructure/deduplication/CommandDeduplicationBase.scala Outdated Show resolved Hide resolved
miklos-da
miklos-da approved these changes Sep 10, 2021
View reviewed changes
Copy link
Contributor

@miklos-da miklos-da left a comment

Choose a reason for hiding this comment

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

Approving in the interest of increasing test coverage.

...a/com/daml/ledger/api/testtool/infrastructure/deduplication/KVCommandDeduplicationBase.scala Outdated Show resolved Hide resolved
ledger/ledger-api-test-tool/src/main/scala/com/daml/ledger/api/testtool/tests/Tests.scala Show resolved Hide resolved
fabiotudone-da
fabiotudone-da approved these changes Sep 13, 2021
View reviewed changes
...ala/com/daml/ledger/api/testtool/infrastructure/deduplication/CommandDeduplicationBase.scala
Comment on lines +307 to +309
)(request: SubmitRequest, party: Party)(implicit ec: ExecutionContext): Future[Completion] = {
requestHasCompletionWithStatusCode(ledger)(request, party, Code.OK)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: unneeded braces

...ala/com/daml/ledger/api/testtool/infrastructure/deduplication/CommandDeduplicationBase.scala
Comment on lines +315 to +323
): Future[Completion] = {
submitRequestAndFindCompletion(ledger)(request, party).map(completion => {
assert(
completion.getStatus.code == code.value(),
s"Expecting completion with status code $code but completion has status ${completion.status}",
)
completion
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: unneeded braces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing another cleanup in a PR that I'm about to open so I'll change all these braces there.

...ala/com/daml/ledger/api/testtool/infrastructure/deduplication/CommandDeduplicationBase.scala
Comment on lines +327 to +332
)(request: SubmitRequest, statusCode: Code)(implicit ec: ExecutionContext): Future[Unit] = {
ledger
.submit(request)
.mustFail(s"Request expected to fail with status $statusCode")
.map(assertGrpcError(_, statusCode, None))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: unneeded braces

...pi-test-tool/src/main/scala/com/daml/ledger/api/testtool/suites/CommandDeduplicationIT.scala
Comment on lines +26 to +27
/** Assertions for append-only schema are important for KV ledgers
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Assertions for append-only schema are important for KV ledgers
*/
/** Enables additional assertions available when running with the append-only schema.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e., I'd avoid a ledger-specific reference in the general test logic. Maybe you can add a comment about KV in the KV subclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will address that in a follow-up PR to clean-up the tests

@nicu-da nicu-da merged commit 8a9d19a into main Sep 13, 2021
@nicu-da nicu-da deleted the nicuda/kvl-1098/conformance_test_max_deduplication branch September 13, 2021 13:48
azure-pipelines bot pushed a commit that referenced this pull request Sep 15, 2021
release 1.17.0-snapshot.20210915.7832.0.38227a8e
edadd33 
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.

@cocreature is in charge of this release.

Commit log:
```
38227a8 [Ledger API error codes] ErrorCode enrichments [DPP-591] (#10874)
e7c443a enable json index for all fields that are queried with JSON_EXISTS (#10658)
6c1c02a document complete authorized auth0 setup (#10881)
e4230dc Do not drop generated `submissionId`s in `GrpcCommandService` [KVL-1104] (#10882)
b4750a4 trigger reach auth on internal network (#10844)
7908083 add Auth0 support to create-daml-app (#10673)
b86490c Add @adriaanm-da to the release rotation (#10872)
9e918c3 Update trigger-service docs to use --dar option in the corresponding example (#10877)
49a9556 [docs] Fix minor typo in doc of exerciseByKey in TS. (#10863)
f7c07ea interfaces: scala protobuf encoder (#10878)
be4e064 Ledger API Test Tool: support `--additional` tests [KVL-1100] (#10829)
97e14de [Ledger API error codes] ErrorCode interfaces and generator [DPP-591] (#10836)
6dcdaa4 [DPP-589] Add CLI flag to select minimum enabled TLS version (#10854)
1fc58d9 Navigator customviews highlight and choices button, apply custom theme on the login screen (#10859)
6faddc9 Update Daml Documentation to reflect command deduplication related changes [KVL-1094] (#10852)
7c29eee Cleanup normalize from svalue (#10873)
053f22a Convert SValue to Value, and normalize, in a single code pass. (#10828)
37a1cb2 compatibility-tests - Exclude CommandDeduplicationIT from running for existing 1.17 snapshots (#10866)
dfae9f6 Command deduplication - better support for different deduplication modes in conformance tests [KVL-1099] (#10864)
6f151e2 save kibana exports (#10861)
99f0362 [JSON-API] drop package token doc changes (#10865)
b50bb8e Populate `definite_answer` in `ApiException` [KVL-1004] (#10832)
a471225 LF: Add missing collision check for type synonyms (#10841)
1e1c452 LF: drop ad-hoc FrontStack builders (#10839)
8f5b4fa interfaces: protobuf encoder haskell side (#10850)
63f6678 ParticipantPruningIT divulgence test fixes to avoid flakiness on canton (#10860)
8a9d19a Command deduplication - KV conformance test for usage of max deduplication duration [KVL-1098] (#10846)
24fff88 LF: Simplify TransactionBuilder (#10753)
9a4c9df Implement LF desugaring of interface definitions (#10834)
2aaf601 interfaces: protobuf decoder haskell side (#10849)
6dc769b interfaces: lf typechecker implementation (#10843)
d9178d2 Clarify version usage in test tool exclusion docs (#10858)
c113954 Clarify docs for test tool exclusions (#10855)
8c9edd8 es cluster tweaks (#10853)
842c5b1 Drop early access notice from profiler docs (#10856)
7c47aca Improvements to wording in ledger-api protobuf docs (#10851)
cff0358 ledger-api: Remove unimplemented fields [KVL-1094] (#10822)
dcec6ea kvutils: Populate `definite_answer` in rejections [KVL-1004] (#10801)
1c4f173 Command deduplication - kvutils - Always use max deduplication duration as deduplication period [KVL-1098] (#10824)
567fe43 tweak trigger-service docs (#10845)
fb5ab5d setvar doesn't like new lines in assignment, refactor (#10842)
7225c04 [docs] Replace AdoptOpenJDK suggestion by Adoptium (#10837)
6a9c8a6 release 1.17.0-snapshot.20210910.7786.0.976ca400 (#10838)
6ed2124 LF: clean up useless version tests. (#10833)
85f6f36 Modify the name of the secrets-url CLI flag to tls-secrets-url [DPP-604] (#10840)
d809fd9 [JSON-API] surrogate template id cache (#10806)
```
Changelog:
```
- The Trigger Service can now accept separate `--auth-internal` and
  `--auth-external` CLI arguments, where `--auth-internal` is the
  address used by the Trigger Service to reach the Auth Middleware
  directly, and `--auth-external` is the address the Trigger Service uses
  in generated URLs sent back to the client. The `--auth` option remains
  and keeps working as before, setting both internal and external
  addresses to the same given value.
- The `create-daml-app` template now includes support for a third
  authentication scheme (in addition to the existing "dev mode" and Daml
  Hub support): Auth0.
Sandbox: Add CLI flag to select minimum enabled TLS version for ledger API server.
- [Navigator] The currently selected custom view is now highlighted on the sidebar

kvutils - committer side deduplication always uses max_deduplication_duration + min_skew as a deduplication period for all the requests.
Modify the name of the secrets-url CLI flag to tls-secrets-url.
```

CHANGELOG_BEGIN
CHANGELOG_END
akrmn added a commit that referenced this pull request Sep 15, 2021
@akrmn
release 1.17.0-snapshot.20210915.7842.0.b5648c0e
87fbad5 
Manual release process. @akrmn is in charge of this release.

Commit log:
```
b5648c0 Make `CommandTracker` distinguish submissions of the same command using `submissionId` [KVL-1104] (#10868)
b4328b3 ledger-api-test-tool - Add conformance test for parallel command deduplication using CommandSubmissionService [KVL-1099] (#10869)
0c32e3b Fix Parallel Indexer initialization issue [DPP-542] (#10889)
b3e4975 Chore slow migration error removal (#10886)
e4cce53 Create a new grpc exception for each duplicate result [KVL-1099] (#10887)
a939594 Sandbox on H2 - performance improvements for the append-only schema [DPP-600] (#10888)
9a1a101 Increase timeout for heavy tests in ParticipantPruningIT (#10894)
9093c6c Improve wording for the active contracts service description (#10880)
c12f546 Document #10780 (#10781)
5814f6a update NOTICES file (#10893)
38227a8 [Ledger API error codes] ErrorCode enrichments [DPP-591] (#10874)
e7c443a enable json index for all fields that are queried with JSON_EXISTS (#10658)
6c1c02a document complete authorized auth0 setup (#10881)
e4230dc Do not drop generated `submissionId`s in `GrpcCommandService` [KVL-1104] (#10882)
b4750a4 trigger reach auth on internal network (#10844)
7908083 add Auth0 support to create-daml-app (#10673)
b86490c Add @adriaanm-da to the release rotation (#10872)
9e918c3 Update trigger-service docs to use --dar option in the corresponding example (#10877)
49a9556 [docs] Fix minor typo in doc of exerciseByKey in TS. (#10863)
f7c07ea interfaces: scala protobuf encoder (#10878)
be4e064 Ledger API Test Tool: support `--additional` tests [KVL-1100] (#10829)
97e14de [Ledger API error codes] ErrorCode interfaces and generator [DPP-591] (#10836)
6dcdaa4 [DPP-589] Add CLI flag to select minimum enabled TLS version (#10854)
1fc58d9 Navigator customviews highlight and choices button, apply custom theme on the login screen (#10859)
6faddc9 Update Daml Documentation to reflect command deduplication related changes [KVL-1094] (#10852)
7c29eee Cleanup normalize from svalue (#10873)
053f22a Convert SValue to Value, and normalize, in a single code pass. (#10828)
37a1cb2 compatibility-tests - Exclude CommandDeduplicationIT from running for existing 1.17 snapshots (#10866)
dfae9f6 Command deduplication - better support for different deduplication modes in conformance tests [KVL-1099] (#10864)
6f151e2 save kibana exports (#10861)
99f0362 [JSON-API] drop package token doc changes (#10865)
b50bb8e Populate `definite_answer` in `ApiException` [KVL-1004] (#10832)
a471225 LF: Add missing collision check for type synonyms (#10841)
1e1c452 LF: drop ad-hoc FrontStack builders (#10839)
8f5b4fa interfaces: protobuf encoder haskell side (#10850)
63f6678 ParticipantPruningIT divulgence test fixes to avoid flakiness on canton (#10860)
8a9d19a Command deduplication - KV conformance test for usage of max deduplication duration [KVL-1098] (#10846)
24fff88 LF: Simplify TransactionBuilder (#10753)
9a4c9df Implement LF desugaring of interface definitions (#10834)
2aaf601 interfaces: protobuf decoder haskell side (#10849)
6dc769b interfaces: lf typechecker implementation (#10843)
d9178d2 Clarify version usage in test tool exclusion docs (#10858)
c113954 Clarify docs for test tool exclusions (#10855)
8c9edd8 es cluster tweaks (#10853)
842c5b1 Drop early access notice from profiler docs (#10856)
7c47aca Improvements to wording in ledger-api protobuf docs (#10851)
cff0358 ledger-api: Remove unimplemented fields [KVL-1094] (#10822)
dcec6ea kvutils: Populate `definite_answer` in rejections [KVL-1004] (#10801)
1c4f173 Command deduplication - kvutils - Always use max deduplication duration as deduplication period [KVL-1098] (#10824)
567fe43 tweak trigger-service docs (#10845)
fb5ab5d setvar doesn't like new lines in assignment, refactor (#10842)
7225c04 [docs] Replace AdoptOpenJDK suggestion by Adoptium (#10837)
6a9c8a6 release 1.17.0-snapshot.20210910.7786.0.976ca400 (#10838)
6ed2124 LF: clean up useless version tests. (#10833)
85f6f36 Modify the name of the secrets-url CLI flag to tls-secrets-url [DPP-604] (#10840)
d809fd9 [JSON-API] surrogate template id cache (#10806)
```
Changelog:
```

- [Sandbox] - Added a CLI parameter for configuring the number of connections in the database connection pool used for serving ledger API requests
[Docs] Improved description of the purpose and usage of the active contracts service
[Docs/JSON API] documented 256B limitation of Oracle query store
- The Trigger Service can now accept separate `--auth-internal` and
  `--auth-external` CLI arguments, where `--auth-internal` is the
  address used by the Trigger Service to reach the Auth Middleware
  directly, and `--auth-external` is the address the Trigger Service uses
  in generated URLs sent back to the client. The `--auth` option remains
  and keeps working as before, setting both internal and external
  addresses to the same given value.
- The `create-daml-app` template now includes support for a third
  authentication scheme (in addition to the existing "dev mode" and Daml
  Hub support): Auth0.
Sandbox: Add CLI flag to select minimum enabled TLS version for ledger API server.
- [Navigator] The currently selected custom view is now highlighted on the sidebar

kvutils - committer side deduplication always uses max_deduplication_duration + min_skew as a deduplication period for all the requests.
Modify the name of the secrets-url CLI flag to tls-secrets-url.
```

changelog_begin
changelog_end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@miklos-da miklos-da miklos-da approved these changes

@cocreature cocreature Awaiting requested review from cocreature

@stefanobaghino-da stefanobaghino-da Awaiting requested review from stefanobaghino-da

@hubert-da hubert-da Awaiting requested review from hubert-da

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

Successfully merging this pull request may close these issues.
4 participants
@nicu-da @miklos-da @fabiotudone-da @hubert-da