NC-1880 High TX volume swamps block processing #337
Conversation
Move transaction processing into it's own thread(s).
| @@ -43,6 +43,7 @@ | |||
|
|
|||
| protected final ExecutorService workerExecutor; | |||
There was a problem hiding this comment.
Should we rename this syncWorkerExecutor?
| @@ -56,13 +57,21 @@ | |||
| new ThreadFactoryBuilder() | |||
| .setDaemon(true) | |||
| .setNameFormat(EthScheduler.class.getSimpleName() + "Timer") | |||
| .build()), | |||
| Executors.newFixedThreadPool( | |||
| workerCount, | |||
There was a problem hiding this comment.
We probably want a separate count for the tx workers, right?
* Allow txWorkerExecutor threads to be independently configured * push the above to the config and across the tests
| @@ -92,7 +101,11 @@ protected EthScheduler( | |||
| } | |||
|
|
|||
| public Future<?> scheduleWorkerTask(final Runnable command) { | |||
There was a problem hiding this comment.
Sure. Also scheduleTxWorkerTask, so both align with the field name.
| @@ -744,4 +755,38 @@ public void shouldSuccessfullyRespondToGetHeadersRequestLessThanZero() | |||
| done.get(); | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void transactionMessagesGoToTheCorrectExecutor() { | |||
There was a problem hiding this comment.
nitpick - this might make more sense in the TransactionPoolFactory test, as the tx pool is the object deciding how to use the workers
There was a problem hiding this comment.
The message dispatch occurs within the EthProtocolManager, even though TransactionPool is the one registering the listener. EthProtocolManager::processMessage calls are key to this test. Also, this would result in core then depending on eth (at least the tests) and would result in a quasi-circular dependency. While it does bring the test closer to the other half of the code that is being tested EthScheduler is also as much under test. But the number of new quasi-circular imports that :ethereum:core_test would gain if I moved the test persuades me to leave it where it is.
| @@ -142,7 +142,8 @@ | |||
| protocolContext.getBlockchain(), | |||
| networkId, | |||
| fastSyncEnabled, | |||
| syncConfig.downloaderParallelism()); | |||
| syncConfig.downloaderParallelism(), | |||
| syncConfig.transactionsParallelism()); | |||
There was a problem hiding this comment.
Nitpick (optional) - it looks like SyncConfig is becoming EthConfig - maybe we should rename and move it??
There was a problem hiding this comment.
There may be more to it than a surface renaming. Spun off to NC-1993.
PR description
Move transaction processing into its own thread(s).
Fixed Issue(s)
Fixes NC-1880