-
Notifications
You must be signed in to change notification settings - Fork 87
Feature/binaries storage #51
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
base: master
Are you sure you want to change the base?
Conversation
- Add io.js for file path generation, record reading/writing, and metadata management. - Introduce resample.js to handle resampling of market data to higher timeframes with upsert semantics. - Create write.js for appending and upserting bars to binary files, including null bar handling for dense indexing. - Implement functions for reading and writing binary records, ensuring efficient file operations and metadata updates.
|
Looking at it. |
Thanks 🙏 |
adeacetis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Tucsky,
Here's my 2 cents about the commit on error handling.
Hope that helps.
| ].includes(code) | ||
| } | ||
|
|
||
| async requestWithRetry( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea to implement, but http requests, error handling, retries, logging should be managed by a HttpClient base class and not the Exchange class.
For instance, formatErrorForLog and getErrorStatus are generic helpers. They do not need to live here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's been a long-lasting issue in the code, but since you are there maybe we can start to refactor it. Maybe I can shime in, once you're done with the binaries storage
| ) { | ||
| let attempt = 1 | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While (true) ? Is this AI Slop ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is...😅
| } | ||
|
|
||
| console.warn( | ||
| `[${this.id}] retrying ${label}${pairLabel} (${attempt + 1}/${maxAttempts})`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you were to re-architecture this, please pass the Exchange id as an argument to the function instead of referencing the private property.
|
Also, the eslint commit (thank you for that) could have been part of another PR. Eslint improves the version number to 1.2.1 |
I subscribe to the fact we need to move on from Influx v1 but there are other possibilities in the market such as TimeScaleDB or QuestDB which are both using PostgreSQL's SQL Protocol. |
|
Converted the PR to draft. |
scripts/coinalize/index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we remove this script right now?
scripts/coinalize/utils.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question: why deleting coinalize? It's the sole data provider we use to backfill historical data. Is there a replacement ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check but i think it has not been working since 2-3 years due to Cloudflare blocking the requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sato-ke has shared an implementation in the Discord server no later than last year.
https://discord.com/channels/1110160121813803058/1110584647789838437/1380748613843423362
.prettierrc.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bye prettier.
|
|
||
| export default [ | ||
| { | ||
| ignores: ["node_modules/**", "dist/**", "coverage/**", "tmp/**", "eslint.config.mjs"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe can we add the data/**. I think this is the default path for collection data. Maybe not recommended but that could make eslint very slow.
- Updated BinariesStorage to read from segmented files, computing which segments intersect the requested time range. - Introduced new functions for generating file paths for segments and reading/writing segment metadata. - Modified resampling logic to handle segmented base and target timeframe files, ensuring proper aggregation and upsert semantics. - Enhanced upsertBars function to manage bars by segment, allowing for efficient writing and handling of overwrites and appends. - Updated documentation to reflect changes in file structure and functionality.
The storage layer already behaves as a pluggable abstraction: legacy Influx stays for compatibility. |
The goal is to get rid of influx and improve performance, cost, versatility