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

perf: bulk write candles to db #481

Merged
merged 5 commits into from
Aug 19, 2022

Conversation

habibalkhabbaz
Copy link
Contributor

@habibalkhabbaz habibalkhabbaz commented Aug 18, 2022

Description

I am so sorry about submitting a lot of PRs nowadays 😆 but believe me it's worth it.

This PR has performance improvement which uses bulkWrite instead of looping through all candles and insert them one by one.

Related Issue

Motivation and Context

When we retrieve many candles from Binance, it will be a heavy job to save all candles one by one (many operations).
So I try to improve this with bulkWrite from mongo.

How Has This Been Tested?

Tests provided

Screenshots (if appropriate):

@uhliksk
Copy link
Contributor

uhliksk commented Aug 18, 2022

@habibalkhabbaz I'm getting this error:

{"name":"binance-api","version":"0.0.89","hostname":"5dd1512cdcb2","pid":46,"gitHash":"unspecified","server":"websocket","payload":{"command":"latest","authToken":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdXRoZW50aWNhdGVkQXQiOiIyMDIyLTA3LTI2VDE1OjU4OjU0LjczN1oiLCJpYXQiOjE2NTg4NTExMzQsImV4cCI6MTY1ODg1ODMzNH0.2LHdWlNaI8O3GxCy6RedA6dSS5H_SWRLssyTB_LC98s","data":{"page":1,"searchKeyword":"","sortBy":"sell-profit","sortByDesc":true},"isAuthenticated":true},"level":50,"err":{"message":"Unexpected token u in JSON at position 0","name":"SyntaxError","stack":"SyntaxError: Unexpected token u in JSON at position 0\n    at JSON.parse (<anonymous>)\n    at Object.handleLatest [as latest] (/srv/dist/server.js:1:116082)\n    at runMicrotasks (<anonymous>)\n    at processTicksAndRejections (internal/process/task_queues.js:95:5)\n    at async WebSocket.<anonymous> (/srv/dist/server.js:1:111309)"},"msg":"Something wrong with trailing-trade-common cache","time":"2022-08-18T18:36:08.381Z","v":0}

Edit: Maybe it is not related to this PR, but I found it after I merged into my code.

@habibalkhabbaz
Copy link
Contributor Author

habibalkhabbaz commented Aug 18, 2022

Hello @uhliksk
Kindly stop the containers and pull the latest changes and then restart your container.
Keep me updated when the issue is resolved or still exist.

Update: I think this issue is not related directly to this branch.
We have to find the reason that crashing the trailing-trade-common cache and solve it in another PR.

@uhliksk
Copy link
Contributor

uhliksk commented Aug 18, 2022

@habibalkhabbaz Ok, I'll try.

@uhliksk
Copy link
Contributor

uhliksk commented Aug 18, 2022

@habibalkhabbaz After restarting container I got:

{"name":"binance-api","version":"0.0.89","hostname":"07b69c23226d","pid":46,"gitHash":"unspecified","level":50,"err":{"message":"Unable to fully release the lock on resource \"redlock:trailing-trade-configurations:AVAXBUSD\".","name":"LockError","stack":"LockError: Unable to fully release the lock on resource \"redlock:trailing-trade-configurations:AVAXBUSD\".\n    at loop (/srv/node_modules/redlock/redlock.js:282:18)\n    at /srv/node_modules/redlock/redlock.js:462:10\n    at tryCatcher (/srv/node_modules/standard-as-callback/built/utils.js:12:23)\n    at /srv/node_modules/standard-as-callback/built/index.js:22:53\n    at runMicrotasks (<anonymous>)\n    at processTicksAndRejections (internal/process/task_queues.js:95:5)"},"msg":"Unable to fully release the lock on resource \"redlock:trailing-trade-configurations:AVAXBUSD\".","time":"2022-08-18T18:48:44.442Z","v":0}

It looks like it is not very healthy to restart container at any time. Maybe we should do some cleanup on container startup to prevent unpredictable states in database.

@habibalkhabbaz
Copy link
Contributor Author

@uhliksk
You are right 💯
We have to work on this in another PR.

@chrisleekr chrisleekr added the enhancement New feature or request label Aug 19, 2022
@chrisleekr
Copy link
Owner

chrisleekr commented Aug 19, 2022

I am so sorry about submitting a lot of PRs nowadays laughing but believe me it's worth it.

It's amazing, man. I really appreciate your contributions.
This bot will not improve without your contributions. Same to you @uhliksk

Is this ready to review? @habibalkhabbaz

Ah, there is conflict. Let me know if you resolve it. 😄

@habibalkhabbaz
Copy link
Contributor Author

@chrisleekr
Thank you. We all together doing great job.

Conflict resolved and it's ready for review as well.

@chrisleekr chrisleekr merged commit b7dcc30 into chrisleekr:master Aug 19, 2022
@chrisleekr
Copy link
Owner

As always, looks good to me.

@habibalkhabbaz habibalkhabbaz deleted the perf/bulk-write-candles branch October 7, 2022 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants