Move ETCD client into Session #27069

filip-halt · 2023-09-13T22:58:22Z

This PR is part of a multi-part effort to remove kv dependencies from being hardcoded throughout files.

In this specific pr we are removing the requirement to pass in etcd and metarootpath into session. The ability is still there so functions in cmd, backup, and tests can still function.

#26694

mergify · 2023-09-13T22:58:58Z

@filip-halt Please associate the related issue to the body of your Pull Request. (eg. “issue: #”)

mergify · 2023-09-13T23:11:34Z

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

mergify · 2023-09-13T23:14:31Z

@filip-halt ut workflow job failed, comment rerun ut can trigger the job again.

filip-halt · 2023-09-13T23:55:06Z

@yiwangdr

internal/util/sessionutil/session_util.go

internal/util/dependency/kv/kv_client_handler.go

yiwangdr · 2023-09-14T21:33:02Z

internal/util/dependency/kv/kv_client_handler.go

+	if reuser.Client == nil {
+		var err error
+		reuser.Client, err = NewETCDClient()
+		if err != nil {
+			panic(err)
+		}
+		reuser.Rootpath = GetETCDRootPath()
+	}


If we move client creation code into NewETCDReuser(), can we get rid of mu ?

The issue that comes up here is that the configs don't necessarily exist yet or might need to be modified so we cant immediately try to create the client but we rather have to wait for the first call. Im debating having an etcdFactory created at datacoord creation and then pass that into the session_util. Once the rest of the factory changes are done we can swap the etcdFactory with the overarching dependency factory.

The next step can remove the lock by bringing in the clients to the factory.

internal/util/dependency/kv/kv_client_handler.go

mergify · 2023-09-14T23:35:14Z

@filip-halt, please be sure the pr should only have one commit, check https://github.com/milvus-io/milvus/blob/master/CODE_REVIEW.md for more details.

mergify · 2023-09-18T17:12:06Z

@filip-halt, please be sure the pr should only have one commit, check https://github.com/milvus-io/milvus/blob/master/CODE_REVIEW.md for more details.

mergify · 2023-09-18T18:49:25Z

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

codecov · 2023-09-18T21:42:11Z

Codecov Report

Merging #27069 (c8d7108) into master (f9c6302) will increase coverage by 0.01%.
Report is 4 commits behind head on master.
The diff coverage is 91.79%.

@@            Coverage Diff             @@
##           master   #27069      +/-   ##
==========================================
+ Coverage   81.77%   81.78%   +0.01%     
==========================================
  Files         821      821              
  Lines      116389   116435      +46     
==========================================
+ Hits        95175    95227      +52     
+ Misses      18057    18051       -6     
  Partials     3157     3157

Files	Coverage Δ
internal/datanode/data_node.go	`76.54% <100.00%> (ø)`
internal/distributed/datacoord/client/client.go	`93.92% <100.00%> (ø)`
internal/distributed/datanode/service.go	`81.94% <100.00%> (ø)`
internal/distributed/proxy/service.go	`79.97% <100.00%> (-0.70%)`	⬇️
internal/distributed/querycoord/client/client.go	`93.38% <100.00%> (ø)`
internal/distributed/querycoord/service.go	`73.63% <100.00%> (ø)`
internal/distributed/rootcoord/client/client.go	`94.06% <100.00%> (ø)`
internal/distributed/rootcoord/service.go	`79.92% <100.00%> (ø)`
internal/indexnode/indexnode.go	`79.90% <100.00%> (ø)`
internal/proxy/proxy.go	`68.01% <100.00%> (ø)`
... and 19 more

... and 13 files with indirect coverage changes

sre-ci-robot · 2023-10-24T21:19:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: filip-halt, jiaoew1991, yiwangdr

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:

~~OWNERS~~ [jiaoew1991]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jiaoew1991 · 2023-10-24T21:19:40Z

overall LGTM, need to fix the conflicts

mergify · 2023-10-25T17:38:45Z

@filip-halt ut workflow job failed, comment rerun ut can trigger the job again.

mergify · 2023-10-25T19:06:13Z

@filip-halt, please be sure the pr should only have one commit, check https://github.com/milvus-io/milvus/blob/master/CODE_REVIEW.md for more details.

mergify · 2023-10-25T19:15:40Z

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

filip-halt · 2023-10-25T20:39:13Z

/run-cpu-e2e

mergify · 2023-10-26T00:02:13Z

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

filip-halt · 2023-10-26T17:21:20Z

rerun ut

Signed-off-by: Filip Haltmayer <[email protected]>

jiaoew1991 · 2023-10-26T23:34:16Z

/lgtm

pr: [milvus-io#28971](milvus-io#27069) Signed-off-by: Filip Haltmayer <[email protected]> Signed-off-by: MrPresent-Han <[email protected]>

relate: #26694 pr: #27069 Signed-off-by: Filip Haltmayer <[email protected]> Signed-off-by: MrPresent-Han <[email protected]> Co-authored-by: Filip Haltmayer <[email protected]>

sre-ci-robot requested review from aoiasd and bigsheeper September 13, 2023 22:58

sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Sep 13, 2023

mergify bot added the dco-passed DCO check passed. label Sep 13, 2023

mergify bot added do-not-merge/missing-related-issue and removed do-not-merge/missing-related-issue labels Sep 13, 2023

filip-halt force-pushed the session_etcd branch from e5de7c0 to 9ac2c9a Compare September 13, 2023 23:04

filip-halt force-pushed the session_etcd branch from 9ac2c9a to 291ed48 Compare September 13, 2023 23:22

yiwangdr reviewed Sep 14, 2023

View reviewed changes

internal/util/sessionutil/session_util.go Outdated Show resolved Hide resolved

yiwangdr reviewed Sep 14, 2023

View reviewed changes

mergify bot added the needs-rebase label Sep 14, 2023

filip-halt force-pushed the session_etcd branch from a60592d to fcc95ac Compare September 14, 2023 23:38

mergify bot removed the needs-rebase label Sep 14, 2023

mergify bot added the needs-rebase label Sep 18, 2023

sre-ci-robot added area/test sig/testing test/integration integration test labels Sep 18, 2023

filip-halt force-pushed the session_etcd branch from 022adf2 to 2ff9ad8 Compare September 18, 2023 20:45

mergify bot removed the needs-rebase label Sep 18, 2023

mergify bot added the ci-passed label Sep 18, 2023

filip-halt force-pushed the session_etcd branch from 2ff9ad8 to 447b51a Compare September 18, 2023 21:58

mergify bot removed the ci-passed label Sep 18, 2023

sre-ci-robot added the approved label Oct 24, 2023

filip-halt force-pushed the session_etcd branch from 461f90c to 9fdb7c9 Compare October 25, 2023 17:13

mergify bot removed the ci-passed label Oct 25, 2023

filip-halt force-pushed the session_etcd branch from 9fdb7c9 to e2e57e6 Compare October 25, 2023 17:15

mergify bot added the needs-rebase label Oct 25, 2023

mergify bot added the ci-passed label Oct 25, 2023

filip-halt force-pushed the session_etcd branch from b2b2093 to d9d21f5 Compare October 25, 2023 22:35

mergify bot removed ci-passed needs-rebase labels Oct 25, 2023

Moving etcd client into session

c8d7108

Signed-off-by: Filip Haltmayer <[email protected]>

filip-halt force-pushed the session_etcd branch from d9d21f5 to c8d7108 Compare October 26, 2023 17:22

mergify bot added the ci-passed label Oct 26, 2023

sre-ci-robot assigned jiaoew1991 Oct 26, 2023

sre-ci-robot added the lgtm label Oct 26, 2023

sre-ci-robot merged commit 6b1a106 into milvus-io:master Oct 26, 2023
12 checks passed

MrPresent-Han mentioned this pull request Dec 5, 2023

enhance: [Cherry-pick] Moving etcd client into session (#27069) #28996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ETCD client into Session #27069

Move ETCD client into Session #27069

filip-halt commented Sep 13, 2023 •

edited

Loading

mergify bot commented Sep 13, 2023

mergify bot commented Sep 13, 2023

mergify bot commented Sep 13, 2023

filip-halt commented Sep 13, 2023

yiwangdr Sep 14, 2023

filip-halt Sep 14, 2023

filip-halt Sep 19, 2023

mergify bot commented Sep 14, 2023

mergify bot commented Sep 18, 2023

mergify bot commented Sep 18, 2023

codecov bot commented Sep 18, 2023 •

edited

Loading

sre-ci-robot commented Oct 24, 2023

jiaoew1991 commented Oct 24, 2023

mergify bot commented Oct 25, 2023

mergify bot commented Oct 25, 2023

mergify bot commented Oct 25, 2023

filip-halt commented Oct 25, 2023

mergify bot commented Oct 26, 2023

filip-halt commented Oct 26, 2023

jiaoew1991 commented Oct 26, 2023

Move ETCD client into Session #27069

Move ETCD client into Session #27069

Conversation

filip-halt commented Sep 13, 2023 • edited Loading

mergify bot commented Sep 13, 2023

mergify bot commented Sep 13, 2023

mergify bot commented Sep 13, 2023

filip-halt commented Sep 13, 2023

yiwangdr Sep 14, 2023

Choose a reason for hiding this comment

filip-halt Sep 14, 2023

Choose a reason for hiding this comment

filip-halt Sep 19, 2023

Choose a reason for hiding this comment

mergify bot commented Sep 14, 2023

mergify bot commented Sep 18, 2023

mergify bot commented Sep 18, 2023

codecov bot commented Sep 18, 2023 • edited Loading

Codecov Report

sre-ci-robot commented Oct 24, 2023

jiaoew1991 commented Oct 24, 2023

mergify bot commented Oct 25, 2023

mergify bot commented Oct 25, 2023

mergify bot commented Oct 25, 2023

filip-halt commented Oct 25, 2023

mergify bot commented Oct 26, 2023

filip-halt commented Oct 26, 2023

jiaoew1991 commented Oct 26, 2023

filip-halt commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 18, 2023 •

edited

Loading