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.

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

fix: Update segment compactTo when compactTo segment is compacted #28755

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

congqixia
Copy link
Contributor

Related to #28736 #28748
See also #27675
Previous PR: #28646

This PR fixes SegmentNotFound issue when compaction happens multiple times and the buffer of first generation segment is sync due to stale policy

Now the CompactSegments API of metacache shall update the compactTo field of segmentInfo if the compactTo segment is also compacted to keep the bloodline clean

Also, add the CompactedSegment SyncPolicy to sync the compacted segment asap to keep metacache clean

Now the SyncPolicy is an interface instead of a function type so that when it selects some segments to sync, we colud log the reason and target segment

Related to milvus-io#28736 milvus-io#28748
See also milvus-io#27675
Previous PR: milvus-io#28646

This PR fixes `SegmentNotFound` issue when compaction happens multiple
times and the buffer of first generation segment is sync due to stale
policy

Now the `CompactSegments` API of metacache shall update the compactTo
field of segmentInfo if the compactTo segment is also compacted to keep
the bloodline clean

Also, add the `CompactedSegment` SyncPolicy to sync the compacted
segment asap to keep metacache clean

Now the `SyncPolicy` is an interface instead of a function type so that
when it selects some segments to sync, we colud log the reason and
target segment

Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Nov 27, 2023
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: congqixia

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

@mergify mergify bot added dco-passed DCO check passed. kind/bug Issues or changes related a bug labels Nov 27, 2023
Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Merging #28755 (61971e0) into master (911a915) will increase coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #28755      +/-   ##
==========================================
+ Coverage   82.24%   82.26%   +0.01%     
==========================================
  Files         856      856              
  Lines      119817   119837      +20     
==========================================
+ Hits        98539    98578      +39     
+ Misses      17971    17956      -15     
+ Partials     3307     3303       -4     
Files Coverage Δ
internal/datanode/metacache/meta_cache.go 100.00% <100.00%> (ø)
internal/datanode/writebuffer/options.go 74.19% <100.00%> (+1.77%) ⬆️
internal/datanode/writebuffer/sync_policy.go 94.00% <100.00%> (+2.33%) ⬆️
internal/datanode/writebuffer/write_buffer.go 78.49% <100.00%> (+0.14%) ⬆️

... and 9 files with indirect coverage changes

@mergify mergify bot added the ci-passed label Nov 27, 2023
@XuanYang-cn
Copy link
Contributor

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved ci-passed dco-passed DCO check passed. kind/bug Issues or changes related a bug lgtm size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants