-
Notifications
You must be signed in to change notification settings - Fork 46
Add LOAD_TEST_FILE
configuration to enable load testing
#518
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: main
Are you sure you want to change the base?
Conversation
LOAD_TEST_FILE
configuration to enable load testingLOAD_TEST_FILE
configuration to enable load testing
LOAD_TEST_FILE
configuration to enable load testingLOAD_TEST_FILE
configuration to enable load testing
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…nto load-testing-flags
BufferedStorageBackendConfig ledgerbackend.BufferedStorageBackendConfig | ||
DataStoreConfig datastore.DataStoreConfig | ||
|
||
LoadTestFile string |
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 the only addition - the other changes are just grouping the parameters based on functionality rather than in a monolithic, aligned list.
backend = loadtest.NewLedgerBackend(loadtest.LedgerBackendConfig{ | ||
NetworkPassphrase: cfg.NetworkPassphrase, | ||
LedgersFilePath: cfg.LoadTestFile, | ||
LedgerCloseDuration: time.Second * 5000, |
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.
in horizon we have 2 more configuration parameters:
https://github.com/stellar/go/blob/master/services/horizon/cmd/ingest.go#L88-L112
merge
- configures whether we merge real ledgers with the synthetic ledgers in the load testclose-duration
- configures the close time. This is pretty important because we can use it to test if we're able to handle a faster close duration. We configure this value to 2 seconds for the horizon load tests
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.
Yeah I kept it simple on this pass to do the initial load testing: I will add the other parameters (and restoration ability) later as I feel that's lower priority given our small retention window and staging-only deployment for mainnet in RPC.
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.
for the initial load testing runs, you should hardcode LedgerCloseDuration to 2 seconds because at that rate we get 500 tps with the ledger file I sent you.
We also should do two runs one with merging enabled and one with merging disabled. I would like to compare the run with merging enabled against the previous load testing attempt I did a few months ago. Running with merging disabled is also useful to see if RPC's results are similar to horizon (horizon was significantly faster with merging disabled) .
could you setup a feature branch where you can target the subsequent changes for the remaining features? I think it will be easier to review the entire feature branch once it's ready. However, if it makes it easier to have the code in master for testing purposes we can scrap the feature branch idea and I can approve this PR when it's ready
In horizon we introduced a separate https://github.com/stellar/go/blob/master/services/horizon/internal/ingest/main.go#L650-L669 I would recommend implementing the same pattern here. There is also a https://github.com/stellar/go/blob/master/services/horizon/cmd/ingest.go#L270 |
What
This introduces a toml configuration parameter enabling passing a file with ledgers bundled in to perform load testing instead of Captive Core-based ingestion.
Why
We need to evaluate ingestion performance with ledgers that are denser than what is currently available on pubnet, see #503 et al.