kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffset`. [KVL-1154] #11277

ghost · 2021-10-18T10:45:12Z

We had a few issues with OffsetBuilder:

We were always using OffsetBuilder.highestIndex to extract the highest index, which was crashing if the offset version was non-zero.
We used OffsetBuilder.dropLowestIndex and OffsetBuilder.setLowestIndex, which also crashed if the offset version was non-zero.
There were a bunch of unused methods on OffsetBuilder and VersionedOffsetBuilder.

To remedy these, I have made the following changes:

Removed the unused methods.
Switched to VersionedOffsetBuilder instead of OffsetBuilder for constructing the offset and extracting the highest index.
Introduced VersionedOffset, which constructs the offset, extracts its parts, and zeroes and sets the lowest index while taking the version into account.
Removed OffsetBuilder.
Randomized the offset version in KV participant state test cases to ensure that all of the above works well.

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.

Doesn't do anything.

This takes care of the last usages of `OffsetBuilder`, which were to modify the lowest component of the offset. CHANGELOG_BEGIN CHANGELOG_END

To make sure we don't use a hard-coded offset version anywhere.

hubert-da

LGTM, thanks!

fabiotudone-da · 2021-10-18T12:46:04Z

...vutils/src/main/scala/com/daml/ledger/participant/state/kvutils/VersionedOffsetMutator.scala

+
+/** Modifies specific offset fields while preserving all other fields.
+  */
+object VersionedOffsetMutator {


Suggested change

object VersionedOffsetMutator {

object OffsetMutator {

But why not having them rather as instance methods of VersionedOffset?

Yeah, that would be nicer, right? 🙂

I will do that.

Is the VersionedOffsetMutator used anywhere now? I couldn't find any usages.

No, I moved the code into VersionedOffset as Fabio suggested.

Why keep the file then? Sorry if I'm not seeing anything obvious :)

Ugh, I thought I deleted it. Thank you for pointing this out!

Addressed in #11286.

…offset-builder

The `VersionedOffsetBuilder` is still useful as it remembers the version, meaning we only need to specify it once.

Not all ledgers may allow this, and we still want to run the tests.

kamil-da · 2021-10-18T15:26:32Z

...vutils/src/main/scala/com/daml/ledger/participant/state/kvutils/VersionedOffsetMutator.scala

+
+/** Modifies specific offset fields while preserving all other fields.
+  */
+object VersionedOffsetMutator {


Is the VersionedOffsetMutator used anywhere now? I couldn't find any usages.

kamil-da · 2021-10-18T15:26:55Z

ledger/indexer-benchmark/src/app/scala/ledger/indexerbenchmark/Main.scala

@@ -38,14 +38,15 @@ object Main {
    }
    val importer = ProtobufBasedLedgerDataImporter(path)

+    val offsetBuilder = new VersionedOffsetBuilder(0)


Would it be possible to introduce a constant for this version number? It's hardcoded in multiple places.

It is only hardcoded in test code (and test-adjacent code, such as integrity checking and benchmarking).

In production, this value should most definitely not be hardcoded, and so I would prefer to not hardcode it anywhere, to ensure that any user thinks about it.

That seems sensible, thank you!

@sofiafaro-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. @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

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]>

SamirTalwar added 12 commits October 18, 2021 11:46

kvutils: Remove VersionedOffsetBuilder.apply.

e870f7d

Doesn't do anything.

ledger-on-memory: Use VersionedOffsetBuilder.

225cedc

indexer-benchmark: Use VersionedOffsetBuilder.

a5c3c49

ledger-on-sql: Use VersionedOffsetBuilder.

3dd21ee

kvutils: Use VersionedOffsetBuilder in tests.

59486d7

kvutils: Create a case class for VersionedOffsetBuilder#split.

46696af

kvutils: Delete unused methods from the offset builders.

44eea4f

kvutils: Use Bytes#startWith to check the offset version.

e5a43e7

kvutils: Move offset splitting into VersionedOffset.

058f416

kvutils: Extract out versioned offset generators.

3961aa1

kvutils: Replace OffsetBuilder with VersionedOffsetMutator.

1e31a73

This takes care of the last usages of `OffsetBuilder`, which were to modify the lowest component of the offset. CHANGELOG_BEGIN CHANGELOG_END

kvutils: Randomize the offset version in testing.

d9e4794

To make sure we don't use a hard-coded offset version anywhere.

ghost requested review from a team as code owners October 18, 2021 10:45

kvutils: Random.between is not available in Scala 2.12.

52611f1

hubert-da approved these changes Oct 18, 2021

View reviewed changes

fabiotudone-da approved these changes Oct 18, 2021

View reviewed changes

SamirTalwar added 3 commits October 18, 2021 15:21

Merge remote-tracking branch 'origin/main' into samir/kvutils/remove-…

a68b990

…offset-builder

kvutils: Move offset mutation methods to VersionedOffset.

68e91e1

kvutils: Move the versioned offset construction into VersionedOffset.

6ce9ca6

The `VersionedOffsetBuilder` is still useful as it remembers the version, meaning we only need to specify it once.

ghost closed this Oct 18, 2021

ghost reopened this Oct 18, 2021

ghost enabled auto-merge (squash) October 18, 2021 14:16

kvutils: Only vary the offset version in ledger-on-memory tests.

6152a42

Not all ledgers may allow this, and we still want to run the tests.

kamil-da reviewed Oct 18, 2021

View reviewed changes

ghost changed the title ~~kvutils: Use VersionedOffsetBuilder where possible, and introduce VersionedOffsetMutator. [KVL-1154]~~ kvutils: Use VersionedOffsetBuilder where possible, and introduce VersionedOffset. [KVL-1154] Oct 18, 2021

ghost merged commit 8f94cff into main Oct 18, 2021

ghost deleted the samir/kvutils/remove-offset-builder branch October 18, 2021 16:50

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffset`. [KVL-1154] #11277

kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffset`. [KVL-1154] #11277

ghost commented Oct 18, 2021 •

edited by ghost

Loading

hubert-da left a comment

fabiotudone-da Oct 18, 2021

ghost Oct 18, 2021

ghost Oct 18, 2021

kamil-da Oct 18, 2021

ghost Oct 18, 2021

kamil-da Oct 18, 2021

ghost Oct 19, 2021

kamil-da Oct 18, 2021

kamil-da Oct 18, 2021

ghost Oct 18, 2021

kamil-da Oct 18, 2021

kvutils: Use VersionedOffsetBuilder where possible, and introduce VersionedOffset. [KVL-1154] #11277

kvutils: Use VersionedOffsetBuilder where possible, and introduce VersionedOffset. [KVL-1154] #11277

Conversation

ghost commented Oct 18, 2021 • edited by ghost Loading

Pull Request Checklist

hubert-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffset`. [KVL-1154] #11277

kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffset`. [KVL-1154] #11277

ghost commented Oct 18, 2021 •

edited by ghost

Loading