-
Notifications
You must be signed in to change notification settings - Fork 3k
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
enhance: Add delete buffer related quota logic #35918
enhance: Add delete buffer related quota logic #35918
Conversation
See also milvus-io#35303 Signed-off-by: Congqi Xia <[email protected]>
[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 |
Signed-off-by: Congqi Xia <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch looks good to me, except some minor issues.
Question: will this patch deprecate the same protection feature based on L0 row number?
configs/milvus.yaml
Outdated
deleteBufferRowCountProtection: | ||
enabled: false # switch to enable delete buffer row count quota | ||
lowWaterLevel: 32768 # delete buffer row count quota, low water level | ||
highWaterLevel: 65536 # delete buffer row count quota, high water level | ||
deleteBufferSizeProtection: | ||
enabled: false # switch to enable delete buffer size quota | ||
lowWaterLevel: 32768 # delete buffer size quota, low water level | ||
highWaterLevel: 65536 # delete buffer size quota, high water level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default values may be too small, I suppose it won't be a problem if the deletions can be easily consumed in less than 1 minute or so.
And, please note the unit of the delete buffer size in the comments.
Signed-off-by: Congqi Xia <[email protected]>
Signed-off-by: Congqi Xia <[email protected]>
nope, this PR only add quota rules based on delete buffer number/size on querynodes, which will not affect the previously added l0 segment statistics one |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35918 +/- ##
==========================================
- Coverage 81.58% 72.64% -8.94%
==========================================
Files 1264 1264
Lines 150745 150876 +131
==========================================
- Hits 122983 109604 -13379
- Misses 22878 36385 +13507
- Partials 4884 4887 +3
|
deleteBufferRowCountProtection: | ||
enabled: false # switch to enable delete buffer row count quota | ||
lowWaterLevel: 32768 # delete buffer row count quota, low water level | ||
highWaterLevel: 65536 # delete buffer row count quota, high water level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be merged ahead, but the waterlevel value is not sensible, need to adjust it before put in prod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, this value shall be updated after we get some experience data from the tests
/lgtm |
See also milvus-io#35303 --------- Signed-off-by: Congqi Xia <[email protected]>
See also milvus-io#35303 --------- Signed-off-by: Congqi Xia <[email protected]>
Cherry pick from master pr: #35128 #35918 See also #35303 --------- Signed-off-by: aoiasd <[email protected]> Signed-off-by: Congqi Xia <[email protected]> Co-authored-by: aoiasd <[email protected]>
See also milvus-io#35303 --------- Signed-off-by: Congqi Xia <[email protected]>
See also milvus-io#35303 --------- Signed-off-by: Congqi Xia <[email protected]>
See also #35303