Prevent re-preparing already prepared statement in CassandraLayerReader #2696
Conversation
| @@ -43,15 +43,17 @@ class CassandraValueReader( | |||
| val writerSchema = attributeStore.readSchema(layerId) | |||
| val codec = KeyValueRecordCodec[K, V] | |||
|
|
|||
| def read(key: K): V = instance.withSession { session => | |||
| val statement = session.prepare( | |||
| private lazy val statement = instance.withSession{ session => | |||
There was a problem hiding this comment.
Do you think it should be private?
There was a problem hiding this comment.
I debated... I saw that none of the other instance fields were, but if someone accesses this statement from the outside and uses a different Session than the one originally used to prepare it then that would cause an exception. I ended up deciding to keep the field hidden to prevent potential misuse.
If the project standard is to mark instance fields public though I don't have a problem changing it... just wasn't sure what the standard was.
|
I'm curious, does this PR solve your issues? |
|
@pomadchin - trying to suppress warnings mostly, but I've seen some evidence from Cassandra maintainers (e.g. https://datastax-oss.atlassian.net/browse/JAVA-236?focusedCommentId=14736&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14736 ) that there is good reason they issue this warning. I also know there's a PreparedStatement cache setting in # Maximum size of the native protocol prepared statement cache
#
# Valid values are either "auto" (omitting the value) or a value greater 0.
#
# Note that specifying a too large value will result in long running GCs and possbily
# out-of-memory errors. Keep the value at a small fraction of the heap.
#
# If you constantly see "prepared statements discarded in the last minute because
# cache limit reached" messages, the first step is to investigate the root cause
# of these messages and check whether prepared statements are used correctly -
# i.e. use bind markers for variable parts.
#
# Do only change the default value, if you really have more prepared statements than
# fit in the cache. In most cases it is not neccessary to change this value.
# Constantly re-preparing statements is a performance penalty.
#
# Default value ("auto") is 1/256th of the heap or 10MB, whichever is greater
prepared_statements_cache_size_mb:which also seems to indicate that re-preparing statements is not a recommended thing to do. Thanks! -Andy |
|
Ha, thanks for references. |
|
Sry that touching you again @ALPSMAC, but have you signed ECA? https://github.com/locationtech/geotrellis/blob/master/docs/CONTRIBUTING.rst#eclipse-contributor-agreement-eca |
|
Yes - but I think I may have inadvertently signed the commit with the wrong email or something. Sorry - first time trying to contribute by signing a commit. The email I've got registered on the machine I'm working from in my .gitconfig is my work email. My personal email is polack.andy@gmail.com if you're looking to match it against the ECA. Please let me know what to do to fix it... sorry about that. |
|
@pomadchin I'm not exactly sure how to fix this PR so that it's signed by my email address that matches the one I used in the ECA. Can you suggest what I should do? |
|
@ALPSMAC I think that |
Signed-off-by: Andrew Polack <polack.andy@gmail.com>
ALPSMAC
force-pushed
the
bugfix/2695-RepreparedStatement
branch
from
6cd5329
to
e7ad394
Compare
|
@pomadchin sorry for the delay in getting back to you - other work responsibilities caught up with me. Okay - I think I have amended the commit properly... hopefully that worked :-) Let me know if it still doesn't look appropriately signed to you. Thanks! |
|
Thank you, it works! |
Signed-off-by: Andrew Polack polack.andy@gmail.com // andrew.polack@navy.mil
Overview
Fixes issue #2695
In short, prevents the re-preparing of an already prepared statement which is a Cassandra anti-pattern.
Checklist
docs/CHANGELOG.rstupdated, if necessarydocsguides update, if necessaryI could use some help figuring out how to test this... generally the symptom is a warning that appears in the log emitted from the Cassandra Datastax Java Driver if a query is re-prepared.
I ran the
cassandraSBT project test suite and all tests passed so... ¯_(ツ)_/¯Notes
If the Session object is somehow pooled and a different Session is provided than the one originally used to prepare the statement then this fix could cause issues.
If this turns out to be a valid fix it would be great to have it back-ported into the released 1.2.x stuff.
Thanks Much!
-Andy
Closes #2695