Skip to content

[scd] Change the subcription locking mechanism to a simple, shared lock. - #1363

Merged
barroco merged 1 commit into
interuss:masterfrom
Orbitalize:simple_db_lock
Feb 24, 2026
Merged

barroco merged 1 commit into
interuss:masterfrom
Orbitalize:simple_db_lock

Conversation

@the-glu

@the-glu the-glu commented Jan 21, 2026

Copy link
Copy Markdown
Member

This PR proposes a new method of locking subscriptions, by having a shared lock used for all subscriptions, instead of locking all rows.

See #1311 (comment) for graphs and details about the behavior change.

I added documentation in operations/performances, expecting #1362 to reuse it as well.

@the-glu
the-glu marked this pull request as ready for review January 29, 2026 14:36
@barroco
barroco requested a review from mickmis January 29, 2026 14:50
Comment thread build/db_schemas/yugabyte/scd/upto-v1.1.0-add_locks.sql
Comment thread build/db_schemas/scd/upto-v3.3.0-add_locks.sql
Comment thread build/db_schemas/scd/upto-v3.3.0-add_locks.sql Outdated
Comment thread build/db_schemas/scd/upto-v3.3.0-add_locks.sql
## The SCD global lock option

!!! danger
All DSS instances in a DSS pool must use the same value for this option. Mixing will result in dramatically lower performance.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO there isn't enough safeguards or mitigation that this does not happen. At least IMO we want some visibility, e.g. expose this configuration flag on the _aux interface?
cc @BenjaminPelletier

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From InterUSS weekly:

  • solution to this should not take a large effort
  • failing fast is OK
  • addressing this through doc only is OK
  • exposing through aux interface is OK

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added the info in the aux interface

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then, since it is exposed, maybe it would makes sense adding some test case in monitoring ensuring the values are consistent across the instances?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mmm I'm not sure it's worth the hassle of having a full validation for that, as we may have better solutions in the future (e.g. with crdb support dropped) and that will only affect performances, in 'high load' cases.

Comment thread pkg/scd/store/datastore/subscriptions.go Outdated

@mickmis mickmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM modulo downfrom entry

Comment thread build/db_schemas/scd.libsonnet
Comment thread build/db_schemas/scd/upto-v3.3.0-add_locks.sql
@the-glu

the-glu commented Feb 19, 2026

Copy link
Copy Markdown
Member Author

NB: CI failure should be fixed with #1374

@barroco

barroco commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Merging since approved by @mickmis and comments addressed.

@barroco
barroco merged commit bcc93e6 into interuss:master Feb 24, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants