Skip to content

Invalidate the Prepared statement cache only if either of the prepare…#4542

Closed
jaydeepkumar1984 wants to merge 1 commit intoapache:cassandra-4.0from
jaydeepkumar1984:CASSANDRA-17401_4.0
Closed

Invalidate the Prepared statement cache only if either of the prepare…#4542
jaydeepkumar1984 wants to merge 1 commit intoapache:cassandra-4.0from
jaydeepkumar1984:CASSANDRA-17401_4.0

Conversation

@jaydeepkumar1984
Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/CASSANDRA-17248 added prepared statement eviction logic because there was a change in prepared statements' behavior between 4.0.1 -> 4.0.2. However, the eviction logic introduces a race condition depicted in https://issues.apache.org/jira/browse/CASSANDRA-17401

As per this discussion, we should keep the eviction logic in the 4.0 branch, but invoke only in a real corner case scenario.

This PR will evict the statements only during the corner case scenario in which either of the prepared statement caches is empty, but not both.

The Cassandra Jira

}
}
else
else if (cachedWithoutKeyspace != null || cachedWithKeyspace != null)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need something like else if ((cachedWithoutKeyspace != null) != (cachedWithKeyspace != null)), to make sure at least one of them is in cache.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me elaborate.

boolean safeToReturnCached = cachedWithoutKeyspace != null && cachedWithKeyspace != null;

We are in the else block because safeToReturnCached is false. If safeToReturnCached is false, that means either cachedWithoutKeyspace is null or cachedWithKeyspace is null or both of them.

So, at this place, if (cachedWithoutKeyspace != null || cachedWithKeyspace != null) is sufficient.

Could you please double-check?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaydeepkumar1984 thanks for elaborating, I think you're right.
Just as a nitpick, and if you get a chance, could you add a short comment saying "only evict if we know one of the statements is cached".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated

}
}
else
else if (cachedWithoutKeyspace != null || cachedWithKeyspace != null)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaydeepkumar1984 thanks for elaborating, I think you're right.
Just as a nitpick, and if you get a chance, could you add a short comment saying "only evict if we know one of the statements is cached".

@jaydeepkumar1984
Copy link
Copy Markdown
Contributor Author

Pushed as 077b7eb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants