You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by "nfsantos (via GitHub)" <gi...@apache.org> on 2024/08/23 13:22:21 UTC
[PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
nfsantos opened a new pull request, #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos merged PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#discussion_r1731725499
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/DocumentStoreIndexerBase.java:
##########
@@ -357,26 +369,37 @@ public void reindex() throws CommitFailedException, IOException {
indexParallel(flatFileStores, indexer, progressReporter);
} else if (flatFileStores.size() == 1) {
FlatFileStore flatFileStore = flatFileStores.get(0);
- TopKSlowestPaths slowestTopKElements = new TopKSlowestPaths(TOP_SLOWEST_PATHS_TO_LOG);
- indexer.onIndexingStarting();
- long entryStart = System.nanoTime();
- for (NodeStateEntry entry : flatFileStore) {
- reportDocumentRead(entry.getPath(), progressReporter);
- indexer.index(entry);
- // Avoid calling System.nanoTime() twice per each entry, by reusing the timestamp taken at the end
- // of indexing an entry as the start time of the following entry. This is less accurate, because
- // the measured times will also include the bookkeeping at the end of indexing each entry, but
- // we are only interested in entries that take a significant time to index, so this extra
- // inaccuracy will not significantly change the results.
- long entryEnd = System.nanoTime();
- long elapsedMillis = (entryEnd - entryStart) / 1_000_000;
- entryStart = entryEnd;
- slowestTopKElements.add(entry.getPath(), elapsedMillis);
- if (elapsedMillis > 1000) {
- log.info("Indexing {} took {} ms", entry.getPath(), elapsedMillis);
+ try (AheadOfTimeBlobDownloader aheadOfTimeBlobDownloader = getAheadOfTimeBlobDownloaderInterface(flatFileStore, indexer)) {
Review Comment:
The AheadOfTimeBlobDownloader is an interface and the only implementation requires a FlatFileStore. So another IndexStore is not supported. I would instead extend the FlatFileStore such that it can be wrapped by the AheadOfTimeBlobDownloader and then the AheadOfTimeBlobDownloader could be an IndexStore.
That way there is no hard dependency on the AheadOfTimeBlobDownloader here. Instead, we can use the IndexStore here. We can discuss in person tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#discussion_r1731725499
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/DocumentStoreIndexerBase.java:
##########
@@ -357,26 +369,37 @@ public void reindex() throws CommitFailedException, IOException {
indexParallel(flatFileStores, indexer, progressReporter);
} else if (flatFileStores.size() == 1) {
FlatFileStore flatFileStore = flatFileStores.get(0);
- TopKSlowestPaths slowestTopKElements = new TopKSlowestPaths(TOP_SLOWEST_PATHS_TO_LOG);
- indexer.onIndexingStarting();
- long entryStart = System.nanoTime();
- for (NodeStateEntry entry : flatFileStore) {
- reportDocumentRead(entry.getPath(), progressReporter);
- indexer.index(entry);
- // Avoid calling System.nanoTime() twice per each entry, by reusing the timestamp taken at the end
- // of indexing an entry as the start time of the following entry. This is less accurate, because
- // the measured times will also include the bookkeeping at the end of indexing each entry, but
- // we are only interested in entries that take a significant time to index, so this extra
- // inaccuracy will not significantly change the results.
- long entryEnd = System.nanoTime();
- long elapsedMillis = (entryEnd - entryStart) / 1_000_000;
- entryStart = entryEnd;
- slowestTopKElements.add(entry.getPath(), elapsedMillis);
- if (elapsedMillis > 1000) {
- log.info("Indexing {} took {} ms", entry.getPath(), elapsedMillis);
+ try (AheadOfTimeBlobDownloader aheadOfTimeBlobDownloader = getAheadOfTimeBlobDownloaderInterface(flatFileStore, indexer)) {
Review Comment:
The AheadOfTimeBlobDownloader is an interface and the only "real" implementation requires a FlatFileStore. So another IndexStore is not supported (which is fine because only the FlatFileStore needs it). But it means all IndexStores need to support somehow the AheadOfTimeBlobDownloader which is not true: the TreeStore doesn't need it, so the AheadOfTimeBlobDownloader should not appear here.
I would instead extend the FlatFileStore such that it can be wrapped by the AheadOfTimeBlobDownloader and then the AheadOfTimeBlobDownloader could be an IndexStore.
That way there is no hard dependency on the AheadOfTimeBlobDownloader here. And so the TreeStore doesn't require a dependency on the AheadOfTimeBlobDownloader or implement any of it's methods. Instead, we can use the IndexStore here.
I'm against "forcing" a dependency on the AheadOfTimeBlobDownloader on other IndexStores that do not need it.
We can discuss in person tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#discussion_r1740670806
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/AheadOfTimeBlobDownloadingFlatFileStore.java:
##########
@@ -75,17 +80,35 @@ private AheadOfTimeBlobDownloadingFlatFileStore(FlatFileStore ffs, CompositeInde
maxPrefetchWindowSize,
maxPrefetchWindowMB);
}
- }
+ }
+
+ static boolean isEnabledForIndexes(String indexesEnabledPrefix, List<String> indexPaths) {
+ List<String> enableForIndexes = splitAndTrim(indexesEnabledPrefix);
+ for (String indexPath : indexPaths) {
+ if (enableForIndexes.stream().anyMatch(indexPath::startsWith)) {
+ return true;
Review Comment:
There is a test case for when `indexesEnabledPrefix` is empty, the function correctly returns false. See the first case in the companion test class below. The loop won't be executed if the string is empty and it will fall through to the `return false` at the end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#discussion_r1731725499
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/DocumentStoreIndexerBase.java:
##########
@@ -357,26 +369,37 @@ public void reindex() throws CommitFailedException, IOException {
indexParallel(flatFileStores, indexer, progressReporter);
} else if (flatFileStores.size() == 1) {
FlatFileStore flatFileStore = flatFileStores.get(0);
- TopKSlowestPaths slowestTopKElements = new TopKSlowestPaths(TOP_SLOWEST_PATHS_TO_LOG);
- indexer.onIndexingStarting();
- long entryStart = System.nanoTime();
- for (NodeStateEntry entry : flatFileStore) {
- reportDocumentRead(entry.getPath(), progressReporter);
- indexer.index(entry);
- // Avoid calling System.nanoTime() twice per each entry, by reusing the timestamp taken at the end
- // of indexing an entry as the start time of the following entry. This is less accurate, because
- // the measured times will also include the bookkeeping at the end of indexing each entry, but
- // we are only interested in entries that take a significant time to index, so this extra
- // inaccuracy will not significantly change the results.
- long entryEnd = System.nanoTime();
- long elapsedMillis = (entryEnd - entryStart) / 1_000_000;
- entryStart = entryEnd;
- slowestTopKElements.add(entry.getPath(), elapsedMillis);
- if (elapsedMillis > 1000) {
- log.info("Indexing {} took {} ms", entry.getPath(), elapsedMillis);
+ try (AheadOfTimeBlobDownloader aheadOfTimeBlobDownloader = getAheadOfTimeBlobDownloaderInterface(flatFileStore, indexer)) {
Review Comment:
The AheadOfTimeBlobDownloader is an interface and the only "real" implementation requires a FlatFileStore. So another IndexStore is not supported (which is fine because only the FlatFileStore needs it). But it means all IndexStores need to support somehow the AheadOfTimeBlobDownloader which is not true: the TreeStore doesn't need it, so the AheadOfTimeBlobDownloader should now appear here.
I would instead extend the FlatFileStore such that it can be wrapped by the AheadOfTimeBlobDownloader and then the AheadOfTimeBlobDownloader could be an IndexStore.
That way there is no hard dependency on the AheadOfTimeBlobDownloader here. And so the TreeStore doesn't require a dependency on the AheadOfTimeBlobDownloader or implement any of it's methods. Instead, we can use the IndexStore here.
We can discuss in person tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#discussion_r1731290686
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/DocumentStoreIndexerBase.java:
##########
@@ -357,26 +369,37 @@ public void reindex() throws CommitFailedException, IOException {
indexParallel(flatFileStores, indexer, progressReporter);
} else if (flatFileStores.size() == 1) {
FlatFileStore flatFileStore = flatFileStores.get(0);
- TopKSlowestPaths slowestTopKElements = new TopKSlowestPaths(TOP_SLOWEST_PATHS_TO_LOG);
- indexer.onIndexingStarting();
- long entryStart = System.nanoTime();
- for (NodeStateEntry entry : flatFileStore) {
- reportDocumentRead(entry.getPath(), progressReporter);
- indexer.index(entry);
- // Avoid calling System.nanoTime() twice per each entry, by reusing the timestamp taken at the end
- // of indexing an entry as the start time of the following entry. This is less accurate, because
- // the measured times will also include the bookkeeping at the end of indexing each entry, but
- // we are only interested in entries that take a significant time to index, so this extra
- // inaccuracy will not significantly change the results.
- long entryEnd = System.nanoTime();
- long elapsedMillis = (entryEnd - entryStart) / 1_000_000;
- entryStart = entryEnd;
- slowestTopKElements.add(entry.getPath(), elapsedMillis);
- if (elapsedMillis > 1000) {
- log.info("Indexing {} took {} ms", entry.getPath(), elapsedMillis);
+ try (AheadOfTimeBlobDownloader aheadOfTimeBlobDownloader = getAheadOfTimeBlobDownloaderInterface(flatFileStore, indexer)) {
Review Comment:
Can you elaborate? I'm not sure I understand the suggestion.
The AOT downloader needs to receive callbacks with the position of the indexer, in order to not go to far ahead of the indexer. That is, it requires the callback this callback: `aheadOfTimeBlobDownloader.updateIndexed(entriesRead);`.
which is done in the code below. Where could this code be moved to?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#discussion_r1732737698
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/DocumentStoreIndexerBase.java:
##########
@@ -357,26 +369,37 @@ public void reindex() throws CommitFailedException, IOException {
indexParallel(flatFileStores, indexer, progressReporter);
} else if (flatFileStores.size() == 1) {
FlatFileStore flatFileStore = flatFileStores.get(0);
- TopKSlowestPaths slowestTopKElements = new TopKSlowestPaths(TOP_SLOWEST_PATHS_TO_LOG);
- indexer.onIndexingStarting();
- long entryStart = System.nanoTime();
- for (NodeStateEntry entry : flatFileStore) {
- reportDocumentRead(entry.getPath(), progressReporter);
- indexer.index(entry);
- // Avoid calling System.nanoTime() twice per each entry, by reusing the timestamp taken at the end
- // of indexing an entry as the start time of the following entry. This is less accurate, because
- // the measured times will also include the bookkeeping at the end of indexing each entry, but
- // we are only interested in entries that take a significant time to index, so this extra
- // inaccuracy will not significantly change the results.
- long entryEnd = System.nanoTime();
- long elapsedMillis = (entryEnd - entryStart) / 1_000_000;
- entryStart = entryEnd;
- slowestTopKElements.add(entry.getPath(), elapsedMillis);
- if (elapsedMillis > 1000) {
- log.info("Indexing {} took {} ms", entry.getPath(), elapsedMillis);
+ try (AheadOfTimeBlobDownloader aheadOfTimeBlobDownloader = getAheadOfTimeBlobDownloaderInterface(flatFileStore, indexer)) {
Review Comment:
As discussed in person, could you suggest a refactoring of this PR to remove the dependency from the DocumentStoreIndexerBase to the AheadOfTimeBlobDownloader? I gave it a try but I did not find any obvious and simple way to do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#discussion_r1731234519
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/DocumentStoreIndexerBase.java:
##########
@@ -357,26 +369,37 @@ public void reindex() throws CommitFailedException, IOException {
indexParallel(flatFileStores, indexer, progressReporter);
} else if (flatFileStores.size() == 1) {
FlatFileStore flatFileStore = flatFileStores.get(0);
- TopKSlowestPaths slowestTopKElements = new TopKSlowestPaths(TOP_SLOWEST_PATHS_TO_LOG);
- indexer.onIndexingStarting();
- long entryStart = System.nanoTime();
- for (NodeStateEntry entry : flatFileStore) {
- reportDocumentRead(entry.getPath(), progressReporter);
- indexer.index(entry);
- // Avoid calling System.nanoTime() twice per each entry, by reusing the timestamp taken at the end
- // of indexing an entry as the start time of the following entry. This is less accurate, because
- // the measured times will also include the bookkeeping at the end of indexing each entry, but
- // we are only interested in entries that take a significant time to index, so this extra
- // inaccuracy will not significantly change the results.
- long entryEnd = System.nanoTime();
- long elapsedMillis = (entryEnd - entryStart) / 1_000_000;
- entryStart = entryEnd;
- slowestTopKElements.add(entry.getPath(), elapsedMillis);
- if (elapsedMillis > 1000) {
- log.info("Indexing {} took {} ms", entry.getPath(), elapsedMillis);
+ try (AheadOfTimeBlobDownloader aheadOfTimeBlobDownloader = getAheadOfTimeBlobDownloaderInterface(flatFileStore, indexer)) {
Review Comment:
I would prefer if we wrap the index store, instead of adding the logic in the DocumentStoreIndexerBase.
This is because the tree store doesn't require (this type of) the AheadOfTimeBlobDownloader.
We already have the IndexStore API, which can be wrapped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#issuecomment-2320254220
This PR requires reworking. The logic to decide if a blob should be downloaded is too generic, because it assumes that if a node name matches a given suffix, then it should download the `jcr:data` property. But in fact, we should download the `jcr:data` property if the node matches the suffix and if the `jcr:data` property is actually required for the index being indexes. As a first approach, we can add an additional configuration property that enables AOT blob download only if the index name is in a list given as a configuration property. This would required that we configure Oak with the indexes that require this field.
A better approach would be to somehow introspect the index definitions and figure out if they access the jcr:data field.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11061 - Indexing job: during indexing phase, download blobs ahead of time in separate thread pool [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1669:
URL: https://github.com/apache/jackrabbit-oak/pull/1669#discussion_r1740665524
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/AheadOfTimeBlobDownloadingFlatFileStore.java:
##########
@@ -75,17 +80,35 @@ private AheadOfTimeBlobDownloadingFlatFileStore(FlatFileStore ffs, CompositeInde
maxPrefetchWindowSize,
maxPrefetchWindowMB);
}
- }
+ }
+
+ static boolean isEnabledForIndexes(String indexesEnabledPrefix, List<String> indexPaths) {
+ List<String> enableForIndexes = splitAndTrim(indexesEnabledPrefix);
+ for (String indexPath : indexPaths) {
+ if (enableForIndexes.stream().anyMatch(indexPath::startsWith)) {
+ return true;
Review Comment:
... in which case we have "/oak:index/...".startsWith("")... that would always be true.
I think if enableForIndexes is empty, then this methods should always return false.
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/AheadOfTimeBlobDownloadingFlatFileStore.java:
##########
@@ -75,17 +80,35 @@ private AheadOfTimeBlobDownloadingFlatFileStore(FlatFileStore ffs, CompositeInde
maxPrefetchWindowSize,
maxPrefetchWindowMB);
}
- }
+ }
+
+ static boolean isEnabledForIndexes(String indexesEnabledPrefix, List<String> indexPaths) {
Review Comment:
Assuming indexesEnabledPrefix is not set, we have an empty string here...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org