-
Notifications
You must be signed in to change notification settings - Fork 59
Lock acs_snapshot_data while writing #2436
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
Lock acs_snapshot_data while writing #2436
Conversation
[ci] Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
7717d5b to
1c60bd0
Compare
meiersi-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. Looks good to me, but I'm not deep enough in the code to judge whether it is really safe.
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Outdated
Show resolved
Hide resolved
[ci] Signed-off-by: Robert Autenrieth <robert.autenrieth@digitalasset.com>
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Show resolved
Hide resolved
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.
I don't like locks so you need to convince me there is no other option :-) (looks like I misread 'lock' vs 'advisory lock')
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/common/src/main/scala/org/lfdecentralizedtrust/splice/store/db/AdvisoryLockIds.scala
Show resolved
Hide resolved
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala
Show resolved
Hide resolved
| * In case the application crashes while holding the lock, the server should close the connection | ||
| * and abort the transaction as soon as it detects a disconnect. | ||
| * See [[com.digitalasset.canton.platform.store.backend.postgresql.PostgresDataSourceConfig]] for our connection keepalive settings. | ||
| * With default settings, the server should detect a dead connection within ~15sec. |
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.
will it though? We've seen queries stuck for longer than 15s after a pod kill/restart
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.
@nmarton-da: I remember that there was some trickery around keep alives that you figured out as part of the HA config for the IndexDB. Something around clients and servers having to set the right params? Do you remember?
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.
Actually, this ChatGPT convo has quite a bit of interesting info: https://chatgpt.com/share/68dbb7bd-48c8-8007-85e9-ae43fa3a5b54
Seems like it's worth checking our keep-alive configs for server and client -- independently of this PR.
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.
Added #2488 for double checking disconnect behavior, and leaving the advisory lock. We'd rather have stray locks than a corrupted database.
ray-roestenburg-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!
|
Actual behavior on CILR: I think that's enough to conclude that it takes a few minutes to release the lock, potentially leading to log warnings, but the trigger does not deadlock itself. |
Fixes https://github.com/DACH-NY/canton-network-internal/issues/1782