druid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gian Merlino <g...@apache.org>
Subject Re: compression strategy concurrency
Date Wed, 15 Sep 2021 05:41:28 GMT
Hey Rahul,

What kind of errors are you seeing? I ran the test a few times with a
bumped up number of threads, and I did see a few problems but they were in
the Closer. It looks like a single Closer is used for every thread, which
is bad because Closers are not thread-safe (they are built around an
ArrayDeque and don't use synchronization). But that's a problem with the
test. (That should be fixed, ideally!)

As to the actual CompressionStrategy methods, it looks like the various
implementations all return singletons from getDecompressor, getCompressor.
So they better be thread safe! The LZ4 one uses singleton instances of
LZ4Factory.fastestInstance().safeDecompressor() and
LZ4Factory.fastestInstance().highCompressor(). Both of those look
thread-safe to me. The LZF and Uncompressed ones don't use anything at all
so they seem ok too.

On Tue, Sep 14, 2021 at 2:29 PM rahul gidwani <churro@apache.org> wrote:

> What is the desired thread safety of the CompressionStrategy class?  From
> looking at it from an API perspective, it looks to be you:
>
> Allocate input buffer, Allocate output buffer, compress / decompress.
>
> The CompressionStrategyTest.testConcurrency() test if you bump the number
> of threads to 100, and run it a few times, you will see there are race
> conditions which will cause failures.
>
> The quick and easy solution to make this thread safe is to synchronize the
> methods.  But in reality it looks like this class is mainly used to
> compress, decompress segments so that will be a a thread / segment which is
> okay.
>
> My question is I am confused as to what should be the behavior, should it
> be thread safe and a generic api to compress / decompress.  If so then we
> should fix the code in Compression Strategy to be thread safe, if not then
> maybe remove that test and mark the class as NotThreadSafe.
>
> Was wondering about other people's thoughts.
> Thanks
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message