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

Remove recollect segment stats during starting datacoord #27562

Conversation

jaime0815
Copy link
Contributor

@jaime0815 jaime0815 commented Oct 9, 2023

/kind improvement
cherry pick #27410
pr: #27410

@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Oct 9, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 9, 2023

@jaime0815 Please associate the related pr of master to the body of your Pull Request. (eg. “pr: #”)

@jaime0815
Copy link
Contributor Author

pr: #27410

@jaime0815
Copy link
Contributor Author

/kind improvement

@sre-ci-robot sre-ci-robot added the kind/improvement Changes related to something improve, likes ut and code refactor label Oct 9, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 9, 2023

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

@Bennu-Li
Copy link
Contributor

Bennu-Li commented Oct 9, 2023

pr: #27410

@jaime0815 jaime0815 force-pushed the remove-recollect-segments-during-start-dc-22 branch from cfd92f4 to 1438a5f Compare October 9, 2023 11:29
@mergify
Copy link
Contributor

mergify bot commented Oct 9, 2023

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

@jaime0815 jaime0815 force-pushed the remove-recollect-segments-during-start-dc-22 branch from 1438a5f to c899bdc Compare October 9, 2023 12:29
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #27562 (c899bdc) into 2.2.0 (19ee108) will decrease coverage by 0.04%.
Report is 2 commits behind head on 2.2.0.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2.2.0   #27562      +/-   ##
==========================================
- Coverage   80.94%   80.91%   -0.04%     
==========================================
  Files         745      745              
  Lines      109226   109168      -58     
==========================================
- Hits        88416    88335      -81     
- Misses      17457    17483      +26     
+ Partials     3353     3350       -3     
Files Coverage Δ
internal/datacoord/cluster.go 100.00% <ø> (ø)
internal/datacoord/server.go 75.34% <ø> (-1.00%) ⬇️
internal/datacoord/session_manager.go 86.77% <ø> (+0.45%) ⬆️
internal/datanode/data_node.go 80.35% <100.00%> (-0.09%) ⬇️

... and 16 files with indirect coverage changes

@mergify
Copy link
Contributor

mergify bot commented Oct 9, 2023

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

@jaime0815
Copy link
Contributor Author

/run-cpu-e2e

@mergify
Copy link
Contributor

mergify bot commented Oct 10, 2023

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

@jaime0815
Copy link
Contributor Author

/run-cpu-e2e

3 similar comments
@jaime0815
Copy link
Contributor Author

/run-cpu-e2e

@jaime0815
Copy link
Contributor Author

/run-cpu-e2e

@Bennu-Li
Copy link
Contributor

/run-cpu-e2e

@mergify mergify bot added the ci-passed label Oct 10, 2023
func (node *DataNode) ResendSegmentStats(ctx context.Context, req *datapb.ResendSegmentStatsRequest) (*datapb.ResendSegmentStatsResponse, error) {
log.Info("start resending segment stats, if any",
zap.Int64("DataNode ID", Params.DataNodeCfg.GetNodeID()))
segResent := node.flowgraphManager.resendTT()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove the resendTT logic also?

@xiaofan-luan
Copy link
Collaborator

/assign @wayblink
could you help on check this too

@wayblink
Copy link
Contributor

@jaime0815
The previous Recollect logic aims to resend segmentstats after DC restart to avoid missing segmentStats updates during the crash peroid. Missing segmentStats may lead to DC never trigger these segment flush.

Now we replace segmentStats updates with RPC. Since RPC is synchronous and will cache and resend segmentStats if RPC call failed, so there is no need to Recollect?

Is my understanding correct?

@wayblink
Copy link
Contributor

/lgtm

@xiaofan-luan
Copy link
Collaborator

/approve

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaime0815, xiaofan-luan

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 42d778e into milvus-io:2.2.0 Oct 13, 2023
9 checks passed
@jaime0815 jaime0815 deleted the remove-recollect-segments-during-start-dc-22 branch October 16, 2023 02:30
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/improvement Changes related to something improve, likes ut and code refactor lgtm size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants