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.

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

DYN-6038 Lucene search IMP #14428

Merged
merged 18 commits into from
Oct 12, 2023
Merged

DYN-6038 Lucene search IMP #14428

merged 18 commits into from
Oct 12, 2023

Conversation

QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Sep 20, 2023

Purpose

Improvements to Lucene search, a legacy branch build available at storebox\DynamoCore\LuceneSearchIMP\px64\LuceneSearchIMP_3.0.0.6295

Here are the improvements included:

  • Remove duplicate instances of AddNodeTypeToSearchIndex and add it to utility class
  • Rewrite Lucene search utility initialization logic so code can be much simplified across three cases where we are using search: nodes search, autocomplete search, packages search
  • Introduced LuceneStartConfig class for utility object initialization logic
  • Dispose index writer immediately after writing changes to disk ( not in RAM mode) so that index files on disk are unlocked for multiple Dynamo sessions
  • Skip writing index files if they exist, read index files when available in the same case
  • Add forcing dispose function for LuceneSearchUtilty and call it as part of DynamoModel close, WorkspaceViewModel close, etc.
  • Some small bug fixes

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

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

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 QilongTang added the WIP label Sep 20, 2023
@QilongTang QilongTang added this to the 3.0 milestone Sep 20, 2023
@QilongTang QilongTang marked this pull request as ready for review September 21, 2023 06:44
@QilongTang
Copy link
Contributor Author

Marking this ready just so PR checks could run

@QilongTang
Copy link
Contributor Author

@avidit Branch build available at storebox\DynamoCore\LuceneSearchIMP\px64\LuceneSearchIMP_3.0.0.6295 for testing

@@ -114,7 +169,7 @@ internal void InitializeLuceneConfig(string dirName, LuceneStorage storageType =
/// <returns></returns>
internal Document InitializeIndexDocumentForNodes()
{
if (DynamoModel.IsTestMode && currentStorageType == LuceneStorage.FILE_SYSTEM) return null;
if (DynamoModel.IsTestMode && startConfig.StorageType == LuceneStorage.FILE_SYSTEM) return null;
Copy link
Member

Choose a reason for hiding this comment

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

should this log an error or something to the console?

@QilongTang QilongTang changed the title DYN-6038 Lucene search IMP (WIP) DYN-6038 Lucene search IMP Oct 6, 2023
@QilongTang QilongTang removed the WIP label Oct 6, 2023
Copy link
Contributor

@reddyashish reddyashish 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.

@QilongTang
Copy link
Contributor Author

@reddyashish Thanks for the review! Merging now

@QilongTang QilongTang merged commit 5f30633 into master Oct 12, 2023
25 checks passed
@QilongTang QilongTang deleted the LuceneSearchIMP branch October 12, 2023 16:21
QilongTang added a commit that referenced this pull request Mar 4, 2024
* 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
@QilongTang QilongTang mentioned this pull request Mar 4, 2024
9 tasks
QilongTang added a commit that referenced this pull request Mar 5, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants