Verify keys on read #495
Verify keys on read #495
Conversation
| Tracing.trace("Enqueuing response to {}", message.from); | ||
| MessagingService.instance().sendReply(reply, id, message.from); | ||
|
|
||
| OwnershipVerificationUtils.verifyRead(keyspace, command.key); | ||
| } |
There was a problem hiding this comment.
note that this verification is intentionally after enqueuing the reply in order to avoid introducing additional latency on the read path. We should consider moving it earlier and throwing an exception after understanding in what circumstances this violation is logged, if any.
| { | ||
| if (cacheWasRecentlyRefreshed()) | ||
| { | ||
| throwInvalidMutationException(mutation, keyspace, cachedNaturalEndpoints, pendingEndpoints); | ||
| handler.onViolation(keyspace, key, cachedNaturalEndpoints, pendingEndpoints); |
There was a problem hiding this comment.
an invalid read won't throw an exception here so it will constantly trigger refreshCache on every read, which could potentially be problematic. similarly it will still call handler.onValid(keyspace); at the botton
i think we should define a InvalidReadException and handle it here and below without a refresh
There was a problem hiding this comment.
good catch! To avoid depending on the presence of an exception for correct control flow, I just added some returns in the correct place.
| { | ||
| private static final Logger logger = LoggerFactory.getLogger(OwnershipVerificationUtils.class); | ||
|
|
||
| public static final OwnershipVerificationHandler INSTANCE = new MutationVerificationHandler(); |
There was a problem hiding this comment.
why wouldn't this be of type MutationVerificationHandler and similarly for ReadVerificationHandler
There was a problem hiding this comment.
discussed offline, I'm just following the List l = new ArrayList(); pattern where you always use the most generic interface where possible.
Similar to the motivation for our verify_keys_on_write feature, it appears to be possible for a node to respond to a ReadVerb even if it does not think it owns the data. This PR adds an invalidReads metric behind a similar feature flag so we can start to see if this is actually occurring.
This would cause logical corruption if a client were to make a read request on a coordinator with an out of data view of the ring and received a null after committing a write, and could potentially result in resurrected data if an invalid read is written as part of a read repair.
This PR also adds SafeLogging arguments to the existing
VerifyKeysOnWriteerrors.