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

kvutils - Use the ledger configured time for command dedup [KVL-1149] #11239

Merged
merged 2 commits into from
Oct 15, 2021
Merged

kvutils - Use the ledger configured time for command dedup [KVL-1149] #11239

merged 2 commits into from
Oct 15, 2021

Conversation

nicu-da
Copy link
Contributor

@nicu-da nicu-da commented Oct 13, 2021

CHANGELOG_BEGIN
kvutils - Command deduplication uses the configured time model instead of always using wall clock
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
  • If you mean to change the status of a component, please make sure you keep the Component Status page up to date.

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
Use the ledger configured time for command dedup in normal validation…
4bc290d 
… mode instead of always using wall-clock

CHANGELOG_BEGIN
kvutils - Command deduplication uses the configured time model instead of always using wall clock
CHANGELOG_END
@nicu-da nicu-da requested a review from a team as a code owner October 13, 2021 15:24
fabiotudone-da
fabiotudone-da previously approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@fabiotudone-da fabiotudone-da left a comment

Choose a reason for hiding this comment

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

Simpler, too 👍🏼

@fabiotudone-da
Copy link
Contributor

fabiotudone-da commented Oct 14, 2021 •
edited
Loading

@nicu-da Do you plan to remove the participant dedup enablement hack in sandbox as a separate PR? EDIT: Actually, doing so here would provide a nice test for this.

@nicu-da
Copy link
Contributor Author

nicu-da commented Oct 14, 2021

@nicu-da Do you plan to remove the participant dedup enablement hack in sandbox as a separate PR? EDIT: Actually, doing so here would provide a nice test for this.

@fabiotudone-da Yes, I was just in the process of figuring that out. Will do it in a separate PR

@fabiotudone-da
Copy link
Contributor

@nicu-da Do you plan to remove the participant dedup enablement hack in sandbox as a separate PR? EDIT: Actually, doing so here would provide a nice test for this.

@fabiotudone-da Yes, I was just in the process of figuring that out. Will do it in a separate PR

If it doesn't work we'd be merging a broken change though. Or is this change tested by something else too?

@nicu-da
Copy link
Contributor Author

nicu-da commented Oct 14, 2021

@nicu-da Do you plan to remove the participant dedup enablement hack in sandbox as a separate PR? EDIT: Actually, doing so here would provide a nice test for this.

@fabiotudone-da Yes, I was just in the process of figuring that out. Will do it in a separate PR

If it doesn't work we'd be merging a broken change though. Or is this change tested by something else too?

@fabiotudone-da Did the change locally and tested it out. Command deduplication works as expected (we won't be able to enable all the conformance tests because we don't advance the clock)

Though this change is validated by the existing conformance tests.

@nicu-da nicu-da requested review from a user, miklos-da and hubert-da October 14, 2021 14:41
hubert-da
hubert-da requested changes Oct 14, 2021
View reviewed changes
Copy link
Collaborator

@hubert-da hubert-da left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Other than that LGTM, great stuff

...a/com/daml/ledger/participant/state/kvutils/committer/transaction/TransactionCommitter.scala
@@ -56,7 +56,6 @@ private[kvutils] class TransactionCommitter(
defaultConfig: Configuration,
engine: Engine,
override protected val metrics: Metrics,
inStaticTimeMode: Boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also remove/tune the documentation above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, good catch

@nicu-da nicu-da requested a review from hubert-da October 14, 2021 15:08
ghost
ghost approved these changes Oct 14, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

If your local testing went well, I'm happy. Lets get those tests in order next (perhaps with a --static-time flag for the Test Tool).

@nicu-da nicu-da changed the title Use the ledger configured time for command dedup [KVL-1149] kvutils - Use the ledger configured time for command dedup [KVL-1149] Oct 15, 2021
@nicu-da nicu-da merged commit 68a93cf into main Oct 15, 2021
@nicu-da nicu-da deleted the nicuda/kvl-1149/dedup_static_time branch October 15, 2021 12:56
azure-pipelines bot pushed a commit that referenced this pull request Oct 20, 2021
release 1.18.0-snapshot.20211019.8113.0.8ff347d8
c3ee08e 
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.

@sofiafaro-da is in charge of this release.

Commit log:
```
8ff347d Expand type synonyms lazily (#11282)
2fd200f KV: support contextual logging in KeyValueConsumption [KVL-1143] (#11288)
f4df1cc converting server errors to proper client errors (#11184)
525e4ce Add GrpcStatus.toProto(JavaStatus) -> ScalaStatus conversion [KVL-1143] (#11292)
9b00a1a Rotate release rotation (#11291)
dd09c38 Upgrade rules-nodejs (#11290)
87f1418 [Short] Remove unnecessary traits from ApiSubmissionServiceSpec (#11254)
6b65a72 Merge npm install steps in create-daml-app tests (#11287)
a9f6afb kvutils: Rename `VersionedOffset` to `KVOffset`. (#11286)
98cf8d8 KV: introduce v2 error codes behind a CLI switch [KVL-1140] (#11224)
46f6877 Increase time model skew limits (#11273)
8f94cff kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffsetMutator`. [KVL-1154] (#11277)
81fde97 Bazel: Call `_wrap_rule` directly when building the Scala REPL rule. (#11279)
edb2b04 Document scaladoc usage in Bazel (#11278)
6255837 Data dependencies support for reexports (#11147)
2fc7490 [Self-service error codes] Adapt error factories [DPP-656] (#11270)
b1a6b11 ledger-api-test-tool: Add static time awareness [KVL-1156] (#11266)
243c120 Add an LF typechecking benchmark (#11276)
2db8c13 Test authorization within Exercise choice. (#11246)
ec58ed6 Typecheck nested LF type applications more efficiently (#11253)
a940016 Add debugRaw corresponding to traceM in Haskell (#11259)
a988579 Fix Scala repl targets (#11272)
e4808f6 Extract UtilLF module from daml-lf-conversion to its own library (#11263)
38712c0 sandbox-next - Disable participant command deduplication [KVL-1156] (#11264)
82fa229 Add daml-util-ghc lib (#11260)
68a93cf kvutils - Use the ledger configured time for command dedup [KVL-1149] (#11239)
c8cede0 Bump startup time in create-daml-app tests (#11261)
4fac87f Bump the schema version for the JSON API (#11252)
5507670 update NOTICES file (#11256)
0063b10 Retry faster during startup (#11255)
04feb40 Improve reset service tests (#11240)
b3375fd update compat versions for 1.18.0-snapshot.20211013.8071.0.514e8b50 (#11237)
9ed1eb3 Address security notice on `ansi-regex` (#11243)
387d0e8 Make compatibility migrations `eternal` (#11242)
be899b3 Extract rejection_reason.proto from daml_kvutils.proto [KVL-1090] (#11235)
a1d94e1 LF: Create special case class for VersionedContractInstance (#11127)
b738988 Release 1.18 snapshot (#11234)
3c26852 LF: Use template Id in exercise node of fixed choice. (#11229)
139b6f3 CI: Set `PROJ_DIR` inside the bash lib, not outside. (#11236)
ce64cb2 interfaces: Do some TODOs (#11231)
7a88c7d trigger-service: dev-mode-unsafe flag (#11233)
909a1bf [DPP-417][DDP-612] Adapt ApiSubmissionService to support V2 error codes (#11052)
```
Changelog:
```
- [JSON API] Several kinds of gRPC server errors are now reported with
  associated HTTP statuses; for example, a Daml-LF interpreter error now
  returns a 400 instead of a 500, and an exercise on an archived contract
  returns a 409 Conflict instead of a 500.  Errors internal to JSON API
  (e.g. internal assertion failures) are no longer detailed in the HTTP
  response; their details are only logged.
  See `issue #11184 <https://github.com/digital-asset/daml/pull/11184>`__.

- [Daml Stdlib] Add `debugRaw` as a convenience wrapper around
  `traceRaw` when used inside a do-block. `debugRaw` compares to
  `debug` like `traceRaw` compares to `trace` meaning it expects a
  `Text` instead of calling `show` on an expression.

sandbox-next - Disable participant command deduplication

kvutils - Command deduplication uses the configured time model (static/wall) and not running always using wall-clock
[JSON API] Solving a bug that could cause the JSON API to return
correct result if a contract with the same key is observed twice
required a schema change. The JSON API data needs to be dropped
and the query store needs to reset. If you are migrating from a
previous version, either reset your database manually or start
the HTTP JSON API with one of the options that regenerate the
schema (`create-only`, `create-if-needed-and-start`, `create-and-start`).
```

CHANGELOG_BEGIN
CHANGELOG_END
sofiafaro-da pushed a commit that referenced this pull request Oct 20, 2021
@azure-pipelines
release 1.18.0-snapshot.20211019.8113.0.8ff347d8 (#11300)
f315a90 
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.

@sofiafaro-da is in charge of this release.

Commit log:
```
8ff347d Expand type synonyms lazily (#11282)
2fd200f KV: support contextual logging in KeyValueConsumption [KVL-1143] (#11288)
f4df1cc converting server errors to proper client errors (#11184)
525e4ce Add GrpcStatus.toProto(JavaStatus) -> ScalaStatus conversion [KVL-1143] (#11292)
9b00a1a Rotate release rotation (#11291)
dd09c38 Upgrade rules-nodejs (#11290)
87f1418 [Short] Remove unnecessary traits from ApiSubmissionServiceSpec (#11254)
6b65a72 Merge npm install steps in create-daml-app tests (#11287)
a9f6afb kvutils: Rename `VersionedOffset` to `KVOffset`. (#11286)
98cf8d8 KV: introduce v2 error codes behind a CLI switch [KVL-1140] (#11224)
46f6877 Increase time model skew limits (#11273)
8f94cff kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffsetMutator`. [KVL-1154] (#11277)
81fde97 Bazel: Call `_wrap_rule` directly when building the Scala REPL rule. (#11279)
edb2b04 Document scaladoc usage in Bazel (#11278)
6255837 Data dependencies support for reexports (#11147)
2fc7490 [Self-service error codes] Adapt error factories [DPP-656] (#11270)
b1a6b11 ledger-api-test-tool: Add static time awareness [KVL-1156] (#11266)
243c120 Add an LF typechecking benchmark (#11276)
2db8c13 Test authorization within Exercise choice. (#11246)
ec58ed6 Typecheck nested LF type applications more efficiently (#11253)
a940016 Add debugRaw corresponding to traceM in Haskell (#11259)
a988579 Fix Scala repl targets (#11272)
e4808f6 Extract UtilLF module from daml-lf-conversion to its own library (#11263)
38712c0 sandbox-next - Disable participant command deduplication [KVL-1156] (#11264)
82fa229 Add daml-util-ghc lib (#11260)
68a93cf kvutils - Use the ledger configured time for command dedup [KVL-1149] (#11239)
c8cede0 Bump startup time in create-daml-app tests (#11261)
4fac87f Bump the schema version for the JSON API (#11252)
5507670 update NOTICES file (#11256)
0063b10 Retry faster during startup (#11255)
04feb40 Improve reset service tests (#11240)
b3375fd update compat versions for 1.18.0-snapshot.20211013.8071.0.514e8b50 (#11237)
9ed1eb3 Address security notice on `ansi-regex` (#11243)
387d0e8 Make compatibility migrations `eternal` (#11242)
be899b3 Extract rejection_reason.proto from daml_kvutils.proto [KVL-1090] (#11235)
a1d94e1 LF: Create special case class for VersionedContractInstance (#11127)
b738988 Release 1.18 snapshot (#11234)
3c26852 LF: Use template Id in exercise node of fixed choice. (#11229)
139b6f3 CI: Set `PROJ_DIR` inside the bash lib, not outside. (#11236)
ce64cb2 interfaces: Do some TODOs (#11231)
7a88c7d trigger-service: dev-mode-unsafe flag (#11233)
909a1bf [DPP-417][DDP-612] Adapt ApiSubmissionService to support V2 error codes (#11052)
```
Changelog:
```
- [JSON API] Several kinds of gRPC server errors are now reported with
  associated HTTP statuses; for example, a Daml-LF interpreter error now
  returns a 400 instead of a 500, and an exercise on an archived contract
  returns a 409 Conflict instead of a 500.  Errors internal to JSON API
  (e.g. internal assertion failures) are no longer detailed in the HTTP
  response; their details are only logged.
  See `issue #11184 <https://github.com/digital-asset/daml/pull/11184>`__.

- [Daml Stdlib] Add `debugRaw` as a convenience wrapper around
  `traceRaw` when used inside a do-block. `debugRaw` compares to
  `debug` like `traceRaw` compares to `trace` meaning it expects a
  `Text` instead of calling `show` on an expression.

sandbox-next - Disable participant command deduplication

kvutils - Command deduplication uses the configured time model (static/wall) and not running always using wall-clock
[JSON API] Solving a bug that could cause the JSON API to return
correct result if a contract with the same key is observed twice
required a schema change. The JSON API data needs to be dropped
and the query store needs to reset. If you are migrating from a
previous version, either reset your database manually or start
the HTTP JSON API with one of the options that regenerate the
schema (`create-only`, `create-if-needed-and-start`, `create-and-start`).
```

CHANGELOG_BEGIN
CHANGELOG_END

Co-authored-by: Azure Pipelines Daml Build <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

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

@miklos-da miklos-da Awaiting requested review from miklos-da

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

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