putShortVolatile is not volatile in InMemoryTrie#4820
Conversation
| final void putShortVolatile(int pos, short value) | ||
| { | ||
| getChunk(pos).putShort(inChunkPointer(pos), value); | ||
| getChunk(pos).putShortVolatile(inChunkPointer(pos), value); |
There was a problem hiding this comment.
potentially a copy-paste bug
| ensuringInBuildInternalContext(operationType), | ||
| getIOOptions().flushCompression, | ||
| getCompressionDictionaryManager()); | ||
| dataWriterOpened = true; |
There was a problem hiding this comment.
this is missing setting dataWriterOpened. This doesn't seem to have any functional issues at the moment, but adding the fix for completeness
| { | ||
| sstable.markSuspect(); | ||
|
|
||
| if (reader != null) |
There was a problem hiding this comment.
when an exception occurs, we have a reader that might not get closed properly
| // Follow end position while we still have a prefix, stacking path. | ||
| go(root); | ||
| while (true) | ||
| try |
There was a problem hiding this comment.
this is conforming to the implementation in org.apache.cassandra.io.tries.ValueIterator#initializeWithLeftBound
iamaleksey
left a comment
iamaleksey
left a comment
There was a problem hiding this comment.
The PR looks good, and does what it says it does, and doesn't introduce any new (non-latent) issues, but there are a few things that can be tightened up.
| @@ -140,6 +144,19 @@ protected AbstractSSTableIterator(SSTableReader sstable, | |||
| catch (IOException e) | |||
There was a problem hiding this comment.
UnfilteredValidation.handleInvalid() can throw a CorruptSSTableException which is an RTE so it wouldn't be caught here and we'd still have a leak.
There was a problem hiding this comment.
good point, I'm thinking of handling both CorruptSSTableException | IOException. For the CorruptSSTableException case we rethrow after closing.
There was a problem hiding this comment.
So, both here and for ValueIterator it would probably be better to wrap the entire if-else block in the constructors. So that if e.g. limit.next() call throws, we still clean up. It would also reduce the visual nesting where it matters more.
There was a problem hiding this comment.
This minimizes the diff, but it actually makes more sense to handle it at the contractor instead
iamaleksey
left a comment
iamaleksey
left a comment
There was a problem hiding this comment.
LGTM with the extra commits 👍
On commit though, can you take a look at the other constructors of ValueIterator / ReverseValueIterator and ensure they also can't leak?
This commit fixes several issues found using the Cassandra edge case explorer skill for the BTI feature. patch by Francisco Guerrero; reviewed by Aleksey Yeschenko for CASSANDRA-21353
2 participants
This commit fixes several issues found using the cassandra edge case explorer skill for the BTI feature. The majority are small findings.
Patch by Francisco Guerrero; reviewed by TBD for CASSANDRA-21353