4.1: CASSANDRA-19703: Ensure prepared_statement INSERT timestamp precedes eviction DELETE

[tolbertam]

Updates SystemKeyspace.writePreparedStatement to accept a timestamp
associated with the Prepared creation time. Using this timestamp
will ensure that an INSERT into system.prepared_statements will
always precede the timestamp for the same Prepared in
SystemKeyspace.removePreparedStatement.

This is needed because Caffeine 2.9.2 may evict an entry as soon
as it is inserted if the maximum weight of the cache is exceeded
causing the DELETE to be executed before the INSERT.

Additionally, any clusters currently experiencing a leaky
system.prepared_statements table from this bug may struggle to
bounce into a version with this fix as
SystemKeyspace.loadPreparedPreparedStatements currently does
not paginate the query to system.prepared_statements, causing heap
OOMs. To fix this this patch adds pagination at 5000 rows and
aborts loading once the cache size is loaded. This should allow
nodes to come up and delete older prepared statements that may no
longer be used as the cache fills up (which should happen immediately).

This patch does not address the issue of Caffeine immediately evicting
a prepared statement, however it will prevent the
system.prepared_statements table from growing unbounded. For most users
this should be adequate, as the cache should only be filled when there
are erroneously many unique prepared statements. In such a case we can
expect that clients will constantly prepare statements regardless
of whether or not the cache is evicting statements.

patch by Andy Tolbert; reviewed by Berenguer Blasi and Caleb Rackliffe for CASSANDRA-19703

[bereng]

IIUC here we're fixing 3 things:

• Ordering of timestamps preventing the leak
• Paged reading when preloading to further soften any leaks
• Stop preloading when cache is already full to prevent trashing the cache upon leaks

[bereng]

Aren't we missing a test we stop loading pstmnts once cache is full? Maybe you could use byteman to capture the call to the log warn method. That with a modified yaml to have a smaller pstmnts cache somehow should do the trick. I think it's one important feature we're not testing unless I am missing sthg.

[bereng]

LGTM modulus the 2 minor issues I mentioned last. I would still heavily compact junits but that's just me. We need another C* committer to +1 this now right?

[tolbertam]

Pushed some changes which hopefully addresses the remaining feedback, hopefully everything looks ok! I merged in recent changes from 4.1 and ran a full CI run. I'll find a second reviewer tomorrow.

[tolbertam]

went ahead and rebased and squashed the commits as I had updated from trunk previously which messed with the history a bit. Made no changes and ensured a clean diff before pushing.

[maedhroz]

nit: I think new entries go at the top

[tolbertam]

Could use QueryProcessor.PREPARED_STATEMENT_CACHE_SIZE_BYTES instead now that we have a constant for this. will fix.

[bereng]

LGTM, we need PRs and CI for the other branches now.

[tolbertam]

thanks 👍 will go ahead and squash into a single commit, and kick off CI against 4.1/5.0/trunk branches.

[tolbertam]

squashed with no changes.

[bereng]

I think we need 4.0 as well

[tolbertam]

I think we need 4.0 as well

Technically the bug is not likely to be encountered on 4.0 as Caffeine is running an older version that behaves differently, but it wouldn't hurt to do so, will create a branch.

[tolbertam]

Added two small changes (c85d156 and 3c80c2e) which I have squashed on the other branches. I have added test results to the jira. Can squash into a single commit assuming everything looks good.

[tolbertam]

squashed with no changes.