soda-core icon indicating copy to clipboard operation
soda-core copied to clipboard

Fix YamlHelper for thread safety in multi-threaded environments.

Open NiklasA opened this issue 1 year ago • 6 comments

Overview

This pull request addresses the thread safety issue with YamlHelper as reported in Issue #2064. The modifications ensure that YamlHelper can be safely used in multi-threaded environments, preventing potential data races and concurrency errors.

@m1n0 Can you please review the changes and provide feedback or approve the merge if everything looks in order. Thank you!

NiklasA avatar Sep 13 '24 15:09 NiklasA

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 13 '24 15:09 CLAassistant

hi @NiklasA, thanks for looking into this and thanks for your patience! I am thinking that opening a thread for this is a bit of an overkill. I looked into it myself and it seems to me that all that needs to happen is to have a new yaml instance instead of reusing the same one like this https://github.com/sodadata/soda-core/pull/2188 - my local testing confirms it working, so I merged it to unblock a release, please test it and get back to me. We can definitely revisit your proposal for the fix if it still does not fix the issue

m1n0 avatar Nov 28 '24 19:11 m1n0

Hi @m1n0 ,

thank you for your detailed response! We’ve tested your approach and compared it to our existing implementation. While your solution works well, we’ve observed that its performance is approximately 10% slower than our current approach (within our existing load tests).

For context, we are using the datacontract-cli as part of our data ingestion processes, where we need to verify a significant amount of data. This might represent an edge case, but in these scenarios, performance is critical for us.

We appreciate your input and are happy to discuss potential optimizations or alternative approaches that might bridge this gap. Let us know your thoughts!

Best regards Niklas

NiklasA avatar Nov 28 '24 19:11 NiklasA

FYI: @PascalRosenberger, @ManbirP and @SebastianHirsch

NiklasA avatar Nov 28 '24 19:11 NiklasA