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 ts support for iterator(#22718) #36572

Merged
merged 1 commit into from
Oct 16, 2024
Merged

enhance: add ts support for iterator(#22718) #36572

merged 1 commit into from
Oct 16, 2024

Conversation

MrPresent-Han
Copy link
Contributor

related: #22718

@sre-ci-robot sre-ci-robot added size/S Denotes a PR that changes 10-29 lines. area/dependency Pull requests that update a dependency file labels Sep 27, 2024
@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Sep 27, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 27, 2024

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 27, 2024

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

@sre-ci-robot sre-ci-robot added size/L Denotes a PR that changes 100-499 lines. and removed size/S Denotes a PR that changes 10-29 lines. labels Sep 29, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 29, 2024

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 29, 2024

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 29, 2024

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 29, 2024

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

@MrPresent-Han MrPresent-Han force-pushed the refine-iterator branch 2 times, most recently from 3072734 to d3eadd1 Compare September 30, 2024 06:47
@mergify mergify bot added the ci-passed label Sep 30, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 30, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.32258% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.50%. Comparing base (aa904be) to head (2a4025a).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
internal/proxy/task_search.go 77.27% 3 Missing and 2 partials ⚠️
internal/proxy/search_util.go 96.77% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #36572      +/-   ##
==========================================
+ Coverage   72.51%   81.50%   +8.98%     
==========================================
  Files        1308     1308              
  Lines      156196   156250      +54     
==========================================
+ Hits       113271   127354   +14083     
+ Misses      37806    23767   -14039     
- Partials     5119     5129      +10
Files with missing lines Coverage Δ
internal/proxy/task_query.go 82.21% <100.00%> (+0.24%) ⬆️
internal/querynodev2/delegator/delegator.go 86.89% <100.00%> (+0.28%) ⬆️
internal/proxy/search_util.go 81.46% <96.77%> (+0.32%) ⬆️
internal/proxy/task_search.go 78.10% <77.27%> (+0.66%) ⬆️

... and 255 files with indirect coverage changes

@mergify mergify bot added ci-passed and removed ci-passed labels Sep 30, 2024
@sre-ci-robot sre-ci-robot added size/XL Denotes a PR that changes 500-999 lines. and removed size/L Denotes a PR that changes 100-499 lines. labels Oct 9, 2024
@mergify mergify bot removed the ci-passed label Oct 9, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 9, 2024

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 9, 2024

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

@mergify mergify bot added the ci-passed label Oct 9, 2024
@mergify mergify bot added ci-passed and removed ci-passed labels Oct 11, 2024
@mergify mergify bot added ci-passed and removed ci-passed labels Oct 12, 2024
@tedxu
Copy link
Contributor

tedxu commented Oct 14, 2024

/lgtm

@sre-ci-robot sre-ci-robot removed the lgtm label Oct 15, 2024
@mergify mergify bot removed the ci-passed label Oct 15, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 15, 2024

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

@MrPresent-Han
Copy link
Contributor Author

/run-cpu-e2e

@mergify mergify bot added the ci-passed label Oct 15, 2024
congqixia
congqixia reviewed Oct 16, 2024
View reviewed changes
internal/proxy/search_util.go Outdated
GroupStrictSize: groupStrictSize,
},
offset: offset,
isIterator: isIterator == "True",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "True" const is friendly to language that boolean const does not start with capital T

congqixia
congqixia reviewed Oct 16, 2024
View reviewed changes
internal/proxy/task_query.go
Comment on lines +552 to +555
if t.queryParams.isIterator && t.request.GetGuaranteeTimestamp() == 0 {
// first page for iteration, need to set up sessionTs for iterator
t.result.SessionTs = t.BeginTs()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe all iterator query shall return session ts so that it's easier for sdk to implement iterator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the first page for iterator, it's right for all sdks:
milvus only set up sessionTs for sdk-side iterator for the first time,
since second page, this sessinTs is set by iterator into guaranteeTs

@mergify mergify bot added ci-passed and removed ci-passed labels Oct 16, 2024
congqixia
congqixia approved these changes Oct 16, 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.

/lgtm

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: congqixia, MrPresent-Han

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 903450f into milvus-io:master Oct 16, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@congqixia congqixia congqixia approved these changes

@czs007 czs007 Awaiting requested review from czs007

Assignees

@tedxu tedxu

@congqixia congqixia

Labels
approved area/dependency Pull requests that update a dependency file area/test ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm sig/testing size/XL Denotes a PR that changes 500-999 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MrPresent-Han @tedxu @sre-ci-robot @congqixia