-
Notifications
You must be signed in to change notification settings - Fork 30
Proposal for ACS ingestion begin #2041
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Oriol Muñoz <[email protected]>
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.
looks pretty reasonable!
Signed-off-by: Oriol Muñoz <[email protected]>
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.
Previously, the choice whether to start ingesting at participant begin or ledger end was left to the IngestionService. The name InitializeAcsAtLatestOffset
is a bit misleading, its comment describes better what it does.
Looking at the code, ALL ingestion services are set up to always initialize at participant begin (unless we override ingestFromParticipantBegin
in some config?). This is a bit surprising to me, as:
- AcsStores without a txlog can safely initialize at ledger end
- I thought the wallet AcsStore can also initialize at ledger end. The store has a txlog, but we create the store when we onboard the user, and before that the user can't do anything relevant to the wallet store.
Point 1 would be addressed by your PR.
I'm not so sure about point 2. The assumption that the store is initialized at an offset before the first relevant update is a bit shaky: the store is created through a trigger, so there are delays, and what if they bypass the validator app and submit commands directly to the participant before onboarding completes. On the other hand, having to stream in all updates since participant begin every time we onboard a new user sounds wasteful?
Other than that, the change looks good to me. Let's make sure we remove all the ingestFromParticipantBegin
and ingestUpdateHistoryFromParticipantBegin
config parameters when they become unused.
@@ -1055,7 +1055,7 @@ final class DbMultiDomainAcsStore[TXE]( | |||
s"Initializing the ACS at an offset chosen by the ingestion service, and resuming ingestion from there." | |||
) | |||
initState(acsStoreId, Some(txLogStoreId), None) | |||
IngestionStart.InitializeAcsAtLatestOffset | |||
IngestionStart.InitializeAtParticipantBegin |
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.
We could probably also leave this at InitializeAcsAtLatestOffset
, as we have a process for backfilling the missing txlog entries. This could mean the ACS store could be faster to catch up, but I don't think it's worth the trouble with dealing with incomplete txlog stores at app startup.
@rautenrieth-da wasn't that the behavior before we added the ingestFromParticipantBegin boolean flag? Which was fine afaiu |
Afaik, ledger API streams scale primarily in the number of events for the party you filter for. So if the party has been allocated at time X, it doesn't matter that participant begin is very long ago. Am I misremembering how the leger API indices work here? |
(does not compile, this is just for discussion's sake)