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

Disable Otterscan indices by default #1987

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Disable Otterscan indices by default #1987

merged 1 commit into from
Dec 11, 2024

Conversation

JamesHinshelwood
Copy link
Contributor

This significantly improves performance on nodes which don't need to serve the ots_ APIs.

produce-full/produce-full
                        time:   [1.0781 s 1.2170 s 1.3138 s]
                        thrpt:  [0.7612  elem/s 0.8217  elem/s 0.9275  elem/s]
                 change:
                        time:   [-50.851% -43.988% -35.863%] (p = 0.00 < 0.05)
                        thrpt:  [+55.917% +78.532% +103.46%]
                        Performance has improved.

@rrw-zilliqa
Copy link
Contributor

Is there any chance of returning flags like this in the GetVersion() output so that otterscan has a chance of warning you that you're about to encounter errors, rather than ots just failing out silently and leaving the user to guess that something went wrong somewhere?

@JamesHinshelwood JamesHinshelwood changed the base branch from api-config to main December 9, 2024 21:40
@Zilliqa Zilliqa deleted a comment from github-actions bot Dec 9, 2024
Copy link
Contributor

github-actions bot commented Dec 9, 2024

🐰 Bencher Report

Branchdisable-ots
Testbedself-hosted
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)
Upper Boundary
nanoseconds (ns)
(Limit %)
process-empty/process-empty📈 view plot
🚷 view threshold
9,402,300.00
(+1.27%)
10,848,496.60
(86.67%)
produce-full/produce-full📈 view plot
🚷 view threshold
1,974,300,000.00
(-21.95%)
3,010,337,941.91
(65.58%)
🐰 View full continuous benchmarking report in Bencher

@JamesHinshelwood
Copy link
Contributor Author

Hmm. My concern with exposing this information in a separate API is that there are lots of possible reasons some an API may not work:

  • The API is not enabled in configuration
  • The required indices are not enabled
  • The node version you are running does not have this API
  • The data required by this API has been pruned (in the future)

I think that trying to expose all this information separately and let the client decide whether it should call an API or not will lead to pain. Instead, I think we should settle for returning good error messages from APIs if they are called and don't work (for one of the above reasons). And the client should handle those errors in some reasonably graceful manner.

In other words, I don't think the client should try to decide "Am I able to call the X API?". Instead it should just call the X API and see if it works.

This significantly improves performance on nodes which don't need
to serve the `ots_` APIs.

```
produce-full/produce-full
                        time:   [1.0781 s 1.2170 s 1.3138 s]
                        thrpt:  [0.7612  elem/s 0.8217  elem/s 0.9275  elem/s]
                 change:
                        time:   [-50.851% -43.988% -35.863%] (p = 0.00 < 0.05)
                        thrpt:  [+55.917% +78.532% +103.46%]
                        Performance has improved.
```
@JamesHinshelwood JamesHinshelwood added this pull request to the merge queue Dec 11, 2024
Merged via the queue into main with commit e4596d7 Dec 11, 2024
6 checks passed
@JamesHinshelwood JamesHinshelwood deleted the disable-ots branch December 11, 2024 14:57
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.

3 participants