Lucene index amend improvements #14513

QilongTang · 2023-10-23T23:27:12Z

Purpose

Lucene index improvements to make sure index locks can always be released and amended when needed by multiple Dynamo sessions. Index lifetime diagrams as below:

What this PR includes:

Add getter for luceneSearchUtility.DirReader so we can remove the setting in each Dynamo class
User logger from DynamoModel to avoid threading issues when index lock gain timeout exception happens
Dispose indexwriter after package install and package path updates so node index are unlocked right away
Re-create indexwriter when asked if it is not available after DynamoModel initialization

Here is what it looks like:

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

Multiple Dynamo sessions can use search independently

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

QilongTang · 2023-10-23T23:28:32Z

Opened for running regressions, still WIP

QilongTang · 2023-10-25T05:21:28Z

src/DynamoCore/Utilities/LuceneSearchUtility.cs

@@ -147,12 +153,12 @@ internal void CreateLuceneIndexWriter()
            {

                DisposeWriter();
-                (ExecutionEvents.ActiveSession.GetParameterValue(ParameterKeys.Logger) as DynamoLogger).LogError($"LuceneNET LockObtainFailedException {ex}");
+                dynamoModel.Logger.LogError($"LuceneNET LockObtainFailedException {ex}");


I had to revert this because I found in edge case if the index is ever locked (although would rarely happen after all the improvements in this PR), the previous code will make Dynamo hang because of threading issues

src/DynamoCore/Utilities/LuceneSearchUtility.cs

reddyashish

LGTM with a comment

QilongTang · 2023-10-25T20:13:47Z

src/DynamoCoreWpf/Controls/StartPage.xaml.cs

@@ -366,7 +367,7 @@ private void RefreshBackupFileList(IEnumerable<string> filePaths)
            IEnumerable<string> filePaths)
        {
            files.Clear();
-            foreach (var filePath in filePaths)
+            foreach (var filePath in filePaths.Where(x => x != null))


This is throwing a bunch of exceptions so filtering out the nulls from the list

QilongTang · 2023-10-25T20:14:22Z

src/DynamoUtilities/PathHelper.cs

@@ -37,6 +37,9 @@ public static Exception CreateFolderIfNotExist(string folderPath)
        {
            try
            {
+                // Do not even try when folder path is null or empty.
+                // This usually happens when system folder dialog is initialized with empty path
+                if (string.IsNullOrEmpty(folderPath)) return null;


This was throwing exception when folder path is null

QilongTang · 2023-10-25T20:25:17Z

I am considering adding getter for dirReader and Searcher because they are always the same code so we do not have to leak this Lucence specific code everywhere in the Dynamo code base

QilongTang · 2023-10-25T20:51:42Z

Hi @reddyashish This should be ready for a final review

QilongTang · 2023-10-26T15:18:33Z

There are some new regressions that I am taking a look

reddyashish

LGTM once the regressions are fixed.

QilongTang · 2023-10-26T18:31:52Z

LGTM once the regressions are fixed.

Looks like it is my last commit about refactoring cause the regressions, looking at it

QilongTang · 2023-10-26T18:56:57Z

src/DynamoCore/Utilities/LuceneSearchUtility.cs

@@ -395,8 +403,8 @@ internal void DisposeWriter()
        /// </summary>
        internal void DisposeAll()
        {
-            writer?.Dispose();
-            dirReader?.Dispose();
+            DisposeWriter();


It looks like this was causing the regressions, because writer was disposed first so we need to set it to null, otherwise in the DirReader getter, we will be trying to access an already disposed object

QilongTang · 2023-10-26T18:57:28Z

Regressions fixed, waiting for PR validation task to pass

QilongTang · 2023-10-27T18:27:32Z

All tests passing, merging

* Use index writer to amend index after Dynamo Launch * Update sorting * Update * Update * Update * Make sure package loading end will already release index lock * Update Comments * Clean Up * Code clean up * clean up * Code Clean Up * regressions * revert code clean up because it affects running tests in parallel * update

* DYN-6038 Lucene search IMP (#14428) * improvements * Simply initialization logic * Skip indexing as part of DynamoModel if the index files already exist * Remove DynamoModel reference and use singleton * clean up * improvements and package manager search * Use single constructor and other code clean up * Update... * Update * Dispose all Lucene objects in the correct order * Adding DynamoModel back to test regressions * Null check * Update * IndexWriter dispose sequence only for non-RAM mode * Update * Lucene index amend improvements (#14513) * Use index writer to amend index after Dynamo Launch * Update sorting * Update * Update * Update * Make sure package loading end will already release index lock * Update Comments * Clean Up * Code clean up * clean up * Code Clean Up * regressions * revert code clean up because it affects running tests in parallel * update * update * Update AssemblySharedInfo.cs

Use index writer to amend index after Dynamo Launch

b103148

QilongTang marked this pull request as ready for review October 23, 2023 23:28

QilongTang added the DNM Do not merge. For PRs. label Oct 23, 2023

QilongTang added this to the 3.0 milestone Oct 23, 2023

QilongTang added 4 commits October 24, 2023 00:24

Merge branch 'master' into LuceneIndexAmend

d958562

Update sorting

d3b1cb9

Update

a4d66c2

Update

bfe91af

QilongTang commented Oct 25, 2023

View reviewed changes

QilongTang and others added 5 commits October 25, 2023 01:42

Update

1b8456c

Make sure package loading end will already release index lock

de04a5e

Update Comments

60a8189

Clean Up

c00b481

Merge branch 'master' into LuceneIndexAmend

25101cd

reddyashish reviewed Oct 25, 2023

View reviewed changes

src/DynamoCore/Utilities/LuceneSearchUtility.cs Show resolved Hide resolved

reddyashish reviewed Oct 25, 2023

View reviewed changes

QilongTang removed the DNM Do not merge. For PRs. label Oct 25, 2023

Code clean up

6d7adf0

QilongTang commented Oct 25, 2023

View reviewed changes

clean up

132786e

Code Clean Up

c6954b5

reddyashish approved these changes Oct 26, 2023

View reviewed changes

regressions

70531f9

QilongTang commented Oct 26, 2023

View reviewed changes

QilongTang added 2 commits October 27, 2023 12:18

revert code clean up because it affects running tests in parallel

fd46dfa

update

8be7597

QilongTang merged commit 4f279a9 into master Oct 27, 2023
25 checks passed

QilongTang deleted the LuceneIndexAmend branch October 27, 2023 18:27

QilongTang mentioned this pull request Mar 4, 2024

[CherryPick] Lucene search IMP #14997

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lucene index amend improvements #14513

Lucene index amend improvements #14513

QilongTang commented Oct 23, 2023 •

edited

Loading

QilongTang commented Oct 23, 2023

QilongTang Oct 25, 2023 •

edited

Loading

reddyashish left a comment

QilongTang Oct 25, 2023

QilongTang Oct 25, 2023

QilongTang commented Oct 25, 2023

QilongTang commented Oct 25, 2023

QilongTang commented Oct 26, 2023

reddyashish left a comment

QilongTang commented Oct 26, 2023

QilongTang Oct 26, 2023

QilongTang commented Oct 26, 2023

QilongTang commented Oct 27, 2023

Lucene index amend improvements #14513

Lucene index amend improvements #14513

Conversation

QilongTang commented Oct 23, 2023 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

QilongTang commented Oct 23, 2023

QilongTang Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

reddyashish left a comment

Choose a reason for hiding this comment

QilongTang Oct 25, 2023

Choose a reason for hiding this comment

QilongTang Oct 25, 2023

Choose a reason for hiding this comment

QilongTang commented Oct 25, 2023

QilongTang commented Oct 25, 2023

QilongTang commented Oct 26, 2023

reddyashish left a comment

Choose a reason for hiding this comment

QilongTang commented Oct 26, 2023

QilongTang Oct 26, 2023

Choose a reason for hiding this comment

QilongTang commented Oct 26, 2023

QilongTang commented Oct 27, 2023

QilongTang commented Oct 23, 2023 •

edited

Loading

QilongTang Oct 25, 2023 •

edited

Loading