CASSJAVA-130: Fix Result-Metadata Race Causing Corrupt SKIP_METADATA Row Decoding#2094
CASSJAVA-130: Fix Result-Metadata Race Causing Corrupt SKIP_METADATA Row Decoding#2094yifan-c wants to merge 1 commit into
Conversation
…Row Decoding Patch by Yifan Cai; Reviewed by TBD for CASSJAVA-130
tejalchak
left a comment
tejalchak
left a comment
There was a problem hiding this comment.
Solid fix. One follow-up question inline around toMessage worth deciding whether to thread the snapshot there too before/after merge.
| logPrefixJoiner.join(this.sessionName, nodeRequestId, currentExecutionIndex), | ||
| resultMetadataSnapshot(statement)); | ||
| Message message = Conversions.toMessage(statement, executionProfile, context); |
There was a problem hiding this comment.
I was looking at the race condition window here and wanted to get a second pair of eyes ...
While the callback now safely uses the snapshot, toMessage still reads the live metadata state. I'm wondering if a concurrent schema update between capturing the snapshot and the toMessage execution could cause a mismatch (where the request sends the new metadata ID, but the callback decodes using the old snapshot). Should toMessage accept the snapshot to close any chance of discrepancy?
Since ConversionsMetadataRaceTest calls getResultDefinitions directly rather than driving through toMessage, so it might not catch this without some change.
| static class ResultMetadata { | ||
| private final ByteBuffer resultMetadataId; | ||
| private final ColumnDefinitions resultSetDefinitions; | ||
|
|
||
| private ResultMetadata(ByteBuffer resultMetadataId, ColumnDefinitions resultSetDefinitions) { | ||
| this.resultMetadataId = resultMetadataId; | ||
| this.resultSetDefinitions = resultSetDefinitions; | ||
| } | ||
|
|
||
| ByteBuffer getResultMetadataId() { | ||
| return resultMetadataId; | ||
| } | ||
|
|
||
| ColumnDefinitions getResultSetDefinitions() { | ||
| return resultSetDefinitions; | ||
| } |
There was a problem hiding this comment.
A minor thought: if this branch is strictly Java 17+, we could consider using a record to avoid the boilerplate code
static record ResultMetadata(ByteBuffer resultMetadataId, ColumnDefinitions resultSetDefinitions) {}
3 participants
Patch by Yifan Cai; Reviewed by TBD for CASSJAVA-130