optakt / flow-dps

Flow Data Provisioning Service
Apache License 2.0
29 stars 13 forks source link

Add configuration option to run with or without readonly mode #503

Closed m4ksio closed 2 years ago

m4ksio commented 2 years ago

Add configuration option to run with or without readonly mode

Goal of this PR

Fixes #504

Let configure if database is to be open as readonly mode. This come in handy when shutdown wasn't clean and BadgerDB needs to replay logs

Checklist

Does the PR require tests to be added or updated?

No tests required

Does the PR require documentation to be added or updated?

No, default behaviour stays the same

Misc

Ullaakut commented 2 years ago

@m4ksio

Also, why is this PR only against the snapshot creation tool and not the other instances of us opening badger DBs? Is it only an issue in the case of the snapshot creation tool?

I'll wait for your answer before merging anything, since we most likely want to have this wherever we open badger DBs with read-only, which is in pretty much every binary.

Ullaakut commented 2 years ago

OK it seems I do not have write access on your branch unfortunately. Here is the required change:

index 68f995b..8789f6a 100644
--- a/cmd/create-index-snapshot/README.md
+++ b/cmd/create-index-snapshot/README.md
@@ -16,6 +16,7 @@ Usage of create-index-snapshot:
   -c, --compression string   compression algorithm ("none", "zstd" or "gzip") (default "zstd")
   -e, --encoding string      output encoding ("none", "hex" or "base64") (default "none")
   -i, --index string         database directory for state index (default "index")
+  -r, --readonly bool        open database as read-only (default true)
m4ksio commented 2 years ago

Also, why is this PR only against the snapshot creation tool and not the other instances of us opening badger DBs? Is it only an issue in the case of the snapshot creation tool?

Its the only one I experienced problem with. Most tools open BadgerDB for writing, so this point is moot, for all but flow-dps-server. I guess its worth porting this flag there. Let me update the PR

Ullaakut commented 2 years ago

Also if we're doing this, I'd suggest not using shorthand flags, since this should really only be used in edge cases, I'd rather not have a -r for it, but make it be --read-only only instead. SGTY?

Ullaakut commented 2 years ago

@m4ksio 👋 Did you have time to check my previous comments on your PR? I can't make the modifications myself unfortunately

m4ksio commented 2 years ago

Also if we're doing this, I'd suggest not using shorthand flags, since this should really only be used in edge cases, I'd rather not have a -r for it, but make it be --read-only only instead. SGTY?

Yeah, thats a good idea as its a special flag. Changed that, also added readonly flag for flow-dps-server

m4ksio commented 2 years ago

Regarding the rest of the PR, it looks like you also added the Rosetta dispatch server to the PR? Would it be possible to make the changes without adding anything else?

Yep, sorry, too many branches and repos in flight. Will clean up and reopen later.