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

91 transactions api performance #92

Merged
merged 4 commits into from
Aug 27, 2019

Conversation

atul-hedera
Copy link
Contributor

Fixes issue #91

Atul Mahamuni and others added 3 commits August 25, 2019 05:54
vs_nanos since (a) these fields will be removed, and (b) these don't
have indexes - making the queries slow.
2. Added support for caching of responses
3. Added gzip (compression) support
4. Fixed a bug to handle decimal pointin /balances?timestamp=ddddd.dddd
query
5. Fixed naming for accounts.test file
Copy link
Contributor

@mike-burrage-hedera mike-burrage-hedera left a comment

Choose a reason for hiding this comment

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

The cache strategy doesn't make sense.

Use normal cache headers and web caching (or don't bother with caching for now). The better cache solution would be to cache both the page response and query results using something like memcached, and utilize standard HTTP cache headers.

If the data requested is for fixed/unchanging data - cache it. If not, don't cache it. Everything is cached the same way here. So if we know the data for "timestamp_ns=lt:12345" since we've processed up to or past that timestamp, that can be cached, but "timestamp_ns=lt:9999999" where that's past the time we've processed, should not be cached at all. If we cache the latter, we're basically adding a long delay until somebody can get the fresh data.

rest-api/utils.js Outdated Show resolved Hide resolved
rest-api/utils.js Outdated Show resolved Hide resolved
rest-api/utils.js Outdated Show resolved Hide resolved
rest-api/utils.js Show resolved Hide resolved
rest-api/balances.js Show resolved Hide resolved
rest-api/events.js Outdated Show resolved Hide resolved
rest-api/transactions.js Outdated Show resolved Hide resolved
rest-api/transactions.js Outdated Show resolved Hide resolved
Copy link
Contributor

@gregscullard gregscullard left a comment

Choose a reason for hiding this comment

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

Rename sql migration file to 1.9__ ... to avoid clashes with other PRs.

Disabled cache for now, and took care of all other reivew comments
Copy link
Contributor

@mike-burrage-hedera mike-burrage-hedera left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor

@gregscullard gregscullard left a comment

Choose a reason for hiding this comment

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

Looks good

@steven-sheehy steven-sheehy merged commit f2ab1b9 into master Aug 27, 2019
@steven-sheehy steven-sheehy deleted the 91-transactions-api-performance branch August 27, 2019 03:18
@steven-sheehy steven-sheehy added this to the 0.1.0 milestone Aug 27, 2019
@steven-sheehy steven-sheehy added rest Area: REST API bug Type: Something isn't working P1 performance labels Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Type: Something isn't working P1 performance rest Area: REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants