Add Pinot as new visibility option #5201
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neil-xie
changed the title
shijiesheng
reviewed
Aug 15, 2023
shijiesheng
reviewed
Aug 15, 2023
* enable json index, change the quries for exact/partial match, update the unit tests * make it pass integration test: found that order by is not supported in json index column * update partial match query. It didn't work in the mono repo * Implemented deletion method for Pinot visibility store (#5404)
bowenxia
reviewed
Sep 29, 2023
bowenxia
reviewed
Sep 29, 2023
bowenxia
reviewed
Oct 2, 2023
#5411) * Add pinot metrics client and update pinot visibility manager to use it
shijiesheng
reviewed
Oct 11, 2023
shijiesheng
reviewed
Oct 11, 2023
shijiesheng
approved these changes
Oct 18, 2023
3vilhamster
reviewed
Feb 15, 2024
|
|
||
| // ValidateQuery validates that search attributes in the query and returns modified query. | ||
| func (qv *VisibilityQueryValidator) ValidateQuery(whereClause string) (string, error) { | ||
| if len(whereClause) != 0 { |
There was a problem hiding this comment.
nit: use short return.
if len(whereClause) == 0 {
return whereClause, nil
}
that will make this function easier to read.
| } | ||
|
|
||
| func (qv *VisibilityQueryValidator) processSystemKey(expr sqlparser.Expr) (string, error) { | ||
| comparisonExpr := expr.(*sqlparser.ComparisonExpr) |
There was a problem hiding this comment.
nit: for all methods that expect a single type, it would be easier to use it directly instead of asserting to the interface.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Updated server start up to choose visibility manager based on the advanced visibility option
Added new triple visibility manager to provide options to write to both ES and Pinot for future migration
Added pinotVisibilityStore and pinotClient to support storing visibility messages in Pinot
Added util methods to flatten the search attributes into columns
Added util methods to validate pinot response and query
Added new kafka indexer message type for Pinot since Pinot doesn't need indexer to process
Added integration test to set up Pinot test cluster and test Pinot functionality
Why?
ElasticSearch is going to be deprecated with Uber, we will migrate from ElasticSearch to Pinot. This is the first step to make Cadence work with Pinot as the advanced visibility store. In the future, we will add support for more components like migration between ES and Pinot.
How did you test it?
Added unit test for Pinot visibility store and pinot client.
Added unit test for Pinot response comparator
Added integration test for Pinot to test the functionality
Bench test in staging environment to test performance.
Potential risks
Pinot doesn't support deletion yet, the storage may continue growing without segment compaction.
We have the pinot/es response comparator to help detect data issue during migration
Release notes
Documentation Changes