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: separate serializer logic from sync task #29413

Merged
merged 6 commits into from
Dec 26, 2023
Merged

enhance: separate serializer logic from sync task #29413

merged 6 commits into from
Dec 26, 2023

Conversation

congqixia
Copy link
Contributor

See also #27675

Since serialization segment buffer does not related to sync manager can
shall be done before submit into sync manager. So that the pk statistic
file could be more accurate and reduce complex logic inside sync
manager.

@sre-ci-robot sre-ci-robot added size/XXL Denotes a PR that changes 1000+ lines. area/compilation approved labels Dec 21, 2023
@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Dec 21, 2023
@congqixia congqixia mentioned this pull request Dec 21, 2023
Closed
33 tasks
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 21, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 21, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 21, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 21, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 21, 2023

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 21, 2023

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

1 similar comment
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@congqixia congqixia force-pushed the split_serilizer_sync_task branch from 45a1fa7 to dd44c79 Compare December 22, 2023 09:22
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

2 similar comments
@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 22, 2023

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

@congqixia
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 23, 2023

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

@congqixia
Stash change
a16bfec 
Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@congqixia
enhance: separate serializer logic from sync task
f8167ce 
See also milvus-io#27675

Since serialization segment buffer does not related to sync manager can
shall be done before submit into sync manager. So that the pk statistic
file could be more accurate and reduce complex logic inside sync
manager.

Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@congqixia
Fix metacache unit test
96a22da 
Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@congqixia
Resolve conflict & statslog field id not match
82cefa6 
Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@congqixia
Fix unit test expectation after logic changed
43d3d7c 
Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@congqixia congqixia force-pushed the split_serilizer_sync_task branch from e595afa to 43d3d7c Compare December 25, 2023 03:54
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 25, 2023

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

@congqixia
fix wrong parameter used for inCoded.Serialize
4dacfbc 
Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@codecov Codecov
Copy link

codecov bot commented Dec 25, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (13988cb) 80.77% compared to head (4dacfbc) 80.86%.
Report is 11 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #29413      +/-   ##
==========================================
+ Coverage   80.77%   80.86%   +0.09%     
==========================================
  Files         913      916       +3     
  Lines      126750   126802      +52     
==========================================
+ Hits       102376   102537     +161     
+ Misses      21029    20946      -83     
+ Partials     3345     3319      -26
Files Coverage Δ
internal/datanode/metacache/actions.go 91.56% <100.00%> (ø)
internal/datanode/metacache/bloom_filter_set.go 90.47% <100.00%> (+1.00%) ⬆️
internal/datanode/syncmgr/options.go 100.00% <100.00%> (ø)
internal/datanode/syncmgr/serializer.go 100.00% <100.00%> (ø)
internal/datanode/writebuffer/bf_write_buffer.go 87.75% <100.00%> (+1.08%) ⬆️
internal/datanode/writebuffer/l0_write_buffer.go 68.00% <100.00%> (+1.80%) ⬆️
internal/datanode/data_sync_service.go 63.96% <0.00%> (ø)
internal/datanode/syncmgr/taskv2.go 61.78% <50.00%> (-2.92%) ⬇️
internal/datanode/syncmgr/meta_writer.go 80.44% <50.00%> (-0.22%) ⬇️
internal/datanode/writebuffer/write_buffer.go 93.37% <95.74%> (+11.22%) ⬆️
... and 3 more

... and 20 files with indirect coverage changes

@mergify mergify bot added the ci-passed label Dec 25, 2023
XuanYang-cn
XuanYang-cn approved these changes Dec 26, 2023
View reviewed changes
Copy link
Contributor

@XuanYang-cn XuanYang-cn left a comment

Choose a reason for hiding this comment

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

/lgtm

internal/datanode/syncmgr/task.go Show resolved Hide resolved
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: congqixia, XuanYang-cn

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 277849a into milvus-io:master Dec 26, 2023
14 checks passed
congqixia added a commit to congqixia/milvus that referenced this pull request Dec 26, 2023
@congqixia
enhance: fix serialization record span & flushed buffer size metrics
e81a3bf 
See also milvus-io#27675 milvus-io#29413

Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@congqixia congqixia mentioned this pull request Dec 26, 2023
Merged
sre-ci-robot pushed a commit that referenced this pull request Dec 27, 2023
@congqixia
enhance: fix serialization record span & flushed buffer size metrics (#…
f6cff25 
…29482)

See also #27675 #29413

---------

Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@congqixia congqixia deleted the split_serilizer_sync_task branch December 29, 2023 03:12
@congqixia congqixia mentioned this pull request Jan 3, 2024
Merged
congqixia added a commit to congqixia/milvus that referenced this pull request Jan 4, 2024
@congqixia
fix: serializer shall bypass L0 segment merge stats step
c60db8c 
See also milvus-io#27675
Fix logic problem introduced by milvus-io#29413, which is serializer tries to
merge statslog list while level segments do not have statslog. This
shall result returning error. `writeBufferBase` ignores this error but
it shall only ignore `ErrSegmentNotFound`.

This PR add logic checking segment level before execution of merging
statslog list. And add error type check for getSyncTask failure.

Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
sre-ci-robot pushed a commit that referenced this pull request Jan 4, 2024
@congqixia
fix: serializer shall bypass L0 segment merge stats step (#29636)
79c06c5 
See also #27675
Fix logic problem introduced by #29413, which is serializer tries to
merge statslog list while level segments do not have statslog. This
shall result returning error. `writeBufferBase` ignores this error but
it shall only ignore `ErrSegmentNotFound`.

This PR add logic checking segment level before execution of merging
statslog list. And add error type check for getSyncTask failure.

Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuanYang-cn XuanYang-cn XuanYang-cn approved these changes

@sunby sunby Awaiting requested review from sunby

@xiaofan-luan xiaofan-luan Awaiting requested review from xiaofan-luan

Assignees

@XuanYang-cn XuanYang-cn

Labels
approved area/compilation ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm size/XXL Denotes a PR that changes 1000+ lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@congqixia @sre-ci-robot @XuanYang-cn