ACCUMULO-4153: Update the getCodec method to no longer be synchronize…#106
ACCUMULO-4153: Update the getCodec method to no longer be synchronize…#106asfgit merged 1 commit intoapache:1.6from
Conversation
|
@madrob When are you cutting 1.7.2? I'd like to make an additional change per @keith-turner and get this into 1.7.2, but don't wanna to slow you down. |
|
Made some change per Keith to avoid synchronization entirely. I initially didn't notice those two functions causing issues; however, I agree that the synchronization isn't necessary. Removing it by using a codec cache when we aren't using the default buffer size for that specific codec. I'm kind of iffy on the design, but any input by those familiar with the code would be appreciated. |
|
|
||
| @Override | ||
| public Boolean call() throws Exception { | ||
| Assert.assertNotNull(Compression.Algorithm.GZ.getCodec()); |
There was a problem hiding this comment.
If an assertion throws an exception in a background thread, will this fail the test?
|
I'm looking at branching after ACCUMULO-4157 makes it in, so there is still time. |
|
@madrob FWIW, the build.sh branches for you, if it helps. Of course, you can still branch first, then let build.sh branch again. It's all local anyway. |
| * the default within the codec. | ||
| * | ||
| * There is a Guava cache defined within Algorithm that allows us to cache Codecs for re-use. Since they are immutable, there is no concern for using them | ||
| * concurrently; however, the Guava cache exists to ensure a maximal size of the cache and efficient and concurrent read/write access to the cache itself. |
There was a problem hiding this comment.
@phrocker a few days ago you told me offline that the Codecs maintain an internal reference to a Configuration object. This was why the old code would sync, change the config, then create the stream. When you explained this everything clicked for me. You did not find that w/o some spelunking, might be useful to work this insight into the comments.
|
@phrocker I am finished looking at this. +1 |
|
Would anyone be opposed to me squashing this into a single commit? |
| } | ||
|
|
||
| @Override | ||
| CompressionCodec getCodec() throws IOException { |
There was a problem hiding this comment.
Can this 'throws IOException' be removed?
There was a problem hiding this comment.
They exist because in the codecs that aren't the default ( gz ), they will throw an IOException when the isSupported is false and getCodec is called (i.e. you called it even if a check to isSupported is false.
I'm not. |
Update the getCodec method to no longer be synchronized using static initializer in enum Update so that we use a codec cache if we are not using the default buffer size for each specific codec. LZO does not need this change. Update to improve comments and other readability concerns. Update tests to check all codecs. Add checks for failures in executor. Instead of more unit tests with Assume checks, we'll simply use a map and loop in existing unit tests to check all codecs. A failure in one will cause a failure
|
Unfortunately this slipped through the cracks and did not make it into 1.7.2. Bummer, I really wanted this in 1.7.2. I merged this from 1.6 to master, running |
7 participants
…d using static initializer in enum
Targeting 1.6 as that is the branch we are currently using. I'd rather re-target than ask someone to cherry-pick back.