chore: added size based retention policy #2098

ABresting · 2023-10-02T11:01:09Z

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 the database for defragmentation is triggered.

Changes

Size-related retention policy.
Vacuum process after the deletion of overflowed database entries in size-based retention policy.
Vacuum process in capacity-based retention policy.

Issue

#1885

github-actions · 2023-10-02T11:07:43Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2098

Built from 711f77d

Ivansete-status

Thanks for that PR! It is awesome!
I'm just adding a few comments that I hope you find useful.
Especially interesting to me is to only run the retention policy once, and make the limit validation before and after the retention policy execution.

Ivansete-status · 2023-10-02T11:18:53Z

tests/waku_archive/test_retention_policy.nim

+
+    ## Then
+    # calculate the current database size
+    let pageSize = (waitFor driver.getPagesSize()).tryGet()


This check is great!
However, shall we apply this check right before the retention policy is being applied?
I'd like to see the test checking that the current db size is beyond the limit, and once we execute the retention policy, then effectively check that the size got lower the limit.

The retention policy can only be executed once.

Good point Ivan,
Changes made to compare the current db size with the size limit, retention policy enabled only in case of overflow.
After the retention policy is executed, again comparison is made between the current db size and the size limit.

Ivansete-status · 2023-10-02T11:29:23Z

waku/waku_archive/retention_policy/builder.nim

+      try:
+        sizeQuantity = parseFloat(sizeQuantityStr)
+      except ValueError:
+        return err("invalid size retention policy argument")


Great error management!
It is advisable though to also append the current exception message.
That can be achieved with the next technique:

Suggested change

return err("invalid size retention policy argument")

return err("invalid size retention policy argument: " & getCurrentExceptionMsg())

That can be applied elsewhere.

Ivansete-status · 2023-10-02T11:34:39Z

waku/waku_archive/retention_policy/retention_policy_size.nim

+  var pageCountRes = await driver.getPagesCount()
+  if pageCountRes.isErr():
+    return err("failed to get Pages count: " & pageCountRes.error)
+
+  var pageCount: int64 = pageCountRes.value


This is great indeed!
However, we can make it a bit shorter with:

Suggested change

var pageCountRes = await driver.getPagesCount()

if pageCountRes.isErr():

return err("failed to get Pages count: " & pageCountRes.error)

var pageCount: int64 = pageCountRes.value

let pageCount = await driver.getPagesCount().isOkOr:

return err("failed to get Pages count: " & $error)

This technique is preferable whenever possible.

since the getPagesCount method is of future return type .sOkOr is not possible.

Sorry, I missed to add the parenthesis in my suggestion.
It compiles with the next:

let pageCount = (await driver.getPagesCount()).valueOr: return err("failed to get Pages count: " & $error)

Ivansete-status · 2023-10-02T11:37:48Z

waku/waku_archive/retention_policy/retention_policy_size.nim

+  var numMessagesRes = await driver.getMessagesCount()
+  if numMessagesRes.isErr():
+    return err("failed to get messages count: " & numMessagesRes.error)
+  var numMessages = numMessagesRes.value


Suggested change

var numMessagesRes = await driver.getMessagesCount()

if numMessagesRes.isErr():

return err("failed to get messages count: " & numMessagesRes.error)

var numMessages = numMessagesRes.value

let numMessages = await driver.getMessagesCount().isOkOr:

return err("failed to get messages count: " & $error)

Notice that I suggest to also use a let type because we don't need a mutable var at this point.

Ivansete-status · 2023-10-02T11:39:13Z

waku/waku_archive/retention_policy/retention_policy_size.nim

+  let resVaccum = await driver.performsVacuum()
+  if resVaccum.isErr():
+    return err("vacuumming failed: " & resVaccum.error)


Suggested change

let resVaccum = await driver.performsVacuum()

if resVaccum.isErr():

return err("vacuumming failed: " & resVaccum.error)

await driver.performsVacuum().isOkOr:

return err("vacuumming failed: " & $error)

same as before, future return type.

tests/waku_archive/test_retention_policy.nim

jm-clius

Thanks. Approving from approach POV (will let Ivan take a look at the detail of implementation). Some minor comments on code.

Importantly, though, this doesn't fix #1885, as it still needs to be integrated into the wakunode2 app configuration. This should be done in a separate PR before that issue can be closed (your comment in the issue description will auto close the issue once this is merged).

jm-clius · 2023-10-02T15:47:15Z

waku/waku_archive/driver.nim

+method getPagesSize*(driver: ArchiveDriver):
+                         Future[ArchiveDriverResult[int64]] {.base, async.} = discard
+
+method performsVacuum*(driver: ArchiveDriver):


Suggested change

method performsVacuum*(driver: ArchiveDriver):

method performVacuum*(driver: ArchiveDriver):

Nitpick. :) (function names should be verbs in imperative form, i.e. without the s)

good catch!

jm-clius · 2023-10-02T15:49:52Z

waku/waku_archive/retention_policy/retention_policy_size.nim

+
+  # get page size of database
+  let pageSizeRes = await driver.getPagesSize()
+  var pageSize: int64 = int64(pageSizeRes.valueOr(0) div 1024)


Suggested change

var pageSize: int64 = int64(pageSizeRes.valueOr(0) div 1024)

let pageSize: int64 = int64(pageSizeRes.valueOr(0) div 1024)

Unless you have a good reason to modify a variable later on, it should be final assigned with let. A good rule of thumb is to always use let, unless you are forced to create a variable that needs to be modifiable.

jm-clius · 2023-10-02T15:53:59Z

waku/waku_archive/retention_policy/retention_policy_size.nim

+    return err("failed to get Page size: " & pageSizeRes.error)
+
+  # database size in megabytes (Mb)
+  var totalSizeOfDB: float = float(pageSize * pageCount)/1024.0


Suggested change

var totalSizeOfDB: float = float(pageSize * pageCount)/1024.0

let totalSizeOfDB: float = float(pageSize * pageCount)/1024.0

(and elsewhere)

Ivansete-status · 2023-10-09T09:17:04Z

tests/waku_archive/test_retention_policy.nim

+    if sizeDB >= sizeLimit:
+      require (waitFor retentionPolicy.execute(driver)).isOk()


This is great indeed! However, would it be possible to enforce a require instead?

Suggested change

if sizeDB >= sizeLimit:

require (waitFor retentionPolicy.execute(driver)).isOk()

require sizeDB >= sizeLimit

require (waitFor retentionPolicy.execute(driver)).isOk()

…-add-size-retention-policy

…ty retention

Ivansete-status

LGTM! Thanks for it and for the patience! 💯

I just added a comment to add a further protection but for a separate PR.

@apentori - this feature is very interesting to be applied in the current wakuv2.test and wakuv2.prod fleets. wakuv2.test will receive that once this PR is merged, and wakuv2.prod will have this feature in the next release (I think during next week.)

Ivansete-status · 2023-10-09T13:08:13Z

waku/waku_archive/retention_policy/retention_policy_size.nim

+    return ok()
+
+  # keep deleting until the current db size falls within size limit 
+  while totalSizeOfDB > p.sizeLimit:


This logic looks great to me! However, given the possible issue of stepping into an infinite loop, I think we should have a PR in the near future where we prevent that possible blocking issue. For example, if the measured totalSizeOfDB doesn't change in two consecutive iterations, then we should break the loop by returning the appropriate error.

yes, I do not like loops in the code, what I can imagine we can do is, since we have a max size/threshold of the database, we can have a lower threshold of the database as well, then inside the loop, we introduce a notation, i.e. max number of times the loop should run so that the size of the database will reduce to the lowest,

$$ n > \frac{\log(\text{lowerThresholdInMbs}/\text{maxDbSizeOrCurrentDbSize})}{\log(\text{reducedToUponEachIteration})} $$

Here:
lowerThresholdInMbs is the lowest database size possible
maxDbSizeOrCurrentDbSize is the maximum database size possible
reducedToUponEachIteration is the fraction of size/pages the database reduced to upon each iteration

For eg., in the case of 30720 MBs(30 GB) as max size, 0.1 MBs as a lower threshold, and 0.80 (80% of the db pages retained) as a fraction of the database retained upon each iteration. We get 57 iterations. SO if we use this then there is a guarantee that the loop will not cross 57 times at max.

This way there is a good chance that the forever loop is not encountered.
Post this we can affirm a warning/error if the database size is still not less than the maximum permitted size.

WDYT? @Ivansete-status @alrevuelta

I vote to follow the simplest approach. However it is fine for this PR for now and we can revisit that in a separate PR in the near future.

alrevuelta

minor comment

alrevuelta · 2023-10-09T15:37:30Z

waku/waku_archive/retention_policy/retention_policy_size.nim

+  if totalSizeOfDB < p.sizeLimit:
+    return ok()
+
+  # keep deleting until the current db size falls within size limit 


I see in this function a pattern that is usually solved with a do while. A "do while" can remove aroud 25 lines of code, since totalSizeOfDB, pageSize and pageCount are a bit boilerplate.

Since nim doesnt have "do while" I would suggest:

while true: ...(get page get count) var totalSizeOfDB: float = float(pageSize * pageCount)/1024.0 if totalSizeOfDB > p.sizeLimit: break let res = await driver.deleteOldestMessagesNotWithinLimit(limit=pageDeleteWindow) if res.isErr(): return err("deleting oldest messages failed: " & res.error)

So with something like this you just need 1 single call to totalSizeOfDB, pageSize and pageCount

…-add-size-retention-policy

alrevuelta

lgtm

github-actions · 2023-10-10T09:04:51Z

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 release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

chore: added size based retention policy

0b7e863

ABresting requested review from Ivansete-status, jm-clius, alrevuelta and NagyZoltanPeter October 2, 2023 11:01

ABresting self-assigned this Oct 2, 2023

Ivansete-status requested changes Oct 2, 2023

View reviewed changes

jm-clius approved these changes Oct 2, 2023

View reviewed changes

chore: post review code - size based retention policy

9fedc9f

Ivansete-status reviewed Oct 9, 2023

View reviewed changes

ABresting mentioned this pull request Oct 9, 2023

feat: return different exits code on DB vacuum failure waku-org/go-waku#797

Closed

ABresting added 2 commits October 9, 2023 13:49

Merge branch 'master' of https://github.com/waku-org/nwaku into chore…

675687c

…-add-size-retention-policy

chore: review integrated, size retention updated, vacuuming in capaci…

d7140dd

…ty retention

ABresting requested a review from Ivansete-status October 9, 2023 12:03

chore: typo fixed

c026cc9

Ivansete-status approved these changes Oct 9, 2023

View reviewed changes

alrevuelta requested changes Oct 9, 2023

View reviewed changes

ABresting added 2 commits October 9, 2023 18:08

chore: review integrated

5022d3e

Merge branch 'master' of https://github.com/waku-org/nwaku into chore…

70781ad

…-add-size-retention-policy

ABresting requested a review from alrevuelta October 9, 2023 16:13

alrevuelta approved these changes Oct 10, 2023

View reviewed changes

chore: external config updated to support newly added retention policy

3a71923

ABresting force-pushed the chore-add-size-retention-policy branch from 7a4ebbd to 3a71923 Compare October 10, 2023 09:04

ABresting merged commit 25d6e52 into master Oct 10, 2023
9 of 10 checks passed

ABresting deleted the chore-add-size-retention-policy branch October 10, 2023 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: added size based retention policy #2098

chore: added size based retention policy #2098

ABresting commented Oct 2, 2023 •

edited

Loading

github-actions bot commented Oct 2, 2023 •

edited

Loading

Ivansete-status left a comment

Ivansete-status Oct 2, 2023

ABresting Oct 2, 2023

Ivansete-status Oct 2, 2023

Ivansete-status Oct 2, 2023

ABresting Oct 2, 2023

Ivansete-status Oct 4, 2023

Ivansete-status Oct 2, 2023

Ivansete-status Oct 2, 2023

ABresting Oct 2, 2023

jm-clius left a comment

jm-clius Oct 2, 2023

ABresting Oct 2, 2023

jm-clius Oct 2, 2023

jm-clius Oct 2, 2023

jm-clius Oct 2, 2023

Ivansete-status Oct 9, 2023

Ivansete-status left a comment

Ivansete-status Oct 9, 2023

ABresting Oct 9, 2023 •

edited

Loading

Ivansete-status Oct 9, 2023

alrevuelta left a comment

alrevuelta Oct 9, 2023

alrevuelta left a comment

github-actions bot commented Oct 10, 2023

	return err("invalid size retention policy argument")
	return err("invalid size retention policy argument: " & getCurrentExceptionMsg())

	method performsVacuum*(driver: ArchiveDriver):
	method performVacuum*(driver: ArchiveDriver):

	var pageSize: int64 = int64(pageSizeRes.valueOr(0) div 1024)
	let pageSize: int64 = int64(pageSizeRes.valueOr(0) div 1024)

	var totalSizeOfDB: float = float(pageSize * pageCount)/1024.0
	let totalSizeOfDB: float = float(pageSize * pageCount)/1024.0

		if sizeDB >= sizeLimit:
		require (waitFor retentionPolicy.execute(driver)).isOk()

chore: added size based retention policy #2098

chore: added size based retention policy #2098

Conversation

ABresting commented Oct 2, 2023 • edited Loading

Description

Changes

Issue

github-actions bot commented Oct 2, 2023 • edited Loading

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ABresting Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrevuelta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrevuelta left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 10, 2023

ABresting commented Oct 2, 2023 •

edited

Loading

github-actions bot commented Oct 2, 2023 •

edited

Loading

ABresting Oct 9, 2023 •

edited

Loading