Monitor Improvements LUCENE-10422 #679

mogui · 2022-02-14T14:35:18Z

Description

Change how Monitor manages it's directory, IndexWriter and IndexReader, giving the possibility to

use a custom Directory implementation
use a readonly QueryIndex in order to have more Monitor instance on different server reading from the same index (now the index reader is created from the index writer so it is impossible to make a readonly Monitor)

Solution

I've extended MonitorConfiguration, with a readonly boolean property that governs ho the QueryIndex is created, and I also added a functional property that acts as a Directory Provider

Tests

I've added tests to confirm two readonly monitor could work as expected.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.

romseygeek

Thanks for opening this @mogui! I left some comments - I think we can break things up into read-only and writeable classes which should help simplify things a bit.

lucene/monitor/src/java/org/apache/lucene/monitor/DirectoryProviderFunctionalInterface.java

lucene/monitor/src/java/org/apache/lucene/monitor/MonitorConfiguration.java

lucene/monitor/src/java/org/apache/lucene/monitor/QueryIndex.java

…ReadonlyQueryIndex classes.

mogui · 2022-02-16T11:51:34Z

@romseygeek I've updated with the requested changes

romseygeek

Hi @mogui, thanks for your updates. I left a couple of comments.

lucene/monitor/src/java/org/apache/lucene/monitor/Monitor.java

lucene/monitor/src/java/org/apache/lucene/monitor/MonitorConfiguration.java

lucene/monitor/src/java/org/apache/lucene/monitor/ReadonlyQueryIndex.java

moved common logic to QueryIndex now an abstract class Removed Lazy Parsing of queries, readonly monitor uses in memory cache just like Writable, but without locking and purge executor.

mogui · 2022-02-18T09:51:37Z

@romseygeek OK I've moved things around, making QueryIndex an abstract class sharing common code for both implementations
ReadonlyQueryIndex have the same logic of in-memory laoding of parsed queries, but without locking handling and without purge executor.
I've made it usable by enabling purging cache so it can be done externally.

romseygeek

Thanks @mogui! I left some more comments.

lucene/monitor/src/java/org/apache/lucene/monitor/ReadonlyQueryIndex.java

lucene/monitor/src/java/org/apache/lucene/monitor/WritableQueryIndex.java

lucene/monitor/src/test/org/apache/lucene/monitor/TestCachePurging.java

lucene/monitor/src/test/org/apache/lucene/monitor/TestMonitorReadonly.java

…cache Added some docs

mogui · 2022-02-22T16:33:15Z

@romseygeek I should have fixed everything, also added few lines of docs to explain read-only behaviour.

romseygeek · 2022-02-23T12:34:04Z

lucene/monitor/src/java/org/apache/lucene/monitor/ReadonlyQueryIndex.java

+
+  @Override
+  void purgeCache(CachePopulator populator) throws IOException {
+    manager.maybeRefresh();


There's a race here, I think. Once you've called manager.maybeRefresh() then a subsequent call to search will return the new searcher, but you may not have updated this.queries yet.

should I call maybeRefreshBlocking instead?

I think actually the best solution is to remove the query cache entirely for this impl, which is where you started out - sorry for all the back and forth here. We can have a background thread that calls maybeRefresh() on the manager to keep up with updates, but all the queries will be read directly from the searcher and parsed as they are executed. The in-memory cache works when the Monitor in question is handling updates as well, but trying to do that when you have no idea what the changes are between IndexReaders is going to be nasty.

Ok, I think it was the best solution too, I'll work getting back to that solution.
Don't worry, all the back and forth got me to understand everything better!

@romseygeek I just realized that main branch from which I started the fork requires java 17, is there a way to make a build that targets Java 11 or do I have to remake the pull request on topo of 9_0_0 branch ?

@romseygeek in your opinion it makes sense to reuse purgeFrequency and purgeFrequencyUnits from MonitorConfiguration to configure the refreshing thread or is better a new pairof configurations?

@romseygeek did you had a chanche to look out the new commits?

live query parsing added executor to refresh index

romseygeek

Sorry for the delays @mogui. This is pretty close I think? I asked for a few changes in ReadOnlyQueryCache but the rest is looking good.

You should target main, so we need Java 17, yes.

I wonder if we should rename purgeCache to something like refreshCache, as cacheing only really applies to the writeable Monitor. Maybe in a followup though, as that's an API change and I'd need to think about deprecations etc.

lucene/monitor/src/java/org/apache/lucene/monitor/ReadonlyQueryIndex.java

mogui · 2022-03-14T09:18:08Z

@romseygeek fixed !

romseygeek

I left a couple more comments. Can you merge in the latest master branch as well, as github reckons there are conflicts now?

lucene/monitor/src/java/org/apache/lucene/monitor/ReadonlyQueryIndex.java

lucene/monitor/src/java/org/apache/lucene/monitor/Monitor.java

lucene/monitor/src/test/org/apache/lucene/monitor/TestMonitorReadonly.java

Moved listeners to abstract class QueryIndex to give listener logic to Readonly too and fix an ugly test

mogui · 2022-03-15T09:10:30Z

@romseygeek I've merged main and restored listeners to the abstarct class, You were right the sleep was very ugly

mogui · 2022-03-21T09:17:31Z

@romseygeek sorry to bother you, just a reminder :D

romseygeek · 2022-03-21T14:15:29Z

One last thing, can you add a CHANGES.txt entry in the Lucene 9.2.0 New Features section? And then I think we're good to merge! Thanks for all your patience on this.

* main: LUCENE-10473: Make tests a bit faster when running nightly. (apache#754) LUCENE-9905: Fix check in TestPerFieldKnnVectorsFormat#testMergeUsesNewFormat LUCENE-9614: Fix rare TestKnnVectorQuery failures LUCENE-10472: Fix TestMatchAllDocsQuery#testEarlyTermination (apache#753) LUCENE-10418: Move CHANGES to the correct section. LUCENE-10418: Optimize `Query#rewrite` in the non-scoring case. (apache#672) LUCENE-10469: Fix score mode propagation in ConstantScoreQuery. (apache#750) LUCENE-10452, LUCENE-10451: mention hunspell changes in CHANGES.txt LUCENE-10452: Hunspell: call checkCanceled less frequently to reduce the overhead (apache#723) Add 9.2.0 section to release notes LUCENE-10451 Hunspell: don't perform potentially expensive spellchecking after timeout (apache#721) LUCENE-10463: increment java version to 17 in smoke tester (apache#748)

mogui · 2022-03-21T14:40:12Z

Done!
Thanks for Your patience and support @romseygeek !

romseygeek

One small nit, we wrap lines in CHANGES to make it easier to read in text editors

lucene/CHANGES.txt

Co-authored-by: Alan Woodward <romseygeek@gmail.com>

mogui · 2022-03-21T15:33:05Z

Ok!

This commit adds a read-only monitor implementation that can search the QueryIndex of another monitor without supporting adding new queries.

mogui · 2022-03-23T18:08:31Z

@romseygeek There is something else in roadmap or future ideas regarding this package that I can contribute? Or where I can look to contribute more to the project. I'd like to remain involved if I can, it was nice to wotk with.

romseygeek · 2022-03-28T10:26:53Z

@mogui there isn't a roadmap for the monitor itself, but please do open issues and/or PRs if you have ideas for other improvements!

@monster

* main: (38 commits) remove obsolete image/description from luke/README.md Upgrade to forbiddenapis 3.3 (apache#768) LUCENE-10393: Unify binary dictionary and dictionary writer in kuromoji and nori (apache#740) LUCENE-9651 Update benchmark module docs (apache#759) LUCENE-10458: BoundedDocSetIdIterator may supply error count in Weigth#count(LeafReaderContext) when missingValue enables (apache#736) LUCENE-10481: FacetsCollector will not request scores if it does not use them (apache#760) LUCENE-10477: mention 'call multiple times' in Query.rewrite javadoc (apache#758) Add back-compat indices for 9.1.0. Synchronize CHANGES. LUCENE-10464, LUCENE-10477: WeightedSpanTermExtractor.extractWeightedSpanTerms to rewrite sufficiently (apache#737) Add version 9.1.0. DOAP changes for release 9.1.0 LUCENE-10422: Make errorprone happy LUCENE-10478: mark Test4GBStoredFields as @monster (apache#757) LUCENE-10422: Read-only monitor implementation (apache#679) LUCENE-10473: Make tests a bit faster when running nightly. (apache#754) LUCENE-9905: Fix check in TestPerFieldKnnVectorsFormat#testMergeUsesNewFormat LUCENE-9614: Fix rare TestKnnVectorQuery failures LUCENE-10472: Fix TestMatchAllDocsQuery#testEarlyTermination (apache#753) LUCENE-10418: Move CHANGES to the correct section. ...

@monster

* main: (52 commits) remove obsolete image/description from luke/README.md Upgrade to forbiddenapis 3.3 (apache#768) LUCENE-10393: Unify binary dictionary and dictionary writer in kuromoji and nori (apache#740) LUCENE-9651 Update benchmark module docs (apache#759) LUCENE-10458: BoundedDocSetIdIterator may supply error count in Weigth#count(LeafReaderContext) when missingValue enables (apache#736) LUCENE-10481: FacetsCollector will not request scores if it does not use them (apache#760) LUCENE-10477: mention 'call multiple times' in Query.rewrite javadoc (apache#758) Add back-compat indices for 9.1.0. Synchronize CHANGES. LUCENE-10464, LUCENE-10477: WeightedSpanTermExtractor.extractWeightedSpanTerms to rewrite sufficiently (apache#737) Add version 9.1.0. DOAP changes for release 9.1.0 LUCENE-10422: Make errorprone happy LUCENE-10478: mark Test4GBStoredFields as @monster (apache#757) LUCENE-10422: Read-only monitor implementation (apache#679) LUCENE-10473: Make tests a bit faster when running nightly. (apache#754) LUCENE-9905: Fix check in TestPerFieldKnnVectorsFormat#testMergeUsesNewFormat LUCENE-9614: Fix rare TestKnnVectorQuery failures LUCENE-10472: Fix TestMatchAllDocsQuery#testEarlyTermination (apache#753) LUCENE-10418: Move CHANGES to the correct section. ... # Conflicts: # lucene/sandbox/src/java/org/apache/lucene/sandbox/search/IndexSortSortedNumericDocValuesRangeQuery.java # lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestIndexSortSortedNumericDocValuesRangeQuery.java

niko.usai added 2 commits February 14, 2022 14:44

Read only monitor implementation

ca3c9c1

Format issue and documentation fixes

aa82442

romseygeek requested changes Feb 15, 2022

View reviewed changes

niko.usai and others added 5 commits February 15, 2022 12:17

Use extisting functional interface and giving it a default

adf012d

Moved readonly setting on setDirectoryProvider

b518b9d

Made QueryIndex an interface implemented by a WritableQueryIndex and …

8944a13

…ReadonlyQueryIndex classes.

Moved Purge Executor and listeners from monitor to WritableQueryIndex

8fb6bc8

Merge branch 'apache:main' into readonly-monitor

8136ae2

romseygeek requested changes Feb 17, 2022

View reviewed changes

Small improvements

d889db9

moved common logic to QueryIndex now an abstract class Removed Lazy Parsing of queries, readonly monitor uses in memory cache just like Writable, but without locking and purge executor.

romseygeek reviewed Feb 21, 2022

View reviewed changes

Added test to ensure readonly monitor gets changes after purging its …

f391fb6

…cache Added some docs

romseygeek reviewed Feb 23, 2022

View reviewed changes

Removed query cache from readonly monitor

bcb9c09

live query parsing added executor to refresh index

romseygeek requested changes Mar 9, 2022

View reviewed changes

niko.usai added 2 commits March 14, 2022 09:58

Small fixes on throwed exception

d8a2b90

misisng test fix

9a95855

romseygeek requested changes Mar 14, 2022

View reviewed changes

niko.usai added 3 commits March 15, 2022 09:44

Merge main

1ebcbfe

More Fix on Exception types

f3c6daa

Moved listeners to abstract class QueryIndex to give listener logic to Readonly too and fix an ugly test

addListener no more abstract

de7fa35

niko.usai added 2 commits March 21, 2022 15:33

Added Chacnges entry

5e1c63a

romseygeek approved these changes Mar 21, 2022

View reviewed changes

lucene/CHANGES.txt Outdated Show resolved Hide resolved

Update lucene/CHANGES.txt

21acdcb

Co-authored-by: Alan Woodward <romseygeek@gmail.com>

romseygeek merged commit be99178 into apache:main Mar 21, 2022

asfgit pushed a commit that referenced this pull request Mar 21, 2022

LUCENE-10422: Read-only monitor implementation (#679)

72cf36a

This commit adds a read-only monitor implementation that can search the QueryIndex of another monitor without supporting adding new queries.

cpoerschke added the module:monitor label Mar 1, 2024

cpoerschke mentioned this pull request Mar 1, 2024

LUCENE-9869 allow for configuring a custom cache purge scheduler in Monitor (aka Luwak) #99

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monitor Improvements LUCENE-10422 #679

Monitor Improvements LUCENE-10422 #679

mogui commented Feb 14, 2022

romseygeek left a comment

mogui commented Feb 16, 2022

romseygeek left a comment

mogui commented Feb 18, 2022 •

edited

Loading

romseygeek left a comment

mogui commented Feb 22, 2022

romseygeek Feb 23, 2022

mogui Feb 23, 2022

romseygeek Feb 28, 2022

mogui Feb 28, 2022

mogui Mar 1, 2022

mogui Mar 1, 2022

mogui Mar 9, 2022 •

edited

Loading

romseygeek left a comment

mogui commented Mar 14, 2022

romseygeek left a comment

mogui commented Mar 15, 2022

mogui commented Mar 21, 2022

romseygeek commented Mar 21, 2022

mogui commented Mar 21, 2022

romseygeek left a comment

mogui commented Mar 21, 2022

mogui commented Mar 23, 2022

romseygeek commented Mar 28, 2022

Monitor Improvements LUCENE-10422 #679

Monitor Improvements LUCENE-10422 #679

Conversation

mogui commented Feb 14, 2022

Description

Solution

Tests

Checklist

romseygeek left a comment

Choose a reason for hiding this comment

mogui commented Feb 16, 2022

romseygeek left a comment

Choose a reason for hiding this comment

mogui commented Feb 18, 2022 • edited Loading

romseygeek left a comment

Choose a reason for hiding this comment

mogui commented Feb 22, 2022

romseygeek Feb 23, 2022

Choose a reason for hiding this comment

mogui Feb 23, 2022

Choose a reason for hiding this comment

romseygeek Feb 28, 2022

Choose a reason for hiding this comment

mogui Feb 28, 2022

Choose a reason for hiding this comment

mogui Mar 1, 2022

Choose a reason for hiding this comment

mogui Mar 1, 2022

Choose a reason for hiding this comment

mogui Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

romseygeek left a comment

Choose a reason for hiding this comment

mogui commented Mar 14, 2022

romseygeek left a comment

Choose a reason for hiding this comment

mogui commented Mar 15, 2022

mogui commented Mar 21, 2022

romseygeek commented Mar 21, 2022

mogui commented Mar 21, 2022

romseygeek left a comment

Choose a reason for hiding this comment

mogui commented Mar 21, 2022

mogui commented Mar 23, 2022

romseygeek commented Mar 28, 2022

mogui commented Feb 18, 2022 •

edited

Loading

mogui Mar 9, 2022 •

edited

Loading