-
Notifications
You must be signed in to change notification settings - Fork 635
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
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.
@@ -138,7 +143,8 @@ public enum IndexFieldsEnum | |||
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.
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
*.resx
filesRelease Notes
Fixing Lucene search result for overloaded nodes.
Reviewers
@QilongTang @reddyashish
FYIs