-
Notifications
You must be signed in to change notification settings - Fork 50
Switch to relying on package names instead of module names #1976
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.
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
Switch to relying on package names instead of module names #1976
Conversation
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
|
/cluster_test |
|
Deploy cluster test triggered for Commit 3b845ce5b206cfe9ff5ed5af7ef95ee9acbc514e in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/28163 |
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
| } | ||
| } | ||
|
|
||
| "prevent against ingestion of same (moduleName, entityName) with different package name - via interface fallback" in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is the best summary for the reason of being of this PR
| -- strictly not necessary, but lets us define the constraint as mandatory afterwards, | ||
| -- and also makes index creation faster since there's no data to index | ||
| truncate table acs_store_template cascade; | ||
| truncate table user_wallet_acs_store cascade; | ||
| truncate table external_party_wallet_acs_store cascade; | ||
| truncate table validator_acs_store cascade; | ||
| truncate table sv_acs_store cascade; | ||
| truncate table dso_acs_store cascade; | ||
| truncate table scan_acs_store cascade; | ||
| truncate table splitwell_acs_store cascade; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double-check me that this is not a bad idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that all descriptors are bumped
| * another list; that is why the contract ID by itself cannot be used by | ||
| * itself as the source of the sort key. | ||
| */ | ||
| def listAssignedContractsNotOnDomainN( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused except in a test (also deleted)
| storeId: AcsStoreId, | ||
| migrationId: Long, | ||
| where: SQLActionBuilder, | ||
| companion: C, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should've been the case all along, instead of having where ti_q_n = ? on every query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, thanks for pulling through that simplification!
| MultiDomainAcsStoreTest.generatedCoids.value.size should (be >= 900 and be <= 1000) | ||
| } | ||
|
|
||
| "read assignment-mismatched contracts in a stable order" in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested listAssignedContractsNotOnDomainN, deleted
Signed-off-by: Oriol Muñoz <[email protected]>
|
/cluster_test |
|
Deploy cluster test triggered for Commit 1680a64b4cb0ace131dd19435f78cb0d69064cea in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/28663 |
moritzkiefer-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks a lot!
Please run a performance test before merging so we can avoid surprises for long downtimes due to ACS backfilling.
| storeId: AcsStoreId, | ||
| migrationId: Long, | ||
| where: SQLActionBuilder, | ||
| companion: C, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, thanks for pulling through that simplification!
| @@ -0,0 +1,362 @@ | |||
| -- strictly not necessary, but lets us define the constraint as mandatory afterwards, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know what performance we get for a full ACS backfill? I'm a bit nervous about this given that the ACS queries are known to not perform well without pruning and there is no pruning atm on SV participants.
How about on CILR you take one SV (maybe best to use the SV runbook given that it resets daily anyway so if it goes wrong we can just reset again) manually force a reingestion by dropping the data from the table and last ingested offset?
If it does take too long, the easy short term option we have is to merge this into the 3.4 branch only. Afaict, there isn't an immediate need to have this be on 3.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about on CILR you take one SV (maybe best to use the SV runbook given that it resets daily anyway so if it goes wrong we can just reset again) manually force a reingestion by dropping the data from the table and last ingested offset?
What is an "ACS backfill"? After truncating the ACS tables and bumping the store descriptor, the new ACS store will be initialized from the ACS endpoint of the ledger API, is that what you meant? Apart from the participant being slow to deliver the ACS, we are not batching SQL inserts when consuming the ACS
override def ingestAcs() = {
...
// TODO (#989): batch inserts
}and we load the whole ACS into one in-memory structure:
private def ingestAcsAndInFlight(offset: Long) = {
...
// TODO(#863): stream contracts instead of ingesting them as a single Seqso I agree that one test on CILR is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that what you meant?
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OriolMunoz-da So given the very rapid increase in ACS size on mainnet I'm rather nervous about merging this atm. Can we hold off on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or merge it into 3.4? There we will ingest the acs anyway so it's not making anything worse
...allet/src/main/scala/org/lfdecentralizedtrust/splice/wallet/store/db/DbUserWalletStore.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
...main/resources/db/migration/canton-network/postgres/stable/V047__package_name_acs_stores.sql
Show resolved
Hide resolved
apps/common/src/main/scala/org/lfdecentralizedtrust/splice/store/db/AcsQueries.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
Signed-off-by: Oriol Muñoz <[email protected]>
rautenrieth-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
…lback [ci] Signed-off-by: Oriol Muñoz <[email protected]>
moritzkiefer-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work thank you!

Fixes #829
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines