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.

Sign up for GitHub

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

Jump to bottom

OAK-8718: LuceneIndexStatsUpdateCallback is slow and synchronous whic… #157

Closed
wants to merge 2 commits into from
Closed

OAK-8718: LuceneIndexStatsUpdateCallback is slow and synchronous whic… #157

wants to merge 2 commits into from

Conversation

tihom88
Copy link
Contributor

@tihom88 tihom88 commented Oct 25, 2019

…h leads to slowness

@tteofili
Copy link
Contributor

+1 LGTM

thomasmueller
thomasmueller reviewed Oct 25, 2019
View reviewed changes
Copy link
Member

@thomasmueller thomasmueller left a comment

Choose a reason for hiding this comment

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

Just "formatting" changes

.../src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexEditorProvider.java Outdated Show resolved Hide resolved
...rc/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/AsyncIndexesSizeStatsUpdate.java Outdated Show resolved Hide resolved
...rc/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/AsyncIndexesSizeStatsUpdate.java Outdated Show resolved Hide resolved
...rc/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/AsyncIndexesSizeStatsUpdate.java Outdated Show resolved Hide resolved
...rc/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/AsyncIndexesSizeStatsUpdate.java Outdated Show resolved Hide resolved
.../src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexEditorProvider.java Outdated Show resolved Hide resolved
...main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexStatsUpdateCallback.java Outdated
Comment on lines 73 to 87
try {
long startTime = System.currentTimeMillis();
int docCount = Integer.parseInt(luceneIndexMBean.getDocCount(indexPath));
HistogramStats docCountHistogram = statisticsProvider.getHistogram(indexPath + NO_DOCS, StatsOptions.METRICS_ONLY);
docCountHistogram.update(docCount);
log.trace("{} stats updated, docCount {}, timeToUpdate {}", indexPath, docCount, System.currentTimeMillis() - startTime);
long indexSize = Long.parseLong(luceneIndexMBean.getSize(indexPath));
HistogramStats indexSizeHistogram = statisticsProvider.getHistogram(indexPath + INDEX_SIZE, StatsOptions.METRICS_ONLY);
indexSizeHistogram.update(indexSize);
long endTime = System.currentTimeMillis();
asyncIndexesSizeStatsUpdate.setLastStatsUpdateTime(indexPath, endTime);
log.debug("{} stats updated; docCount {}, size {}, timeToUpdate {}", indexPath, docCount, indexSize, endTime - startTime);
} catch (IOException e) {
log.debug("could not update no_docs/index_size stats for index at {}", indexPath, e);
}
Copy link
Member

Choose a reason for hiding this comment

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

Great!

...main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexStatsUpdateCallback.java Outdated Show resolved Hide resolved
...main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexStatsUpdateCallback.java Outdated Show resolved Hide resolved
fabriziofortino
fabriziofortino requested changes Oct 25, 2019
View reviewed changes
Copy link
Contributor

@fabriziofortino fabriziofortino left a comment

Choose a reason for hiding this comment

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

I have added other suggestions on top of @thomasmueller ones. Moreover, could we have some unit tests around this?

...rc/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/AsyncIndexesSizeStatsUpdate.java Outdated Show resolved Hide resolved
...rc/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/AsyncIndexesSizeStatsUpdate.java Outdated Show resolved Hide resolved
...src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexProviderService.java Outdated Show resolved Hide resolved
...src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexProviderService.java Show resolved Hide resolved
...main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexStatsUpdateCallback.java Outdated Show resolved Hide resolved
...main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexStatsUpdateCallback.java Outdated Show resolved Hide resolved
...main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexStatsUpdateCallback.java Outdated Show resolved Hide resolved
...main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexStatsUpdateCallback.java Outdated Show resolved Hide resolved
@catholicon
Copy link
Contributor

I'd do the review a bit later... But had a general comment - would it be useful to have an interface that's used to store stats. There can then a NOOP implementation which does nothing. A working one which is essentially things extracted from current patch.
On the same lines, I think the decision to record stats it not could also be extracted out into that class. That's probably be quite helpful in testing if the implemention is recording stats correctly.
On the thought about testing, it'd easier to use a Clock object. Setting Clock.VIRTUAL would allow to test "update only if time has elapsed" behaviour.

thomasmueller
thomasmueller reviewed Oct 30, 2019
View reviewed changes
Copy link
Member

@thomasmueller thomasmueller left a comment

Choose a reason for hiding this comment

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

Looks good to me now!

thomasmueller
thomasmueller approved these changes Oct 30, 2019
View reviewed changes
Copy link
Member

@thomasmueller thomasmueller left a comment

Choose a reason for hiding this comment

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

Forgot to approve

@github-actions
Copy link

This PR is stale because it has been open 24 months with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Nov 15, 2022
@github-actions github-actions bot closed this Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catholicon catholicon catholicon left review comments

@thomasmueller thomasmueller thomasmueller approved these changes

@fabriziofortino fabriziofortino fabriziofortino approved these changes

Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
5 participants
@tihom88 @tteofili @catholicon @thomasmueller @fabriziofortino