Skip to content
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

feat(stencil): add support for search API #92

Merged
merged 3 commits into from
Feb 2, 2022
Merged

Conversation

ramey
Copy link
Member

@ramey ramey commented Jan 21, 2022

  • add RPC call Search
  • define messages SearchRequest, SearchResponse and SearchMeta

@ramey ramey requested review from h4rikris and ravisuhag January 21, 2022 03:35
- add RPC call `SearchSchemas`
- define messages `SearchSchemasRequest` and `SearchSchemasResponse`
* modify `SearchSchemas` API to global `Search` API
* modify `SearchSchemasRequest`
* modify `SearchSchemasResponse` to include search `meta`
string schema_id = 2;
int32 version_id = 3;
string query = 4;
bool all = 5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ramey What is the role of all here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all returns matches from all the versions, by default only the matches from the latest versions are returned

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense. Is there some better name we can think of for this? At first look I got confused that it is related to pagination,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does scan_all_versions sound?

Copy link
Member

@h4rikris h4rikris Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what we need is oneof between version_id and all fields. User can't set both fields together right? If we do allow setting two fields then, which field should take precedence?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is while processing the value in the application. In case no value has been specified as it is a bool it defaults to false and there is no way to find out whether it is explicitly given by the user or default bool value.
Agreed with oneof, will change that.

Copy link
Member

@ravisuhag ravisuhag Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense. In that case, another name of all could be history/past as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ramey @harikrishnakanchi lets go with history then?

Copy link
Member Author

@ramey ramey Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ravisuhag I am thinking of going with history

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ramey cool.

* change `all` param in `SearchRequest` to `history`
* use oneof for `version_id` and `history`

code review by @ravisuhag and @harikrishnakanchi
@ravisuhag ravisuhag self-requested a review February 1, 2022 12:42
@ravisuhag ravisuhag merged commit 3309ff0 into main Feb 2, 2022
@ravisuhag ravisuhag deleted the stencil-search branch February 2, 2022 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants