Implement incremental acs snapshots #3733
Open
+1,717
−737
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rautenrieth-da
force-pushed
the
rautenrieth-da/incremental-acs-snapshots
branch
3 times, most recently
from
5f293c9 to
cf930a7
Compare
[ci] Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
[ci] Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
[ci] Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
[ci] Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
[ci] Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
[ci] Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
rautenrieth-da
force-pushed
the
rautenrieth-da/incremental-acs-snapshots
branch
from
cf930a7 to
97db5bc
Compare
Contributor
OriolMunoz-da
left a comment
OriolMunoz-da
left a comment
There was a problem hiding this comment.
thanks! looking good, the only really blocking comment is the advisory lock one
Also, for testing, should we run https://github.com/DACH-NY/canton-network-node/issues/17033 on cilr after a while? (and ideally before this is merged, for a sanity check...)
...in/resources/db/migration/canton-network/postgres/stable/V054__incremental_acs_snapshots.sql
Show resolved
Hide resolved
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Show resolved
Hide resolved
apps/common/src/main/scala/org/lfdecentralizedtrust/splice/store/UpdateHistory.scala
Show resolved
Hide resolved
.../src/main/scala/org/lfdecentralizedtrust/splice/scan/automation/AcsSnapshotTriggerBase.scala
Show resolved
Hide resolved
.../src/main/scala/org/lfdecentralizedtrust/splice/scan/automation/AcsSnapshotTriggerBase.scala
Show resolved
Hide resolved
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Show resolved
Hide resolved
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Show resolved
Hide resolved
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Show resolved
Hide resolved
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Show resolved
Hide resolved
.../src/test/scala/org/lfdecentralizedtrust/splice/scan/automation/AcsSnapshotTriggerTest.scala
Show resolved
Hide resolved
...in/resources/db/migration/canton-network/postgres/stable/V054__incremental_acs_snapshots.sql
Show resolved
Hide resolved
...in/resources/db/migration/canton-network/postgres/stable/V054__incremental_acs_snapshots.sql
Show resolved
Hide resolved
...in/resources/db/migration/canton-network/postgres/stable/V054__incremental_acs_snapshots.sql
Show resolved
Hide resolved
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Show resolved
Hide resolved
| val now = context.clock.now | ||
|
|
||
| // Next, make sure we don't process too little. | ||
| if (now.isBefore(updateUntil)) { |
Contributor
There was a problem hiding this comment.
Nice to have: a test checking this logic, or extract this in a separate method to easy unit test
| -- In production, we have a separate table for each scan instance so this | ||
| -- column will be the same for all rows. However, in tests we can have multiple | ||
| -- scan instances writing to the same table, so we need to include it. | ||
| history_id bigint not null references update_history_descriptors (id), |
Contributor
There was a problem hiding this comment.
This table and the backfil one should have a foreign key to the acs_incremental_snapshot table, as discussed
| targetRecordTime = now.minusSeconds(1800L), | ||
| ) | ||
| previousIncrementalSnapshot(snapshot) | ||
| updatesBetween(currentMigrationId, now.minusSeconds(6000L), now.minusSeconds(1L)) |
Contributor
There was a problem hiding this comment.
maybe some more constants in these tests as we talked about, if that makes it easier to understand.
Contributor
ray-roestenburg-da
left a comment
ray-roestenburg-da
left a comment
There was a problem hiding this comment.
Looks good, in the right direction, main thing that you discovered during our chat, to handle the import updates
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2456