search

cybercongress / cyber-search

🚀 Toolchain for transactions parsing and processing
https://cybercongress.github.io/cyber-search/
Other
33 stars 14 forks source link

Fix Kafka tests and use a single Kafka instance per test module #268

Closed KevinLiLu closed 5 years ago

KevinLiLu commented 5 years ago

resolves #255

KevinLiLu commented 5 years ago

Hey @hleb-albau @arturalbov , need some advice here.

I have identified that the tests fail when tearing down the Kafka broker (and we even see warning and error logs in CI jobs that pass) due to certain race conditions that arise when log.dirs configuration is defaulted to /tmp/kafka-logs. Seems like /tmp/ directory does not work well with spring-kafka/kafka/CI.

When changing the log.dirs to point to a subdirectory in the project directory (like this PR uses ./kafka-logs-X/), everything works perfectly and we do not see any warning or error logs at all.

One problem is the log directory has a lock that can only be used by 1 Kafka broker at a time, so if we want to use a constant log.dirs value, then we need to ensure the Kafka broker (KafkaEmbedded) instance is shared among all tests. If we want to run tests in parallel, then we must also think about that case then. I am having some trouble figuring out how to implement this shared KafkaEmbedded instance using Spring (as I am new to Spring).

Any suggestions?

A naive implementation that does not share one Kafka instance is to specify a unique log.dirs value that has the test class name suffixed. Ex: log.dirs=./kafka-logs-SinglePartitionLackOfRecordsReaderTest/. This will make it work and allow for parallel tests to be run. This is rather hacky though, so a shared Kafka instance would be preferable.

I made a new bean to hold the KafkaEmbedded object, and it works in common-kafka. I am having trouble getting it to work with the other projects however...

Got it to work! Just debugging one final exception that is in :contract-summary:bitcoin:test: java.lang.IllegalArgumentException: Could not resolve placeholder 'spring.embedded.kafka.brokers' in value "${spring.embedded.kafka.brokers}"

For some strange reason, this test passes in my IntelliJ, but not through gradle or CI...

hleb-albau commented 5 years ago

@KevinLiLu As far as i understand from that issue, we should create single kafka test-base class. Currently. there are 4-5 classes annotated with @EmbeddedKafka. So 4-5 kafka instances are spawned.

PS. Also, when you create new instance of KafkaEmbedded not via spring context, method afterPropertiesSet() and destroy() should be called manually. spring.embedded.kafka.brokers are set in afterPropertiesSet().

KevinLiLu commented 5 years ago

Hi @hleb-albau, finally got some time to resolve my previous roadblocks. This PR is now ready to review & merge! Can you do a review? Thanks!

hleb-albau commented 5 years ago

@KevinLiLu Cool, thanks for contribution!

hleb-albau commented 5 years ago

@KevinLiLu Please, submit your work on gitcoin.

KevinLiLu commented 5 years ago

@hleb-albau Submitted work. Thanks!