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: Adding a generic stream payload reader #30682

Merged
merged 3 commits into from
Feb 21, 2024
Merged

enhance: Adding a generic stream payload reader #30682

merged 3 commits into from
Feb 21, 2024

Conversation

tedxu
Copy link
Contributor

@tedxu tedxu commented Feb 20, 2024

See: #30404

@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Feb 20, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 20, 2024

@tedxu Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco.

@mergify mergify bot added needs-dco DCO is missing in this pull request. kind/enhancement Issues or changes related to enhancement labels Feb 20, 2024
@tedxu
Copy link
Contributor Author

tedxu commented Feb 20, 2024

Benchmark result on my laptop:

goos: darwin
goarch: arm64
pkg: github.com/milvus-io/milvus/internal/storage
BenchmarkDefaultReader-64        	       1	24722705375 ns/op	115230256 B/op	   89862 allocs/op
BenchmarkDataSetReader-64        	1000000000	         0.2998 ns/op	       0 B/op	       0 allocs/op
BenchmarkArrowRecordReader-64    	1000000000	         0.3556 ns/op	       0 B/op	       0 allocs/op
PASS

The result shows that DataSetReader from #30407 and ArrowRecordReader both require no additional memory.

@tedxu tedxu force-pushed the enhance/generic_stream_reader branch from 20a21ce to 342e0ab Compare February 20, 2024 04:15
@mergify mergify bot added dco-passed DCO check passed. and removed needs-dco DCO is missing in this pull request. labels Feb 20, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 20, 2024

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 20, 2024

@tedxu ut workflow job failed, comment rerun ut can trigger the job again.

@tedxu
Code style fixes.
4d15cb9 
Signed-off-by: Ted Xu <[email protected]>
@congqixia congqixia changed the title enhance: Adding a generic stream payload reader. enhance: Adding a generic stream payload reader Feb 20, 2024
aoiasd
aoiasd reviewed Feb 20, 2024
View reviewed changes
internal/storage/data_codec.go
@@ -1007,43 +1007,38 @@ func (deleteCodec *DeleteCodec) Deserialize(blobs []*Blob) (partitionID UniqueID
if err != nil {
return InvalidUniqueID, InvalidUniqueID, nil, err
}
defer binlogReader.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Defer close will cause that Reader close after all blobs deserialize finished, but old codes support close Reader if a blob deserialize finished.

@codecov Codecov
Copy link

codecov bot commented Feb 20, 2024 •
edited
Loading

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (43e8cd5) 80.43% compared to head (1dc2e2a) 80.41%.
Report is 21 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #30682      +/-   ##
==========================================
- Coverage   80.43%   80.41%   -0.02%     
==========================================
  Files         966      966              
  Lines      136767   136748      -19     
==========================================
- Hits       110006   109971      -35     
- Misses      23037    23048      +11     
- Partials     3724     3729       +5
Files Coverage Δ
internal/storage/data_codec.go 67.94% <81.81%> (+0.42%) ⬆️
internal/storage/payload_reader.go 78.72% <40.00%> (-4.07%) ⬇️

... and 33 files with indirect coverage changes

@aoiasd
Copy link
Contributor

aoiasd commented Feb 21, 2024

/lgtm

@yanliang567 yanliang567 added ci-passed manual-pass manually set pass before ci-passed labeled labels Feb 21, 2024
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 12acaf3 into milvus-io:master Feb 21, 2024
13 of 14 checks passed
@tedxu tedxu deleted the enhance/generic_stream_reader branch April 23, 2024 09:55
@tedxu tedxu mentioned this pull request Oct 29, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aoiasd aoiasd aoiasd left review comments

@congqixia congqixia congqixia approved these changes

@xiaocai2333 xiaocai2333 Awaiting requested review from xiaocai2333

@XuanYang-cn XuanYang-cn Awaiting requested review from XuanYang-cn

Assignees

@aoiasd aoiasd

Labels
approved ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm manual-pass manually set pass before ci-passed labeled 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
@tedxu @aoiasd @sre-ci-robot @congqixia @yanliang567