Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support index-level request caching #931

Merged
merged 7 commits into from
Oct 26, 2021
Merged

Conversation

cmark
Copy link
Member

@cmark cmark commented Oct 25, 2021

This PR introduces index-level search request caching and potential configuration on request API level via cacheHits(ServiceProvider).
By default the system will cache:

  • all requests that are being executed on versioned content
  • certain ECL search requests (or partial ECL search requests)

@cmark cmark requested review from apeteri and nagyo October 25, 2021 15:44
@cmark cmark self-assigned this Oct 25, 2021
* @return whether the search should cache the resulting hits in the underlying index system or not
*/
protected boolean cacheHits(C context) {
return Revision.class.isAssignableFrom(getFrom()) && context.optionalService(PathWithVersion.class).isPresent();
Copy link
Member

Choose a reason for hiding this comment

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

PathWithVersion does not always contain a version URI; when the original request path does not correspond to a version identifier, eg. when it contains a branch separator character, only an absolute branch path will be returned from the resolver:

final String relativeBranchPath = terminologyResource.getRelativeBranchPath(uriToResolve.getPath());
if (uriToResolve.getPath().contains(Branch.SEPARATOR)) {
final String absoluteBranchPath = relativeBranchPath + uriToResolve.getTimestampPart();
return new PathWithVersion(absoluteBranchPath);
}

Copy link
Member

Choose a reason for hiding this comment

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

I agree, it would be nice to introduce a more rigorous condition for this, given that it is quite hard to validate if a search result is indeed correct when it was cached.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to any suggestions. This was the easiest way at the moment to determine whether a request is being executed on a version or not (and I think it's not that bad 😅).

Copy link
Member Author

Choose a reason for hiding this comment

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

@apeteri see this piece of code, the PathWithVersion instance gets injected only when it has a version URI:
https://github.com/b2ihealthcare/snow-owl/pull/931/files#diff-a4877e9655543b6f3a4aeb80fb2df33cfb521470f823c273b36214aad0b087ecR47

* @return whether the search should cache the resulting hits in the underlying index system or not
*/
protected boolean cacheHits(C context) {
return Revision.class.isAssignableFrom(getFrom()) && context.optionalService(PathWithVersion.class).isPresent();
Copy link
Member

Choose a reason for hiding this comment

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

I agree, it would be nice to introduce a more rigorous condition for this, given that it is quite hard to validate if a search result is indeed correct when it was cached.

Copy link
Member

@nagyo nagyo left a comment

Choose a reason for hiding this comment

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

👍🏻

@cmark cmark merged commit de2a41e into 8.x Oct 26, 2021
@cmark cmark deleted the feature/search-request-caching branch October 26, 2021 13:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants