DYN-6148 Dynamo Library Lucene Search #14287
Conversation
Implementing the use of Lucene Search in the Dynamo Library
|
This is a GIF showing a comparison between Library Search and InCanvas Search.
|
| @@ -50,10 +52,11 @@ public NodeItemDataProvider(NodeSearchModel model) : base(false) | |||
| this.model = model; | |||
| } | |||
|
|
|||
| public NodeItemDataProvider(NodeSearchModel model, IconResourceProvider iconProvider) | |||
| public NodeItemDataProvider(NodeSearchModel model, IconResourceProvider iconProvider, DynamoModel dynModel) | |||
There was a problem hiding this comment.
Please let me know if this will produce an API Breaking Change since the method is public but we need to have the DynamoModel in order to pass the dynamoModel.LuceneSearchUtility to the Search method.
There was a problem hiding this comment.
I have concern over this and would like to discuss with @reddyashish and you on Monday
There was a problem hiding this comment.
@mjkkirschner This is the place I mentioned in the meeting that I feel Singleton pattern could be good that we do not want to pass DynamoModel around for access to search
src/LibraryViewExtensionWebView2/Handlers/SearchResultDataProvider.cs
Outdated
Show resolved
Hide resolved
|
Now that #14248 is merged, let's rework this PR |
Fixing calls to LuceneSearchUtility for using the Lucene Singleton.
src/LibraryViewExtensionWebView2/Handlers/SearchResultDataProvider.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM, I guess in order to apply the same change on RC2.19, I will need to cherry pick the Singleton change there as well. See #14308
Removing extra space and removing unused usings
|
One regression to look at |
The SearchNodeTest() test was moved to DynamoLibraryItemsTest due that in the LibraryResourceProviderTests we are not creating the DynamoModel (also there is no way to get the DynamoModel) so the Lucene Singleton is not created and is crashing.
Also I did some minor changes in the test due that the FullName contains the category (so instead of "dyf://123.456.somepackage.{nodeName}" was replaced by "dyf://Core.Input.{nodeName}" otherwise the Code Block node is not found).
|
Hi @RobertGlobant20 Given the PR check is not triggering for you right now, and there is no active job running. You may want to trigger it manually |
|
|
@RobertGlobant20 Please also cherry-pick this to RC2.19.0 branch |
* Fix Node Autocomplete Regressions (#14275) * Fix Node Autocomplete Regressions Due that we are adding ZeroTouchNodes during test mode around 9 tests are failing, seems that there are several nodes duplicates so each test needs to be analyzed separately. Then in this fix I'm reverting the code for adding ZeroTouchNodes and also removing a test that will fail due to this change. * Fix Node Autocomplete Regressions Re-adding unit test with Failure category * DYN-6148 Dynamo Library Lucene Search (#14287) * DYN-6148 Dynamo Library Lucene Search Implementing the use of Lucene Search in the Dynamo Library * DYN-6148 Dynamo Library Lucene Search Fixing calls to LuceneSearchUtility for using the Lucene Singleton. * DYN-6148 Dynamo Library Lucene Search Removing extra space and removing unused usings * DYN-6148 Dynamo Library Lucene Search Regressions The SearchNodeTest() test was moved to DynamoLibraryItemsTest due that in the LibraryResourceProviderTests we are not creating the DynamoModel (also there is no way to get the DynamoModel) so the Lucene Singleton is not created and is crashing. Also I did some minor changes in the test due that the FullName contains the category (so instead of "dyf://123.456.somepackage.{nodeName}" was replaced by "dyf://Core.Input.{nodeName}" otherwise the Code Block node is not found).
Purpose
Switching from using the legacy search to the use of Lucene Search in the Dynamo Library
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Switching from using the legacy search to the use of Lucene Search in the Dynamo Library
Reviewers
@QilongTang @reddyashish
FYIs