OAK-11131 - indexing-job: AOT Blob downloader may download blobs that are not needed for the indexes #1738
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nsidered for download by matching only against the indexes for which the feature is enabled. Previously, it was checking against all indexes, which could lead to downloading blobs for nodes that are not indexed by an index that needs the blob. Add tests for AOT blob downloader.
thomasmueller
approved these changes
Sep 26, 2024
...commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/CompositeIndexer.java
Outdated
Show resolved
Hide resolved
| @@ -86,6 +87,13 @@ public Set<String> getRelativeIndexedNodeNames() { | |||
| return result; | |||
| } | |||
|
|
|||
| @Override | |||
| public String getIndexName() { | |||
| // This is susceptible to infinite recursion if a composite indexer contains itself, directly or indirectly. | |||
There was a problem hiding this comment.
Suggested change
| // This is susceptible to infinite recursion if a composite indexer contains itself, directly or indirectly. | |
| // Many methods are susceptible to infinite recursion if a composite indexer contains itself, directly or indirectly. | |
| // In this case, the methods would throw a StackOverflowException |
There was a problem hiding this comment.
Yes, I agree, it should be a warning in the public documentation of the class. The users of this class have to be aware to avoid this issue. I updated the comment.
| @@ -126,6 +126,11 @@ public Set<String> getRelativeIndexedNodeNames() { | |||
| return definition.getRelativeNodeNames(); | |||
| } | |||
|
|
|||
| @Override | |||
| public String getIndexName() { | |||
| return definition.getIndexName(); | |||
There was a problem hiding this comment.
I think the usage of getIndexName() is fine. However, just FYI:
For on-prem and managed services, we support the same node name in multiple locations. For example it's possible to have
- /oak:index/damAssetLucene
- /content/dam/oak:index/damAssetLucene
both in the same repository.
It is unlikely that this causes issues for this case here however (in my view).
There was a problem hiding this comment.
definition.getIndexName() returns the full path of the index, so there is no risk of collisions for indexes that are outside /oak:index.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The AOT Blob downloader makes several checks on each line/node in the FFS before downloading an non-inlined blob. One of these checks is if the line/node is needed by the indexers for which the feature is enabled. The current implementation is checking if any of the indexers requires the line. This is incorrect, because it may download blobs for lines that are not required by the indexers with AOT blob download enabled. In the worst case, this will slow down significantly the downloader, which may make it fall behind the indexer, thereby preventing it from downloading the blobs for the lines that are actually needed by the indexers.
This PR changes the logic so that the AOT blob downloader to only check against the indexes for which AOT blob download is enabled when deciding if a line/node is to be considered for blob download or not.
This PR also adds additional tests for the AOT blob downloader.