Skip to content
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.

Sign up for GitHub

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

Add postgres basic support #1590

Closed
wants to merge 2 commits into from
Closed

Add postgres basic support #1590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a83876e
Add postgres basic support
cammellos Mar 1, 2023
2d287ec
[WIP] Address feedback
cammellos Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ test: | test1 test2

v1: | wakunode1 example1 sim1
v2: | wakunode2 example2 wakubridge chat2 chat2bridge
v2light: | wakunode2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used for testing, will remove

waku.nims:
ln -s waku.nimble $@
Expand Down Expand Up @@ -144,12 +145,17 @@ testcommon: | build deps
#############
## Waku v2 ##
#############
.PHONY: test2 wakunode2 example2 sim2 scripts2 wakubridge chat2 chat2bridge
.PHONY: test2 wakunode2 example2 sim2 scripts2 wakubridge chat2 chat2bridge mytests

test2: | build deps librln testcommon
echo -e $(BUILD_MSG) "build/$@" && \
$(ENV_SCRIPT) nim test2 $(NIM_PARAMS) $(EXPERIMENTAL_PARAMS) waku.nims


mytests: | build deps librln
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used for testing, will remove

echo -e $(BUILD_MSG) "build/$@" && \
$(ENV_SCRIPT) nim mytests $(NIM_PARAMS) $(EXPERIMENTAL_PARAMS) waku.nims

wakunode2: | build deps librln
echo -e $(BUILD_MSG) "build/$@" && \
$(ENV_SCRIPT) nim wakunode2 $(NIM_PARAMS) $(EXPERIMENTAL_PARAMS) waku.nims
Expand Down
38 changes: 3 additions & 35 deletions apps/wakunode2/wakunode2.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,14 @@ import
../../waku/common/sqlite,
../../waku/common/utils/nat,
../../waku/common/logging,
../../waku/v2/config,
../../waku/v2/node/peer_manager,
../../waku/v2/node/peer_manager/peer_store/waku_peer_storage,
../../waku/v2/node/peer_manager/peer_store/migrations as peer_store_sqlite_migrations,
../../waku/v2/node/wakuswitch,
../../waku/v2/node/waku_node,
../../waku/v2/node/waku_metrics,
../../waku/v2/protocol/waku_archive,
../../waku/v2/protocol/waku_archive/driver/queue_driver,
../../waku/v2/protocol/waku_archive/driver/sqlite_driver,
../../waku/v2/protocol/waku_archive/driver/sqlite_driver/migrations as archive_driver_sqlite_migrations,
../../waku/v2/protocol/waku_archive/retention_policy,
../../waku/v2/protocol/waku_archive/retention_policy/retention_policy_capacity,
../../waku/v2/protocol/waku_archive/retention_policy/retention_policy_time,
Expand All @@ -50,7 +48,7 @@ import
../../waku/v2/utils/peers,
./wakunode2_setup_rest,
./wakunode2_setup_rpc,
./config
./wakunode2_archive_driver

when defined(rln):
import
Expand Down Expand Up @@ -158,36 +156,6 @@ proc setupWakuArchiveRetentionPolicy(retentionPolicy: string): SetupResult[Optio
else:
return err("unknown retention policy")

proc setupWakuArchiveDriver(dbUrl: string, vacuum: bool, migrate: bool): SetupResult[ArchiveDriver] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the reasons behind these changes (e.g., sharing the initialization code), we cannot accept them as part of this PR. I see these changes as out of scope.

We aim to simplify the WakuNode type as much as possible. Moving this logic inside the waku_node module increases the code coupling affecting the code testability and maintainability.

If it is strictly necessary, I suggest you duplicate the code you need. But, please, do not remove (or move) already existing code :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand what you mean, you prefer a dependency injection pattern, I can move things back as they were.

The reason I have moved the code, is because this namespace/file will know about postgres, which to me indicates that logic is not particularly well encapsulated, as this namespace is doing a lot of things already, and some of them are low-level concerns, like setting up sqlite/in-memory (and eventually postgres), driver, so I would suggest moving those decisions somewhere else.

Nothing in the apps/wakunode2 seems to be tested by the way (though I maybe wrong, apologies if that's the case, but as far as I can see it's not imported in tests/, and breaking any of the function does not seem make any of the tests fail), so I'd also recommend having at least a few tests making sure it works, as there's definitely some non trivial logic.

As a side note "please, do not remove (or move) already existing code", it's not something I can agree with, and that's not an acceptable statement for working on a codebase, happy to agree with you on what changes needs making and the overall strategy of course (ergo the draft PR), it's always a discussion, but it's just not how software development is done.

Copy link
Contributor

@LNSD LNSD Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note "please, do not remove (or move) already existing code", it's not something I can agree with, and that's not an acceptable statement for working on a codebase, happy to agree with you on what changes needs making and the overall strategy of course (ergo the draft PR), it's always a discussion, but it's just not how software development is done.

Sorry if it sounded bad. Very far from my intention 😅

I meant that these big and "unrelated changes" are touching common parts of the app and the codebase that are very git-conflict prone. This part has not been refactored and cleaned up yet. And this will affect our near-future work and productivity in the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's cool, I thought that might be the case :)
I'll update the PR according to this discussion, thanks for the feedback

let db = ?setupDatabaseConnection(dbUrl)

if db.isSome():
# SQLite vacuum
# TODO: Run this only if the database engine is SQLite
let (pageSize, pageCount, freelistCount) = ?gatherSqlitePageStats(db.get())
debug "sqlite database page stats", pageSize=pageSize, pages=pageCount, freePages=freelistCount

if vacuum and (pageCount > 0 and freelistCount > 0):
?performSqliteVacuum(db.get())

# Database migration
if migrate:
?archive_driver_sqlite_migrations.migrate(db.get())

if db.isSome():
debug "setting up sqlite waku archive driver"
let res = SqliteDriver.new(db.get())
if res.isErr():
return err("failed to init sqlite archive driver: " & res.error)

ok(res.value)

else:
debug "setting up in-memory waku archive driver"
let driver = QueueDriver.new() # Defaults to a capacity of 25.000 messages
ok(driver)


proc retrieveDynamicBootstrapNodes*(dnsDiscovery: bool, dnsDiscoveryUrl: string, dnsDiscoveryNameServers: seq[ValidIpAddress]): SetupResult[seq[RemotePeerInfo]] =

if dnsDiscovery and dnsDiscoveryUrl != "":
Expand Down Expand Up @@ -642,7 +610,7 @@ when isMainModule:
error "failed to configure the message store database connection", error=dbUrlValidationRes.error
quit(QuitFailure)

let archiveDriverRes = setupWakuArchiveDriver(dbUrlValidationRes.get(), vacuum=conf.storeMessageDbVacuum, migrate=conf.storeMessageDbMigration)
let archiveDriverRes = setupWakuArchiveDriver(conf)
if archiveDriverRes.isOk():
archiveDriver = some(archiveDriverRes.get())
else:
Expand Down
94 changes: 94 additions & 0 deletions apps/wakunode2/wakunode2_archive_driver.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import
std/strutils,
chronicles,
stew/results,
../../waku/v2/protocol/waku_archive,
../../waku/v2/config,
../../waku/v2/protocol/waku_archive/driver/sqlite_driver,
../../waku/v2/protocol/waku_archive/driver/postgres_driver,
../../waku/v2/protocol/waku_archive/driver/queue_driver,
../../waku/v2/protocol/waku_archive/driver/sqlite_driver/migrations as archive_driver_sqlite_migrations,
../../waku/common/sqlite

type SetupResult[T] = Result[T, string]

proc performSqliteVacuum(db: SqliteDatabase): SetupResult[void] =
## SQLite database vacuuming
# TODO: Run vacuuming conditionally based on database page stats
# if (pageCount > 0 and freelistCount > 0):

debug "starting sqlite database vacuuming"

let resVacuum = db.vacuum()
if resVacuum.isErr():
return err("failed to execute vacuum: " & resVacuum.error)

debug "finished sqlite database vacuuming"



proc gatherSqlitePageStats(db: SqliteDatabase): SetupResult[(int64, int64, int64)] =
let
pageSize = ?db.getPageSize()
pageCount = ?db.getPageCount()
freelistCount = ?db.getFreelistCount()

ok((pageSize, pageCount, freelistCount))


proc setupSqliteDriver(conf: WakuNodeConf, path: string): SetupResult[ArchiveDriver] =
let res = SqliteDatabase.new(path)

if res.isErr():
return err("could not create sqlite database")

let database = res.get()

# TODO: Run this only if the database engine is SQLite
let (pageSize, pageCount, freelistCount) = ?gatherSqlitePageStats(database)
debug "sqlite database page stats", pageSize=pageSize, pages=pageCount, freePages=freelistCount

if conf.storeMessageDbVacuum and (pageCount > 0 and freelistCount > 0):
?performSqliteVacuum(database)

# Database migration
if conf.storeMessageDbMigration:
?archive_driver_sqlite_migrations.migrate(database)

debug "setting up sqlite waku archive driver"
let sqliteDriverRes = SqliteDriver.new(database)
if sqliteDriverRes.isErr():
return err("failed to init sqlite archive driver: " & res.error)

ok(sqliteDriverRes.value)

proc setupPostgresDriver(conf: WakuNodeConf): SetupResult[ArchiveDriver] =
let res = PostgresDriver.new(conf)
if res.isErr():
return err("could not create postgres driver")

ok(res.value)

proc setupWakuArchiveDriver*(conf: WakuNodeConf): SetupResult[ArchiveDriver] =
if conf.storeMessageDbUrl == "" or conf.storeMessageDbUrl == "none":
let driver = QueueDriver.new() # Defaults to a capacity of 25.000 messages
return ok(driver)

let dbUrlParts = conf.storeMessageDbUrl.split("://", 1)
let
engine = dbUrlParts[0]
path = dbUrlParts[1]

let connRes = case engine
of "sqlite":
setupSqliteDriver(conf, path)
of "postgres":
setupPostgresDriver(conf)
else:
return err("unknown database engine")

if connRes.isErr():
return err("failed to init connection" & connRes.error)
ok(connRes.get())


2 changes: 1 addition & 1 deletion apps/wakunode2/wakunode2_setup_rest.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import
../../waku/v2/node/rest/debug/handlers as debug_api,
../../waku/v2/node/rest/relay/handlers as relay_api,
../../waku/v2/node/rest/relay/topic_cache,
./config
../../waku/v2/config
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved config to its own namespace so it can be reused



logScope:
Expand Down
2 changes: 1 addition & 1 deletion apps/wakunode2/wakunode2_setup_rpc.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import
../../waku/v2/node/jsonrpc/filter/handlers as filter_api,
../../waku/v2/node/jsonrpc/relay/handlers as relay_api,
../../waku/v2/node/jsonrpc/store/handlers as store_api,
./config
../../waku/v2/config

logScope:
topics = "wakunode jsonrpc"
Expand Down
11 changes: 11 additions & 0 deletions tests/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
version: "3.8"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dockerfile to run tests against it, not sure how/when we want to run this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are asking about CI, we can do it like we do it with status-go:
https://github.com/status-im/status-go/blob/070ffd9f5c455e2e8de4ef5ac17b39bb6064a9ef/_assets/ci/Jenkinsfile.tests#L64-L76
But we don't need a Docker Compose for that.



services:
db:
image: postgres:9.6-alpine
restart: always
environment:
POSTGRES_PASSWORD: test123
ports:
- "5432:5432"
5 changes: 5 additions & 0 deletions tests/my_tests.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
## Waku v2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used for tests, will remove


# Waku archive test suite
import
./v2/waku_archive/test_driver_postgres
Loading