[PIE-1707] Implement a timeout in TransactionMessageProcessor #1604
Conversation
- `processTransactionsMessage` now takes a `keepAlive` parameter - don't process the message if expired - add unit tests - use a default timeout for transactions (1 minute)
| final Instant startedAt, | ||
| final Duration keepAlive) { | ||
| // Check if message not expired. | ||
| if (startedAt.plus(keepAlive).isAfter(now())) { |
There was a problem hiding this comment.
Don't you provide any log or some feedback in case you skip the message?
There was a problem hiding this comment.
No because potentially we will drop thousands of messages and logging would be a bottleneck. A potential follow up PR would gather information and maintain a counter as a prometheus metric to see the total number of dropped messages.
There was a problem hiding this comment.
I will create a follow up PR after this one to keep it small.
There was a problem hiding this comment.
As discussed in #1586 we have to deal with up to 500 tx / second. We want to drop oldest messages because they are more likely to contain non relevant transactions. So we check if the message has expired prior to process it. Since we would probably have to skip thousands of messages we don't want to log each individual message skipped. A follow up PR will be created after this one to ensure we have proper feedback.
...t/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionsMessageProcessorTest.java
Outdated
Show resolved
Hide resolved
...t/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionsMessageProcessorTest.java
Show resolved
Hide resolved
…transactions/TransactionsMessageProcessorTest.java Co-Authored-By: Nicolas MASSART <[email protected]>
…tx-msg-processor-timeout
PR description
In order to avoid the transaction pool to grow unbounded, a maximum capacity has been implemented in #1586. This follow up PR enable to skip processing a transaction if too old.
processTransactionsMessagenow takes akeepAliveparameterFixed Issue(s)