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.

Sign up for GitHub

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

Jump to bottom

enhance: add search params in search request in restful #36304

Merged
merged 1 commit into from
Sep 23, 2024
Merged

enhance: add search params in search request in restful #36304

merged 1 commit into from
Sep 23, 2024

Conversation

smellthemoon
Copy link
Contributor

@smellthemoon smellthemoon commented Sep 14, 2024 •
edited
Loading

@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Sep 14, 2024
@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Sep 14, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 14, 2024

@smellthemoon Please associate the related issue to the body of your Pull Request. (eg. “issue: #”)

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 14, 2024

@smellthemoon E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

PwzXxm
PwzXxm reviewed Sep 14, 2024
View reviewed changes
internal/distributed/proxy/httpserver/request_v2.go Outdated
MetricType string `json:"metricType"`
Params map[string]interface{} `json:"params"`
IgnoreGrowing bool `json:"ignore_growing"`
PageRetainOrder bool `json:"page_retain_order"`
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be in SearchParams->Params, not outside.

@smellthemoon smellthemoon force-pushed the search-params branch 2 times, most recently from 41d8474 to 66b8649 Compare September 18, 2024 02:04
@smellthemoon
Copy link
Contributor Author

related with #36321

@codecov Codecov
Copy link

codecov bot commented Sep 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.67%. Comparing base (89397d1) to head (1e617c8).
Report is 10 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #36304       +/-   ##
===========================================
- Coverage   82.36%   69.67%   -12.69%     
===========================================
  Files        1276      277      -999     
  Lines      150677    22966   -127711     
===========================================
- Hits       124105    16002   -108103     
+ Misses      21603     6964    -14639     
+ Partials     4969        0     -4969

see 999 files with indirect coverage changes

PwzXxm
PwzXxm reviewed Sep 18, 2024
View reviewed changes
internal/distributed/proxy/httpserver/handler_v2.go Outdated
OutputFields: httpReq.OutputFields,
PartitionNames: httpReq.PartitionNames,
}
if httpReq.ConsistencyLevel != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to extract this and reuse at below line 997

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 18, 2024

@smellthemoon E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 18, 2024

@smellthemoon E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 18, 2024

@smellthemoon E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 18, 2024

@smellthemoon E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 19, 2024

@smellthemoon E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 19, 2024

@smellthemoon go-sdk check failed, comment rerun go-sdk can trigger the job again.

@smellthemoon
Copy link
Contributor Author

/run-cpu-e2e

@smellthemoon
Copy link
Contributor Author

rerun go-sdk

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 19, 2024

@smellthemoon E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 19, 2024

@smellthemoon go-sdk check failed, comment rerun go-sdk can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 19, 2024

@smellthemoon E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify mergify bot added the ci-passed label Sep 19, 2024
@PwzXxm
Copy link
Contributor

PwzXxm commented Sep 20, 2024

/lgtm

congqixia
congqixia approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@congqixia congqixia left a comment

Choose a reason for hiding this comment

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

/approve

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: congqixia, smellthemoon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot merged commit 6e880d1 into milvus-io:master Sep 23, 2024
14 of 16 checks passed
@smellthemoon
Copy link
Contributor Author

@zhuwenxing could you help to update the related tests in restful?

zhuwenxing
zhuwenxing reviewed Sep 23, 2024
View reviewed changes
internal/distributed/proxy/httpserver/request_v2.go
// not use metricType any more, just for compatibility
MetricType string `json:"metricType"`
Params map[string]interface{} `json:"params"`
IgnoreGrowing bool `json:"ignore_growing"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to maintain consistency in naming conventions around here?
metric_type --> metricType
so ignore_growing --> ignoreGrowing?

smellthemoon pushed a commit to smellthemoon/milvus that referenced this pull request Nov 14, 2024
enhance: add search params in search request in restful(milvus-io#36304)
3e63209 
Signed-off-by: lixinguo <[email protected]>
@smellthemoon smellthemoon mentioned this pull request Nov 14, 2024
Merged
smellthemoon pushed a commit to smellthemoon/milvus that referenced this pull request Nov 14, 2024
enhance: add search params in search request in restful(milvus-io#36304)
cb49ed9 
Signed-off-by: lixinguo <[email protected]>
smellthemoon pushed a commit to smellthemoon/milvus that referenced this pull request Nov 15, 2024
enhance: add search params in search request in restful(milvus-io#36304)
c127271 
Signed-off-by: lixinguo <[email protected]>
smellthemoon pushed a commit to smellthemoon/milvus that referenced this pull request Nov 15, 2024
enhance: add search params in search request in restful(milvus-io#36304)
d13b701 
Signed-off-by: lixinguo <[email protected]>
sre-ci-robot pushed a commit that referenced this pull request Nov 15, 2024
@smellthemoon @zhuwenxing
enhance: add search params in search request in restful(#36304) (#37673)
b3e6482 
pr: #36304 
pr: #36714 
pr: #36448

---------

Signed-off-by: lixinguo <[email protected]>
Co-authored-by: lixinguo <[email protected]>
Co-authored-by: zhuwenxing <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhuwenxing zhuwenxing zhuwenxing left review comments

@PwzXxm PwzXxm PwzXxm left review comments

@congqixia congqixia congqixia approved these changes

@czs007 czs007 Awaiting requested review from czs007

@yanliang567 yanliang567 Awaiting requested review from yanliang567

Assignees

@PwzXxm PwzXxm

Labels
approved area/test ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm sig/testing size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smellthemoon @PwzXxm @sre-ci-robot @zhuwenxing @congqixia