feat(postgresql): 1st commit to async sql (waku_archive/driver...) #1755

Ivansete-status · 2023-05-23T18:42:58Z

Description

This PR is a very first step on getting asynchronous calls to any CRUD archive operation.

Important to notice that this PR doesn't allow async calls per se. For example, in waku/v2/waku_archive/driver/sqlite_driver/sqlite_driver.nim, the method getMessages() will still be sync because the call to s.db.selectMessagesByHistoryQueryWithLimit(..) will still be blocking.

Instead, this PR is a reorganization to adopt the asynchronous patterns for waku archive and adapt the code for the upcoming PR's where we will aggregate the @LNSD's PR, and therefore, start incorporating the needed to allow async interaction with a PostgreSQL database.

Sorry for the big number of changes. These are the most relevant:

waku/v2/waku_archive/driver.nim
waku/v2/waku_archive/driver/sqlite_driver/sqlite_driver.nim
waku/v2/waku_archive/driver/queue_driver/queue_driver.nim
( in the next PR's we will have a postgresql driver )

Changes

Make async all the waku_archive/driver.nim methods

Issue

#1604

jm-clius

Thanks for this! I'm about ready to approve this PR (good work), but have added a couple of comments below.

jm-clius · 2023-05-24T15:08:22Z

waku/v2/waku_archive/driver/queue_driver/queue_driver.nim

+  else:
+    return ok(pageRes.value)


A bit nitpicky here, but I prefer having the "default" return case here be the last statement as before and not just a part of the conditional here. That way the last statement is your expected happy-path return and other returns are either fail conditions (or quick successes).

jm-clius · 2023-05-24T15:13:58Z

waku/v2/waku_archive/driver/sqlite_driver/sqlite_driver.nim

  ## Inserts a message into the store
-
-  let res = s.insertStmt.exec((
+  return s.insertStmt.exec((


I'd still assign this to a res variable here to keep the final return a bit cleaner.

jm-clius · 2023-05-24T15:15:21Z

waku/v2/waku_archive/driver/sqlite_driver/sqlite_driver.nim

  let cursor = cursor.map(toDbCursor)

-  let rows = ?s.db.selectMessagesByHistoryQueryWithLimit(
+  return s.db.selectMessagesByHistoryQueryWithLimit(


As above, I'd keep the indirection here with assigning this to a variable first to have the SQLite-specific failure and return cleanly separated within the SqliteDriver module.

jm-clius · 2023-05-24T15:16:39Z

waku/v2/waku_archive/retention_policy.nim

+method execute*(p: RetentionPolicy, store: ArchiveDriver):
+                Future[Result[void, string]] {.base.} = discard


Suggested change

Future[Result[void, string]] {.base.} = discard

Future[RetentionPolicyResult[void]] {.base.} = discard

Or am I missing something?

jm-clius · 2023-05-24T15:18:48Z

waku/v2/waku_archive/retention_policy.nim

+method execute*(p: RetentionPolicy, store: ArchiveDriver):
+                Future[Result[void, string]] {.base.} = discard


Any reason .base. methods aren't annotated with async pragma too?

jm-clius · 2023-05-24T15:22:00Z

waku/v2/waku_archive/retention_policy/retention_policy_time.nim

+  else:
+    return ok()


Similar to elsewhere, I'd keep this as a statement on its own to signify that it's the expected exit for the method.

jm-clius · 2023-05-24T15:34:47Z

waku/v2/node/waku_node.nim

+  asyncSpawn node.wakuArchive.executeMessageRetentionPolicy()
+  asyncSpawn node.wakuArchive.reportStoredMessagesMetric()


Not sure of impact, but since this is a timed task, do we want to spawn more futures here or shouldn't this function block until we have finished executing the retention policy as it did before (i.e. use waitFor rather than asyncSpawn)?

agree. actually as it is the updated metrics by reportStoredMessagesMetric wont be accurate since reportStoredMessagesMetric will complete before executeMessageRetentionPolicy i guess?

jm-clius · 2023-05-24T15:36:42Z

tests/v2/waku_store/test_wakunode_store.nim

+    let archiveA = block:
+      let driver = newTestArchiveDriver()
+
+      for msg in msgListA:
+        let msg_digest = waku_archive.computeDigest(msg)
+        require (await driver.put(DefaultPubsubTopic, msg, msg_digest, msg.timestamp)).isOk()
+
+      driver


Why did this move into each test? If it's about the await, wouldn't waitFor and blocking solve it?

Yes your are right.
I moved it due to the await.
I'm moving it back and will use waitFor instead.

jm-clius · 2023-05-24T15:38:24Z

tests/v2/waku_archive/test_waku_archive.nim

+    let archiveA = block:
+      let
+        driver = newTestArchiveDriver()
+        archive = newTestWakuArchive(driver)
+
+      for msg in msgListA:
+        require (await driver.put(DefaultPubsubTopic, msg, computeDigest(msg), msg.timestamp)).isOk()
+
+      archive


Perhaps if you use waitFor rather than await this wouldn't need to move into the async block?

jm-clius · 2023-05-24T15:39:46Z

waku/v2/waku_archive/driver.nim

+            message: WakuMessage,
+            digest: MessageDigest,
+            receivedTime: Timestamp):
+            Future[ArchiveDriverResult[void]] {.base.} = discard


Any reason not to annotate base methods as async?

alrevuelta

nothing to add to the existing commnets, thanks for the PR!

alrevuelta · 2023-05-25T07:20:43Z

waku/v2/node/waku_node.nim

+  asyncSpawn node.wakuArchive.executeMessageRetentionPolicy()
+  asyncSpawn node.wakuArchive.reportStoredMessagesMetric()


agree. actually as it is the updated metrics by reportStoredMessagesMetric wont be accurate since reportStoredMessagesMetric will complete before executeMessageRetentionPolicy i guess?

Ivansete-status · 2023-05-25T15:06:29Z

Thanks so much for the comments! It can be re-reviewed :)

jm-clius

Thanks! LGTM.

One thing, but it's up to you if you want to revert: apologies if I was unclear in previous feedback, but in general there is no issue with using asyncTest and await rather than waitFor. In fact, this is what we'll prefer. The suggestion for using waitFor was simply for creating the test archive drivers outside the (async) test blocks, whereas the code in the test blocks can continue using await. Still this does not have to be part of this PR and only affects the tests. :)

feat(postgresql): 1st commit to async sql (waku_archive/driver...)

c822e37

Ivansete-status force-pushed the feat/1604-async-waku-archive branch from 5e5baa9 to c822e37 Compare May 24, 2023 10:09

Ivansete-status marked this pull request as ready for review May 24, 2023 11:10

Ivansete-status requested review from vpavlin, alrevuelta, rymnc and jm-clius May 24, 2023 11:10

Ivansete-status self-assigned this May 24, 2023

jm-clius reviewed May 24, 2023

View reviewed changes

alrevuelta approved these changes May 25, 2023

View reviewed changes

Enhancements based on PR review suggestions

70388fb

Ivansete-status requested a review from jm-clius May 25, 2023 15:06

jm-clius approved these changes May 25, 2023

View reviewed changes

Ivansete-status merged commit 59ca03a into master May 25, 2023

Ivansete-status deleted the feat/1604-async-waku-archive branch May 25, 2023 15:34

Ivansete-status restored the feat/1604-async-waku-archive branch May 26, 2023 16:58

Ivansete-status deleted the feat/1604-async-waku-archive branch May 26, 2023 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(postgresql): 1st commit to async sql (waku_archive/driver...) #1755

feat(postgresql): 1st commit to async sql (waku_archive/driver...) #1755

Ivansete-status commented May 23, 2023 •

edited

Loading

jm-clius left a comment

jm-clius May 24, 2023

jm-clius May 24, 2023

jm-clius May 24, 2023

jm-clius May 24, 2023

jm-clius May 24, 2023

jm-clius May 24, 2023

jm-clius May 24, 2023

alrevuelta May 25, 2023

jm-clius May 24, 2023

Ivansete-status May 25, 2023

jm-clius May 24, 2023

jm-clius May 24, 2023

alrevuelta left a comment

alrevuelta May 25, 2023

Ivansete-status commented May 25, 2023

jm-clius left a comment

		method execute*(p: RetentionPolicy, store: ArchiveDriver):
		Future[Result[void, string]] {.base.} = discard

	Future[Result[void, string]] {.base.} = discard
	Future[RetentionPolicyResult[void]] {.base.} = discard

		asyncSpawn node.wakuArchive.executeMessageRetentionPolicy()
		asyncSpawn node.wakuArchive.reportStoredMessagesMetric()

feat(postgresql): 1st commit to async sql (waku_archive/driver...) #1755

feat(postgresql): 1st commit to async sql (waku_archive/driver...) #1755

Conversation

Ivansete-status commented May 23, 2023 • edited Loading

Description

Changes

Issue

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

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

alrevuelta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status commented May 25, 2023

jm-clius left a comment

Choose a reason for hiding this comment

Ivansete-status commented May 23, 2023 •

edited

Loading