-
Notifications
You must be signed in to change notification settings - Fork 57
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
chore: add size retention policy #2093
Conversation
You can find the image built from this PR at
Built from 8a8e107 |
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.
LGTM
Thank you!
See my comments for minor changes.
@@ -51,5 +52,38 @@ proc new*(T: type RetentionPolicy, | |||
let retPolicy: RetentionPolicy = CapacityRetentionPolicy.init(retentionCapacity) | |||
return ok(some(retPolicy)) | |||
|
|||
elif policy == "size": |
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.
Unrelated but why is this not a enum instead of a string?
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.
overkill for limited parameters?
var numMessagesRes = await driver.getMessagesCount() | ||
if numMessagesRes.isErr(): | ||
return err("failed to get messages count: " & numMessagesRes.error) | ||
var numMessages = numMessagesRes.value |
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.
Same here.
var numMessagesRes = await driver.getMessagesCount() | |
if numMessagesRes.isErr(): | |
return err("failed to get messages count: " & numMessagesRes.error) | |
var numMessages = numMessagesRes.value | |
var numMessages = await driver.getMessagesCount().valueOr: | |
return err("failed to get messages count: " & error) |
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.
idk what if there is a 0 message condition, and someone triggers the policy check on an empty database?
Signed-off-by: Jakub Sokołowski <jakub@status.im>
Signed-off-by: Jakub Sokołowski <jakub@status.im>
Signed-off-by: Jakub Sokołowski <jakub@status.im>
Signed-off-by: Jakub Sokołowski <jakub@status.im>
* Add test aggregator to all directories. * Implement coverage script.
Also use absolute path to load Groovy script. Signed-off-by: Jakub Sokołowski <jakub@status.im>
…tially concurrent, add version (#2080) * chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version * add more metrics, refactor how most metrics are calculated * rework metrics table fillup * reset connErr to make sure we honour successful reconnection
…2079) * Adding cpp example that integrates the `libwaku` --------- Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>
b539a43
to
806c7b9
Compare
This PR may contain changes to configuration options of one of the apps. If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed. Please also make sure the label |
This reverts commit 8897ae1. (cherry picked from commit b213b2c385b0534481448cd6e30af18e183d0504)
Description
Size retention Policy has been added. Based on the size limit provided in mb or gb. Once the size limit overflows by messages, then the 20% of the outdated messages are dropped and a vacuum of database for defragmentation is triggered.
Changes