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

Save the submission time in the command deduplication value [KVL-1173] #11509

Merged
merged 3 commits into from
Nov 3, 2021
Merged

Save the submission time in the command deduplication value [KVL-1173] #11509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a0e7a8
Save the submission time in the command deduplication value within th…
nicu-da Nov 2, 2021
c8721de
Apply suggestions from code review
nicu-da Nov 3, 2021
72c4e28
Revert removal of field
nicu-da Nov 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ...ate/kvutils/src/main/protobuf/com/daml/ledger/participant/state/kvutils/store/state.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,12 @@ message DamlCommandDedupKey {

message DamlCommandDedupValue {
reserved 1; // was record_time
// the time until when future commands with the same
// deduplication key will be rejected due to a duplicate submission
// The time until when future commands with the same
// deduplication key will be rejected due to a duplicate submission.
google.protobuf.Timestamp deduplicated_until = 2;
// The submission time of the initial command.
// We use submission time because record time is not available during pre-execution.
google.protobuf.Timestamp submission_time = 3;
}

message DamlSubmissionDedupKey {
Expand Down
13 changes: 7 additions & 6 deletions ...om/daml/ledger/participant/state/kvutils/committer/transaction/TransactionCommitter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,13 +328,14 @@ private[kvutils] class TransactionCommitter(
if (!transactionEntry.submitterInfo.hasDeduplicationDuration) {
throw Err.InvalidSubmission("Deduplication duration is not set.")
}
val commandDedupBuilder = DamlCommandDedupValue.newBuilder.setDeduplicatedUntil(
Conversions.buildTimestamp(
transactionEntry.submissionTime
.add(parseDuration(transactionEntry.submitterInfo.getDeduplicationDuration))
.add(config.timeModel.minSkew)
)
val deduplicateUntil = Conversions.buildTimestamp(
transactionEntry.submissionTime
.add(parseDuration(transactionEntry.submitterInfo.getDeduplicationDuration))
.add(config.timeModel.minSkew)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment about why we're using the min skew here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is taken from the design doc. I could add my understanding of it but I would prefer to leave it as is for now, and add a proper comment in the follow up PR which changes this logic

)
val commandDedupBuilder = DamlCommandDedupValue.newBuilder
.setDeduplicatedUntil(deduplicateUntil)
.setSubmissionTime(Conversions.buildTimestamp(transactionEntry.submissionTime))
// Set a deduplication entry.
commitContext.set(
commandDedupKey(transactionEntry.submitterInfo),
Expand Down
17 changes: 16 additions & 1 deletion ...aml/ledger/participant/state/kvutils/committer/transaction/TransactionCommitterSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,9 @@ class TransactionCommitterSpec
"setting dedup context" should {
val deduplicateUntil = protobuf.Timestamp.newBuilder().setSeconds(30).build()
val submissionTime = protobuf.Timestamp.newBuilder().setSeconds(60).build()
val deduplicationDuration = time.Duration.ofSeconds(3)

"calculate deduplicate until based on deduplication duration" in {
val deduplicationDuration = time.Duration.ofSeconds(3)
val (context, transactionEntrySummary) =
buildContextAndTransaction(
submissionTime,
Expand All @@ -257,6 +257,21 @@ class TransactionCommitterSpec
.build()
}

"set the submission time in the committer context" in {
val (context, transactionEntrySummary) =
buildContextAndTransaction(
submissionTime,
_.setDeduplicationDuration(Conversions.buildDuration(deduplicationDuration)),
)
transactionCommitter.setDedupEntry(context, transactionEntrySummary)
context
.get(Conversions.commandDedupKey(transactionEntrySummary.submitterInfo))
.map(
_.getCommandDedup.getSubmissionTime
)
.value shouldBe submissionTime
}

"throw an error for unsupported deduplication periods" in {
forAll(
Table[DamlSubmitterInfo.Builder => DamlSubmitterInfo.Builder](
Expand Down