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

fix: Fix memory buffer error & some renaming #33850

Merged
merged 2 commits into from
Jun 21, 2024
Merged

fix: Fix memory buffer error & some renaming #33850

merged 2 commits into from
Jun 21, 2024

Conversation

wayblink
Copy link
Collaborator

@wayblink wayblink commented Jun 13, 2024 •
edited
Loading

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

rerun ut

xiaocai2333
xiaocai2333 approved these changes Jun 14, 2024
View reviewed changes
internal/datanode/compaction/clustering_compactor.go Outdated
if (remained+1)%100 == 0 {
t.rowMemoryEstimationOnce.Do(func() {
clusterBuffer.writer.IsFull()
t.rowMemoryEstimation = int64(clusterBuffer.writer.WrittenMemorySize()) / clusterBuffer.writer.GetRowNum()
Copy link
Contributor

Choose a reason for hiding this comment

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

The method may be inaccurate that estimation the row memory, if it has variable-length field, such as JSON, String, Array, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the original writing.

@codecov Codecov
Copy link

codecov bot commented Jun 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 58.10811% with 31 lines in your changes missing coverage. Please review.

Project coverage is 80.88%. Comparing base (5fac2fa) to head (57e265e).
Report is 26 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #33850      +/-   ##
==========================================
- Coverage   80.89%   80.88%   -0.02%     
==========================================
  Files        1063     1064       +1     
  Lines      136139   136264     +125     
==========================================
+ Hits       110136   110212      +76     
- Misses      21817    21851      +34     
- Partials     4186     4201      +15
Files Coverage Δ
internal/datanode/compaction/segment_writer.go 84.55% <100.00%> (+0.51%) ⬆️
internal/storage/serde.go 94.18% <100.00%> (+4.93%) ⬆️
internal/datanode/compaction/mix_compactor.go 75.71% <50.00%> (+3.80%) ⬆️
...ternal/datanode/compaction/clustering_compactor.go 63.66% <54.54%> (-0.43%) ⬇️

... and 30 files with indirect coverage changes

@mergify mergify bot added needs-dco DCO is missing in this pull request. and removed dco-passed DCO check passed. labels Jun 17, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 17, 2024

@wayblink 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.

@wayblink wayblink force-pushed the fix-major-memory branch 3 times, most recently from 1347914 to 152f09e Compare June 17, 2024 13:03
@mergify mergify bot added dco-passed DCO check passed. and removed needs-dco DCO is missing in this pull request. labels Jun 17, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 18, 2024

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

@wayblink
Fix memory buffer error & some renaming
64b3e10 
Signed-off-by: wayblink <anyang.wang@zilliz.com>
@wayblink
fix field rowcount mismatch
57e265e 
Signed-off-by: wayblink <anyang.wang@zilliz.com>
@czs007
Copy link
Contributor

czs007 commented Jun 21, 2024

/approve
/lgtm

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: czs007, wayblink, xiaocai2333

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

@czs007 czs007 added ci-passed manual-pass manually set pass before ci-passed labeled labels Jun 21, 2024
@sre-ci-robot sre-ci-robot merged commit 380d3f4 into milvus-io:master Jun 21, 2024
14 of 15 checks passed
@czs007 czs007 added the PR | need cherry-pick need cherry pick to other branches label Jun 26, 2024
yellow-shine pushed a commit to yellow-shine/milvus that referenced this pull request Jul 2, 2024
@wayblink @yellow-shine
fix: Fix memory buffer error & some renaming (milvus-io#33850)
b42c31a 
milvus-io#30633

---------

Signed-off-by: wayblink <anyang.wang@zilliz.com>
wayblink added a commit to wayblink/milvus that referenced this pull request Jul 2, 2024
@wayblink
fix: Fix memory buffer error & some renaming (milvus-io#33850)
a43ebf4 
---------

Signed-off-by: wayblink <anyang.wang@zilliz.com>
@wayblink wayblink mentioned this pull request Jul 2, 2024
Merged
wayblink added a commit to wayblink/milvus that referenced this pull request Jul 2, 2024
@wayblink
fix: Fix memory buffer error & some renaming (milvus-io#33850)
830a5bf 
---------

Signed-off-by: wayblink <anyang.wang@zilliz.com>
wayblink added a commit to wayblink/milvus that referenced this pull request Jul 2, 2024
@wayblink
fix: Fix memory buffer error & some renaming (milvus-io#33850)
8b13589 
---------

Signed-off-by: wayblink <anyang.wang@zilliz.com>
wayblink added a commit to wayblink/milvus that referenced this pull request Jul 2, 2024
@wayblink
fix: Fix memory buffer error & some renaming (milvus-io#33850)
2fa124b 
---------

Signed-off-by: wayblink <anyang.wang@zilliz.com>
@czs007 czs007 removed the PR | need cherry-pick need cherry pick to other branches label Jul 2, 2024
czs007 pushed a commit that referenced this pull request Jul 3, 2024
@wayblink @MrPresent-Han
fix: [Cherry-pick]Pick major compaction fixs and optimizations (#34360)
c62bf8a 
This PR cherry-picks the following commits:

- fix: sync partitiion stats blocking balance task #33742
- fix: Fix meta prefix overlap bug #33830
- fix: Small fixs of major compaction #33929 
- fix: Fix memory buffer error & some renaming #33850
- fix: sync part stats task cannot be finished #34027 
- Add an option to enable/disable vector field clustering key #34097
- fix: fix error ignore in compactor #34169
- fix:load major compaction partial result #34052
- Use new stream segment reader in clustering compaction #34232

issue: #30633
pr: #33742 #33830 #33929 #33850 #34027 #34097 #34169 #34052 #34232

---------

Signed-off-by: MrPresent-Han <chun.han@zilliz.com>
Signed-off-by: wayblink <anyang.wang@zilliz.com>
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: Chun Han <116052805+MrPresent-Han@users.noreply.github.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaocai2333 xiaocai2333 xiaocai2333 approved these changes

@sunby sunby Awaiting requested review from sunby

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

Assignees

@czs007 czs007

Labels
approved ci-passed dco-passed DCO check passed. kind/bug Issues or changes related a bug 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.

None yet
4 participants
@wayblink @xiaocai2333 @czs007 @sre-ci-robot