'publish' performance optimization on high parallelism, prevent lock if not needed #1185
Conversation
…n high parallelism, prevent lock if not needed
|
@tulios , need to fix few tests that check the amount of calls to 'refreshMetadata' which is now reduced. |
| if ( | ||
| this.brokerPool.metadata && | ||
| this.previousTopics && | ||
| topics.every(topic => this.previousTopics.has(topic)) |
There was a problem hiding this comment.
why do we need to use previousTopics here? why not just targetTopics ? And related, why do we need to make previousTopics an object property?
There was a problem hiding this comment.
@t-d-d , saving 'previousTopics' as an object property is needed since here we are not holding the lock, and other flow could be already inside the lock and modified 'targetTopics' before calling 'refreshMetadata', so the 'targetTopics' might be reverted on 'refreshMetadata' error. So saving the 'previousTopics' is needed as the latest topics that successfully finished the 'refreshMetadata'
|
@t-d-d |
|
Facing similar issue. 0|ludo-ws-s1 | KafkaJSNonRetriableError: Timeout while acquiring lock (2 waiting locks): "updating target topics" |
|
I am facing this issue also during my high scale performance testing to simulate our production scenario. Without forking this repo and making this simple code change we can't use this library. Please merge it in! |
|
I'm facing the exact same issue. Could these changes be merged? I saw #667 is closed |
Every message 'publish' goes through 'addMultipleTargetTopics' which always takes an async lock.
The lock becomes slow on high publish parallelism (few thousands waiters or more) and could cause errors like:
KafkaJSLockTimeout: Timeout while acquiring lock (2162 waiting locks): "updating target topics"
This PR prevents taking the lock and increases the throughput by ~20% for medium loads and by more for high loads. As well as reducing the need to increase requestTimeout on high parallelism.