-
Notifications
You must be signed in to change notification settings - Fork 26
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
Do not require specifying quota when specifying location watermarks #405
base: main
Are you sure you want to change the base?
Conversation
23a1761
to
a9cfeb2
Compare
a9cfeb2
to
11568d1
Compare
11568d1
to
ff74061
Compare
@l0kix2 could you please check this PR out? It still says that 1 approval is required. |
sure, sorry for the delay, will look into it |
@@ -337,8 +338,8 @@ func getDataNodeServerCarcass(spec *ytv1.DataNodesSpec) (DataNodeServer, error) | |||
storeLocation.HighWatermark = storeLocation.LowWatermark / 2 | |||
storeLocation.DisableWritesWatermark = storeLocation.HighWatermark / 2 | |||
storeLocation.TrashCleanupWatermark = storeLocation.LowWatermark | |||
storeLocation.MaxTrashTtl = location.MaxTrashMilliseconds | |||
} |
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.
It looks a bit hard to understand and error-prone with all these nested ifs.
I suggest we rewrite it in less nested way:
// setting quota
if quota != nil {
storeLocation.Quota = *quota
}
// setting low watermark
if location.LowWatermark != nil {
storeLocation.LowWatermark = location.LowWatermark.Value()
} else if quota != nil {
gb := float64(1024 * 1024 * 1024)
storeLocation.LowWatermark = int64(math.Min(0.1*float64(storeLocation.Quota), float64(25)*gb))
}
// setting other watermarks if low watermark is set
if storeLocation.LowWatermark > 0 {
storeLocation.HighWatermark = storeLocation.LowWatermark / 2
storeLocation.DisableWritesWatermark = storeLocation.HighWatermark / 2
storeLocation.TrashCleanupWatermark = storeLocation.LowWatermark
}
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.
Also I suggest to add test cases for scenarios which aren't currently covered, so this code won't become broken in the next changes.
Tests for config generator are rather easy to write: just add some test cases with specs having quota=nil + low_watermark=nil, quota!=nil + low_watermark=nil, quota=nil + low_watermark!=nil
and simply check that canonized result looks as expected in config.
No description provided.