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

Enable pruning in the sandbox-classic when the append-only schema is used [DPP-567] #10708

Merged
merged 5 commits into from
Aug 31, 2021
Merged

Enable pruning in the sandbox-classic when the append-only schema is used [DPP-567] #10708

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2158fee
Enable pruning in the sandbox-classic when the append-only schema is …
kamil-da Aug 30, 2021
1fa4ad8
Added pruning conformance tests to the build definition of the sandbo…
kamil-da Aug 30, 2021
77c5920
Changed pruning IT time to the wall-clock time
kamil-da Aug 31, 2021
84f4c22
Added a comment explaining changes made
kamil-da Aug 31, 2021
8c494e9
Do not enable pruning when pruneAllDivulgedContracts option is enabled
kamil-da Aug 31, 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
17 changes: 17 additions & 0 deletions ledger/sandbox-classic/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,23 @@ server_conformance_test(
],
)

server_conformance_test(
name = "conformance-test-pruning-append-only",
lf_versions = [
"default",
],
server_args = [
"--static-time",
kamil-da marked this conversation as resolved.
Show resolved Hide resolved
"--contract-id-seeding=testing-weak",
"--enable-append-only-schema",
],
servers = ONLY_POSTGRES_SERVER,
test_tool_args = [
"--open-world",
"--include=ParticipantPruningIT",
],
)

server_conformance_test(
name = "conformance-test-wall-clock",
server_args = [
Expand Down
14 changes: 10 additions & 4 deletions ledger/sandbox-classic/src/main/scala/platform/sandbox/stores/LedgerBackedWriteService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ import io.grpc.Status

import scala.compat.java8.FutureConverters

private[stores] final class LedgerBackedWriteService(ledger: Ledger, timeProvider: TimeProvider)(
implicit loggingContext: LoggingContext
private[stores] final class LedgerBackedWriteService(
ledger: Ledger,
timeProvider: TimeProvider,
enablePruning: Boolean,
)(implicit
loggingContext: LoggingContext
) extends state.WriteService {

override def currentHealth(): HealthStatus = ledger.currentHealth()
Expand Down Expand Up @@ -92,13 +96,15 @@ private[stores] final class LedgerBackedWriteService(ledger: Ledger, timeProvide
FutureConverters.toJava(ledger.publishConfiguration(maxRecordTime, submissionId, config))
}

// WriteParticipantPruningService - not supported by sandbox-classic
override def prune(
pruneUpToInclusive: Offset,
submissionId: Ref.SubmissionId,
pruneAllDivulgedContracts: Boolean,
): CompletionStage[state.PruningResult] =
CompletableFuture.completedFuture(state.PruningResult.NotPruned(Status.UNIMPLEMENTED))
CompletableFuture.completedFuture {
if (enablePruning) state.PruningResult.ParticipantPruned
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not enabling pruning, just pretending to prune, no?
how is the conformance tests pass then?
am I missing something? 🤔

Copy link
Contributor Author

@kamil-da kamil-da Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand correctly the ApiParticipantPruningService performs pruning in two steps:

  1. pruning the write service
  2. pruning the index db

For the sandbox-classic they are the same thing, so returning ParticipantPruned for the step 1. enables the step two.
@mziolekda can you confirm that this is correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

yupp, makes sense, a comment would be nice to clarify this behavior in the write service

Copy link
Contributor

@tudor-da tudor-da Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

pruneAllDivulgedContracts is not meant to be supported for sandbox-classic because the backfiling lookup queries can start failing if used. Please return UNIMPLEMENTED if that flag is set. And please add a mention to the restriction in this method's doc/comments (e.g. Pruning of all divulged contracts is not supported on sandbox-classic)

else state.PruningResult.NotPruned(Status.UNIMPLEMENTED)
}

override def isApiDeduplicationEnabled: Boolean = true
}
19 changes: 16 additions & 3 deletions .../sandbox-classic/src/main/scala/platform/sandbox/stores/SandboxIndexAndWriteService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,14 @@ private[sandbox] object SandboxIndexAndWriteService {
validatePartyAllocation = validatePartyAllocation,
enableAppendOnlySchema = enableAppendOnlySchema,
enableCompression = enableCompression,
).flatMap(ledger => owner(MeteredLedger(ledger, metrics), participantId, timeProvider))
).flatMap(ledger =>
owner(
ledger = MeteredLedger(ledger, metrics),
participantId = participantId,
timeProvider = timeProvider,
enablePruning = enableAppendOnlySchema,
)
)

def inMemory(
name: LedgerName,
Expand All @@ -117,19 +124,25 @@ private[sandbox] object SandboxIndexAndWriteService {
templateStore,
ledgerEntries,
)
owner(MeteredLedger(ledger, metrics), participantId, timeProvider)
owner(
ledger = MeteredLedger(ledger, metrics),
participantId = participantId,
timeProvider = timeProvider,
enablePruning = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we enable pruning for in-memory ledger?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can document it in the PR header why this one is left out

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a corer case that we should not spend any time debating at this point. It would have to go through another story if we wanted that support.

Copy link
Contributor

Choose a reason for hiding this comment

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

fine by me. Just observed that there is no mention in the ticket or the PR regarding it.

)
}

private def owner(
ledger: Ledger,
participantId: Ref.ParticipantId,
timeProvider: TimeProvider,
enablePruning: Boolean,
)(implicit
mat: Materializer,
loggingContext: LoggingContext,
): ResourceOwner[IndexAndWriteService] = {
val indexSvc = new LedgerBackedIndexService(ledger, participantId)
val writeSvc = new LedgerBackedWriteService(ledger, timeProvider)
val writeSvc = new LedgerBackedWriteService(ledger, timeProvider, enablePruning)

for {
_ <- new HeartbeatScheduler(
Expand Down