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

Enable etcd auto compaction in Milvus's Ubuntu package #16723

Conversation

schuberttobias
Copy link

@schuberttobias schuberttobias commented Apr 28, 2022

Issue: #16511

@smeikalr: FYI, I've set up this PR based on our own fix.

@sre-ci-robot
Copy link
Contributor

Welcome @schuberttobias! It looks like this is your first PR to milvus-io/milvus 🎉

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: schuberttobias
To complete the pull request process, please assign congqixia after the PR has been reviewed.
You can assign the PR to them by writing /assign @congqixia in a comment when ready.

The full list of commands accepted by this bot can be found 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 added the size/XS Denotes a PR that changes 0-9 lines. label Apr 28, 2022
@mergify mergify bot added needs-dco DCO is missing in this pull request. do-not-merge/missing-related-issue labels Apr 28, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 28, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Apr 28, 2022

@schuberttobias, 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 mergify bot added the needs-rebase label Apr 28, 2022
@@ -27,4 +27,4 @@ StandardError=inherit
Restart=always

# Start main service
ExecStart=/usr/bin/milvus-etcd --data-dir /etc/milvus/etcd-data
Copy link
Author

@schuberttobias schuberttobias Apr 28, 2022

Choose a reason for hiding this comment

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

@LoveEachDay: What is the correct value for --data-dir?

In the master branch it is --data-dir /etc/milvus/etcd-data.
But when I apt-get install milvus==2.0.2-1 the content of /etc/systemd/system/milvus-etcd.service is different: --data-dir /var/lib/milvus/etcd-data.

image

/var/lib/milvus/etcd-data/ exists and holds files, /etc/milvus/etcd-data does not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

We change the default data directory for milvus 2.0.2.
/cc @Bennu-Li

Copy link
Contributor

Choose a reason for hiding this comment

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

For ubuntu the default data directory for etcd is /var/lib/milvus/ectd-data, and for centos related the default directory for etcd is /etc/milvus/etcd-data .

And for now we don't put the service config for ubuntu into milvus repo.

@schuberttobias schuberttobias force-pushed the EtcdCompactionUbuntuPkg branch from 3c89a5f to 5d90211 Compare April 28, 2022 19:04
@mergify mergify bot removed the needs-rebase label Apr 28, 2022
@schuberttobias schuberttobias force-pushed the EtcdCompactionUbuntuPkg branch from 5d90211 to 7ed3616 Compare April 28, 2022 19:06
@mergify mergify bot added dco-passed DCO check passed. and removed needs-dco DCO is missing in this pull request. do-not-merge/missing-related-issue labels Apr 28, 2022
@schuberttobias
Copy link
Author

I think I've applied all changes as requested and explained by your CI.

Do you have a branch name convention? I did not find one and emulated @xiaofan-luan 's style

@mergify mergify bot added the ci-passed label Apr 28, 2022
@sre-ci-robot sre-ci-robot requested a review from Bennu-Li May 10, 2022 03:14
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #16723 (7ed3616) into master (cf08b5a) will decrease coverage by 0.43%.
The diff coverage is n/a.

❗ Current head 7ed3616 differs from pull request most recent head f84a684. Consider uploading reports for the commit f84a684 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16723      +/-   ##
==========================================
- Coverage   81.16%   80.72%   -0.44%     
==========================================
  Files         457      440      -17     
  Lines       72871    69524    -3347     
==========================================
- Hits        59144    56122    -3022     
+ Misses      10988    10731     -257     
+ Partials     2739     2671      -68     
Impacted Files Coverage Δ
internal/core/src/index/ScalarIndexSort.h 33.33% <0.00%> (-33.34%) ⬇️
internal/core/src/segcore/SegmentInterface.cpp 79.03% <0.00%> (-19.25%) ⬇️
internal/proxy/task_search.go 46.71% <0.00%> (-17.35%) ⬇️
internal/util/logutil/logutil.go 11.53% <0.00%> (-16.50%) ⬇️
internal/querycoord/group_balance.go 62.96% <0.00%> (-12.65%) ⬇️
internal/core/src/segcore/ReduceStructure.h 87.50% <0.00%> (-12.50%) ⬇️
internal/util/retry/options.go 76.00% <0.00%> (-12.00%) ⬇️
internal/core/src/query/PlanProto.cpp 78.47% <0.00%> (-7.81%) ⬇️
internal/core/src/segcore/InsertRecord.cpp 86.20% <0.00%> (-7.74%) ⬇️
internal/querycoord/channel_unsubscribe.go 75.00% <0.00%> (-7.61%) ⬇️
... and 162 more

@LoveEachDay
Copy link
Contributor

/lgtm

@LoveEachDay
Copy link
Contributor

@schuberttobias We've update the data directory for etcd. Could you rebase your code?

@schuberttobias schuberttobias force-pushed the EtcdCompactionUbuntuPkg branch from 7ed3616 to f84a684 Compare May 18, 2022 12:23
@sre-ci-robot sre-ci-robot removed the lgtm label May 18, 2022
@sre-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@mergify mergify bot removed the ci-passed label May 18, 2022
@schuberttobias
Copy link
Author

@LoveEachDay Yes, I rebased onto 1cabaa1.

@mergify mergify bot added the ci-passed label May 18, 2022
@stale
Copy link

stale bot commented Jun 17, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label Jun 17, 2022
@stale stale bot closed this Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-passed dco-passed DCO check passed. size/XS Denotes a PR that changes 0-9 lines. stale indicates no udpates for 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants