-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
misc fixes #7633
misc fixes #7633
Conversation
@@ -41,6 +41,8 @@ services: | |||
- METADATA_SERVICE_AUTH_ENABLED=false | |||
- JAVA_TOOL_OPTIONS=-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:5001 | |||
- BOOTSTRAP_SYSTEM_UPDATE_WAIT_FOR_SYSTEM_UPDATE=false | |||
- SEARCH_SERVICE_ENABLE_CACHE=false |
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.
was search caching causing the failure in the test?
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.
This is not used when running smoke tests. This is run when developing locally (enables debugger for example 2 lines above), not used when running quickstart by default, nor in smoke-tests. I think that cache should be disabled for local development by default because one of the first things I did was to search, then realized I was missing data, loaded data, and the results were cached. This is a quality of life for local development.
...c/main/java/com/linkedin/metadata/search/elasticsearch/query/request/SearchQueryBuilder.java
Show resolved
Hide resolved
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.
The docker/python changes in this PR lgtm
@@ -39,7 +40,12 @@ public class SearchQueryBuilderTest { | |||
exactMatchConfiguration.setCaseSensitivityFactor(0.7f); | |||
exactMatchConfiguration.setEnableStructured(true); | |||
|
|||
PartialConfiguration partialConfiguration = new PartialConfiguration(); | |||
partialConfiguration.setFactor(0.4f); |
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.
Nice refactor
# Field weight annotations are typically calibrated for exact match, if partial match is possible on the field use these adjustments | ||
partial: | ||
urnFactor: ${ELASTICSEARCH_QUERY_PARTIAL_URN_FACTOR:0.7} # multiplier on Urn token match, a partial match on Urn > non-Urn is assumed | ||
factor: ${ELASTICSEARCH_QUERY_PARTIAL_FACTOR:0.4} # multiplier on possible non-Urn token match |
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.
Wonder if we could set this on a per-field basis inside the Searchable annotation...
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.
Probably useful, but ultimately likely to be replaced by a proper search runtime configuration yaml. The model annotations are fine for index time decisions, we can detect and reindex as needed. However query-time/runtime configuration as model annotations is difficult to override at runtime unless you also are introducing custom models replacing the existing ones ....not even sure that would work. The query-time search configuration via annotations in my opinion are likely to be removed for a dedicated configuration file that is used at runtime. cc: @RyanHolstien @iprentic
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.
Yeah this makes sense to me. The model should host static (non-changing) configurations only.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fix slim trivy scan adjust urn partial further
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.
LGTM
Included: #7628
Checklist