| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 667
DYN-6037 lucene overloaded nodes #14136
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-6037 lucene overloaded nodes #14136
Conversation
Some nodes were missing for the search due that were overloaded so the same node was added several times in the search results without considering the parameters. The fix was indexing also the parameters so when executing the search we need to also match the parameters.
Fixing comment
| if (e.Parameters == null) | ||
| return string.IsNullOrEmpty(parameters); | ||
| else | ||
| return e.Parameters.Equals(parameters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put comments for this block, not too intuitive to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added more comments please let me know if the explanation is clear. Thanks
commit: 6bc39b7
Adding comments for explaining a piece of code.
| /// </summary> | ||
| public static string[] NodeIndexFields = { nameof(IndexFieldsEnum.Name), | ||
| nameof(IndexFieldsEnum.FullCategoryName), | ||
| nameof(IndexFieldsEnum.Description), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the Enum name from IndexFieldsEnum to NodeIndexFields, as it is specific to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed IndexFieldsEnum to NodeIndexFields
commit: fab8b07
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion, I just saw that the same enum property is used in PackageIndexFields also and one of the enum values is "Hosts". Not a big deal, you can revert this last name change if you want.
QilongTang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment then LGTM
Renaming IndexFieldsEnum to NodeFieldsEnum.
|
@RobertGlobant20 can you check if the regressions are real? |
|
Failing test is a ASM loading exception on visualization tests. |

Purpose
Fixing Lucene search result for overloaded nodes.
Some nodes were missing for the search due that were overloaded so the same node was added several times in the search results without considering the parameters.
The fix was indexing also the parameters so when executing the search we need to also match the parameters.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Fixing Lucene search result for overloaded nodes.
Reviewers
@QilongTang @reddyashish
FYIs