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

Handle calling the property service in transaction better #4434

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Sep 10, 2024

Summary

  • Pass a transaction handle to RetrieveAllProperties and RetrieveAllPropertiesForEntity - These interface methods wouldn't take transaction into account.
  • Call an explicit commit in ListEvaluationHistory handler - We refresh the properties in the historical evaluation endpoint (which btw should just be temporary to convert from the old tables to the new general tables) and missed the fact that the handler starts a transaction and then always rolls back - that was probably a way to keep consistence. This means that the transactions that refresh properties are rolled back and on next call fetched again. This hurts performance quite a bit on repeated listHistory calls.

Fixes: #4393

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

by calling the list history endpoint with expired entities

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@coveralls
Copy link

Coverage Status

coverage: 53.274% (-0.009%) from 53.283%
when pulling a9a09de on jhrozek:pass_tx_to_prop_svc
into bcdd94e on stacklok:main.

JAORMX
JAORMX previously approved these changes Sep 11, 2024
JAORMX
JAORMX previously approved these changes Sep 11, 2024
@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 11, 2024

smoke tests succeeded locally - I'll merge once we stabilize our infra smoke tests

…pertiesForEntity

These interface methods wouldn't take transaction into account.

Related: mindersec#4393
@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 11, 2024

rebased on current main

We refresh the properties in the historical evaluation endpoint (which
btw should just be temporary to convert from the old tables to the new
general tables) and missed the fact that the handler starts a
transaction and then always rolls back - that was probably a way to keep
consistence.

This means that the transactions that refresh properties are rolled back
and on next call fetched again. This hurts performance quite a bit on
repeated listHistory calls.

Fixes: mindersec#4393
@jhrozek jhrozek force-pushed the pass_tx_to_prop_svc branch from 0b223a0 to 220ab65 Compare September 11, 2024 11:20
@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 11, 2024

and a lint fix

@jhrozek jhrozek merged commit 78652f8 into mindersec:main Sep 11, 2024
21 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.

Need to handle transactions when refreshing properties in historical evaluations better
3 participants