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: When changing the ontologies refresh the cache completely (DEV-4362) #3468

Merged
merged 24 commits into from
Jan 29, 2025

Conversation

seakayone
Copy link
Contributor

@seakayone seakayone commented Jan 21, 2025

  • inline OntologyTriplestoreHelpers trait
  • inline OntologyCacheHelpers trait
  • replace updating the Ontology manually with refreshing from DB
  • remove after update checks

Pull Request Checklist

Task Description/Number

Issue Number: DEV-

PR Type

  • build/chore: maintenance tasks (no production code change)
  • docs: documentation changes (no production code change)
  • feat: represents new features
  • fix: represents bug fixes
  • perf: performance improvements
  • refactor: represents production code refactoring
  • test: adding or refactoring tests (no production code change)
  • deprecated: Deprecation warning (ideally referencing a migration guide)

Basic requirements for bug fixes and features

  • Tests for the changes have been added
  • Docs have been added / updated

Does this PR introduce a breaking change?

  • Yes

@seakayone seakayone changed the title fix/ontology cache fix: When changing the ontologies refresh the cache completely (DEV-4362) Jan 21, 2025
Copy link

linear bot commented Jan 21, 2025

@seakayone seakayone self-assigned this Jan 23, 2025
@seakayone seakayone marked this pull request as ready for review January 27, 2025 10:41
Copy link
Contributor

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

Nice simplification!

Question: Are you confident that the extra work the API now has to do does not cause any significant performance regressions? Should we dedicate some time to test this specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Nice to see so much deleted!

question: Do you think it would be worth adding a test to assert that any change to an ontology also triggers a cache reload?

@seakayone
Copy link
Contributor Author

seakayone commented Jan 27, 2025

does not cause any significant performance regressions

The added overhead is refreshing the ontology cache, which is ~ .4 seconds or less. However, where this is added I removed the after update checks which my guess is take a similar amount of time. All in all I think the "performance" did not decrease significantly.

Do you think it would be worth adding a test to assert that any change to an ontology also triggers a cache reload?

No. Most of it should be covered with existing tests already.

Copy link
Contributor

Grat, thanks!

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.28%. Comparing base (6ebe8e7) to head (c4686c6).
Report is 180 commits behind head on main.

Files with missing lines Patch % Lines
...c/main/scala/org/knora/webapi/core/AppServer.scala 40.00% 3 Missing ⚠️
...ora/webapi/responders/v2/OntologyResponderV2.scala 95.71% 3 Missing ⚠️
...nders/v2/ontology/OntologyTriplestoreHelpers.scala 93.75% 1 Missing ⚠️
...api/slice/admin/api/service/StoreRestService.scala 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3468      +/-   ##
==========================================
- Coverage   83.04%   82.28%   -0.77%     
==========================================
  Files         290      312      +22     
  Lines       23087    22576     -511     
==========================================
- Hits        19172    18576     -596     
- Misses       3915     4000      +85     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seakayone seakayone merged commit 6be8422 into main Jan 29, 2025
9 checks passed
@seakayone seakayone deleted the fix/ontology-cache branch January 29, 2025 08:40
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