Skip to content
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

fix: delete references if schema is deleted #1027

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

keejon
Copy link
Contributor

@keejon keejon commented Jan 21, 2025

Moves handling of references (basically maintaining the reference index) to InMemoryDatabase to prevent issue where a schema is deleted but the references are still indexed in memory which can lead to issues e.g. not being able to delete schemas that are referenced.

Removal of references is still left on API level in addition as otherwise this will introduce some delay on the primary node until references are synced (which could lead to issues if someone wants to delete multiple schemas in consecutive calls)

Also removed removal of references on API level as sending removal message is blocking and waiting for schema_reader to sync (See https://github.com/Aiven-Open/karapace/blob/keejon/fix-delete-references/src/karapace/messaging.py#L63)

@keejon keejon force-pushed the keejon/fix-delete-references branch 2 times, most recently from 73711ca to ba65ad9 Compare January 21, 2025 23:00
Copy link

github-actions bot commented Jan 21, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/karapace
  in_memory_database.py 386
  schema_models.py
  schema_reader.py
  schema_references.py
  schema_registry.py
  src/karapace/coordinator
  master_coordinator.py
Project Total  

This report was generated by python-coverage-comment-action

@keejon keejon force-pushed the keejon/fix-delete-references branch 2 times, most recently from 61fc7c5 to a8cbf98 Compare January 22, 2025 00:05
@keejon keejon marked this pull request as ready for review January 22, 2025 07:49
@keejon keejon requested review from a team as code owners January 22, 2025 07:49
@@ -251,6 +251,10 @@ def insert_schema_version(
references=references,
)

if references:
for ref in references:
self.insert_referenced_by(subject=ref.subject, version=ref.version, schema_id=schema_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

The insert_referenced_by locks self.schema_lock_thread, lock is re-entrant but maybe this function should be private and skip locking as this function does it already?

Similar locking happens with removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made insert and remove referenced_by private and removed it from API level because messaging should be blocking until schema_ready has read and handled message with the latest offset. (have left locking in though because it does not hurt and we can be safe that nobody misses it in the future) Strange thing is that this is not working as expected though and i get 422 in integration tests. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jjaakola-aiven tests are green again

@keejon keejon force-pushed the keejon/fix-delete-references branch 3 times, most recently from 569aa1f to a662bdb Compare January 23, 2025 09:35
@keejon keejon force-pushed the keejon/fix-delete-references branch from a662bdb to 485a95c Compare January 23, 2025 09:59
@jjaakola-aiven jjaakola-aiven merged commit 6e61dfe into main Jan 23, 2025
9 checks passed
@jjaakola-aiven jjaakola-aiven deleted the keejon/fix-delete-references branch January 23, 2025 10:32
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.

2 participants