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.

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

feat/Include drivers for Postgres and MySQL/MariaDB by default #5452

Closed
wants to merge 2 commits into from
Closed

feat/Include drivers for Postgres and MySQL/MariaDB by default #5452

wants to merge 2 commits into from

Conversation

cheemeng
Copy link

@cheemeng cheemeng commented Jul 1, 2022

Current docker image only includes the driver for SQLite.

I believe those of us running many instance of Hummingbot, or want a centralised db storage (for post-trade analytical purposes) would prefer logging trades in a different RDBMS such as PostgreSQL or MySQL/MariaDB. However, this means we would need to build the docker image ourselves every time a new release of Hummingbot is out, in order to work with a non-SQLite database.

@MHHukiewitz
Copy link
Contributor

To help me understand why this is useful: So your workflow would be to install the (updated) Docker image, then use the new Python modules to do what? Wouldn't you need to recompile your own Docker images anyways, if you would add Python scripts allowing you to store the data to your remote DBs?

@cheemeng
Copy link
Author

cheemeng commented Jul 1, 2022

To help me understand why this is useful: So your workflow would be to install the (updated) Docker image, then use the new Python modules to do what? Wouldn't you need to recompile your own Docker images anyways, if you would add Python scripts allowing you to store the data to your remote DBs?

No these are not for custom scripts, but to enable storing of trade info into a centralised DB vs a SQLite file (cf. https://hummingbot.org/global-configs/external-db/) out of the box.

If you are running 20+ docker containers and each of them do frequent trades, SqlAlchemy's ORM via SQLite isn't exactly the most performant out there (cf. https://stackoverflow.com/questions/11769366/why-is-sqlalchemy-insert-with-sqlite-25-times-slower-than-using-sqlite3-directly), and you will end up with lots of disk thrashing.

It's also useful to have a central location to do analysis of trades instead of having to get data from individual SQLite file.

Copy link
Contributor

@aarmoa aarmoa left a comment

Choose a reason for hiding this comment

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

It does not make sense to add more dependencies to libraries for particular DBs. Each user should configure that in their own installation, and it should not be part of the official repository.

-- Reviewed in representation of Hummingbot Foundation Technical DAO

@cheemeng
Copy link
Author

cheemeng commented Jul 4, 2022

Each user should configure that in their own installation, and it should not be part of the official repository.

But it makes sense to only have dependency for SQLite built-in? 🤷‍♂️

Joke aside, would it be better to say, have a different environment.yml file only for the purpose of building a docker image so that it can run out-of-the-box with a db driver other than SQLite? Because when running a cluster of docker containers and all of them write to different SQlite files at the same time, it really cause problems with disk thrashing (backlog disk I/O leading to stalled instance, leading to high memory consumption).

@cheemeng
Copy link
Author

cheemeng commented Jul 4, 2022

Or perhaps there is a better way to do it so that the official docker image we pull can already support other RDBMS?

@cardosofede
Copy link
Contributor

@cheemeng @aarmoa Mike opened a discussion in CommonWealth for this topic. This is the link: https://commonwealth.im/hummingbot-foundation/discussion/6013-support-for-external-dbs

@nikspz
Copy link
Contributor

nikspz commented Dec 22, 2022

Closing PR, snapshot vote: failed

@nikspz nikspz closed this Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants