Enable parallel apply by default, remove experimental flag #5055
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the "experimental" designation from parallel ledger apply functionality by renaming EXPERIMENTAL_PARALLEL_LEDGER_APPLY to PARALLEL_LEDGER_APPLY and changing its default value from false to true. The change includes a deprecation warning for the old config name and updates all references throughout the codebase.
- Renamed configuration field and updated all references in code and tests
- Changed default behavior to enable parallel ledger apply by default
- Added deprecation warning for old configuration name
- Updated test configurations to accommodate the new default (SQLite-based tests explicitly disable the feature)
- Changed default test database from in-memory SQLite to PostgreSQL to support the new default
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/Config.h | Renamed field from EXPERIMENTAL_PARALLEL_LEDGER_APPLY to PARALLEL_LEDGER_APPLY |
| src/main/Config.cpp | Changed default to true, added deprecation warning, updated validation and logging |
| src/util/test/TimerTests.cpp | Updated config field reference |
| src/herder/test/HerderTests.cpp | Updated config field references across multiple test cases |
| src/main/test/ConfigTests.cpp | Updated config field references and explicitly set to false for SQLite-based config tests |
| src/test/test.cpp | Changed default test database mode from TESTDB_BUCKET_DB_VOLATILE to TESTDB_POSTGRESQL to support new default |
| docs/stellar-core_example_validators.cfg | Updated example to use PostgreSQL instead of SQLite |
| src/bucket/LiveBucketIndex.cpp | Added include for JitterInjection.h (appears unrelated to PR scope) |
marta-lokhova
force-pushed
the
enableParallelApply
branch
4 times, most recently
from
62a6064 to
c59d346
Compare
src/main/PersistentState.cpp
Outdated
| st.define_and_bind(); | ||
| { | ||
| ZoneNamedN(selectStoreStateZone, "select storestate", true); | ||
| ZoneNamedN(selectStoreStateZone, "select slotstate", true); |
There was a problem hiding this comment.
This looks suspicious, as selectStoreStateZone name is still unchanged.
src/database/Database.cpp
Outdated
|
|
||
| void | ||
| Database::clearPreparedStatementCache(SessionWrapper& session, | ||
| bool assertOnlySession) |
There was a problem hiding this comment.
assertOnlySession is unused now.
|
heads up, I'm fixing a couple of issues flagged by CI and adding tests. |
marta-lokhova
force-pushed
the
enableParallelApply
branch
2 times, most recently
from
a66b8cd to
33522a9
Compare
src/main/PersistentState.cpp
Outdated
| releaseAssert(threadIsMain() || | ||
| mApp.threadIsType(Application::ThreadType::APPLY)); | ||
| updateDb(getStoreStateName(entry), value, session, getDBForEntry(entry)); | ||
| if (entry > kRebuildLedger) |
There was a problem hiding this comment.
nit: Now that we have kLastEntryMain, I would suggest using it everywhere instead of kRebuildLedger. It might also be useful to make kLastEntryMain to have the same value as kRebuildLedger, or name it kMainEntryCount. Either option would result in more clear inclusive/exclusive ranges, as currently the name suggests that it's an entry id, but in fact represents the entry count.
The same goes for kLastEntry.
There was a problem hiding this comment.
Good idea, done.
marta-lokhova
force-pushed
the
enableParallelApply
branch
2 times, most recently
from
836cccc to
ce0f9bb
Compare
marta-lokhova
force-pushed
the
enableParallelApply
branch
from
ce0f9bb to
1bf005b
Compare
| // Return true if the Database target is SQLite, otherwise false. | ||
| bool isSqlite() const; | ||
|
|
||
| // Return true is the Database can use a miscellaneous database |
2 participants
No description provided.