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

Feature/streaming report #389

Merged
merged 27 commits into from
Dec 5, 2022
Merged

Feature/streaming report #389

merged 27 commits into from
Dec 5, 2022

Conversation

tdroxler
Copy link
Member

@tdroxler tdroxler commented Nov 17, 2022

Resolves: #374

We are still waiting tapir to release a new version, but my PR got merged few days ago .

I still miss the tests, but I think it can be good to start reviewing it, as it's a new concept.

With streaming, when you call the endpoint, you get a response directly:

Request: GET /addresses/1Gzp3Wc42fanrbAqknYmSaZLfC567bP3JWprDCiuJ8rbP/export-transactions/json?toTs=1668701628067&fromTs=1637362800000, handled by: GET /addresses/{address}/export-transactions/json, took: 81ms; response: 200

from the backend side we first fetch the txIds as a stream and as soon psql have the first txIds ready, it starts to push them down the flow, for each txId we fetch the tx detail and it goes down the stream to the http response. The user is dowloading each tx, like any download, we don't wait to have ALL tx to return them.
As it's reactive streams, the all things is using backpressure.

Using streams allows to be able to download for example the 1M+ txs from our biggest miner without issue.

cc/ @mvaivre for the two new endpoints:

/addresses/<address>/export-transactions/csv?fromTs=1234&toTs=5678
/addresses/<address>/export-transactions/json?fromTs=1234&toTs=5678

max time interval is 365 days

This function streams the wanted transactionIds from the db, then
for each txId, the full transaction is fetch and returned to the user
using backpressure.

As a user, the rest endpoint response almost immediately with a 200 and
then it start to download the transactions.

This allows to easily download 1M tx without timeouts.
Copy link
Member

@simerplaha simerplaha left a comment

Choose a reason for hiding this comment

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

Pretty cool how streaming is plugged in. Looks neat! 👍🏼

@tdroxler tdroxler changed the title [Do Not Merge] Feature/streaming report Feature/streaming report Nov 22, 2022
Copy link
Member

@polarker polarker left a comment

Choose a reason for hiding this comment

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

Great job 👍 Most are nitpicking questions and comments.

Keeping the code behind it in services as it doesn't hurt.
When not providing a parent, we were always generated a genesis block,
which isn't what we want for most of the tests.
Without batching we download between 500-900KB/s and with a batching at
100 it's around 1.5-2MB 🚀
@tdroxler
Copy link
Member Author

With the two latest commits, we are now refusing to export addresses with more than 10K txs (within the time range). Currently the biggest exportable address has 8749 txs, and it takes 4s to dl the 6M csv file.

@tdroxler
Copy link
Member Author

tdroxler commented Dec 1, 2022

Fighting a flaky test and after that we should be good.

@tdroxler
Copy link
Member Author

tdroxler commented Dec 1, 2022

I start to be clueless for the flaky test, in #401 I mention that I will reduce the number of calls, but even doing only 1 call per export test fail on CI.
I'm testing few other stuff here #402 but nothing work until now.

I need a good night of sleep and hopefully it will bring me the light!

Copy link
Member

@polarker polarker left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@tdroxler tdroxler merged commit 396810f into master Dec 5, 2022
@tdroxler tdroxler deleted the feature/streaming-report branch December 5, 2022 07:54
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.

Add tapir streaming support
4 participants