[
https://issues.apache.org/jira/browse/CASSANDRA-2475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165844#comment-13165844
]
Rick Shaw commented on CASSANDRA-2475:
--------------------------------------
You are right of course this was a really big bear, and very cumbersome.
I appreciate all the comments and I'll try to address all the problems. Thanks
for the review! Really helpful.
I try and start from the top. I tried several times to break it up. Really. But
there never seemed to be a clean break. I see what you did with generated
thrift... that really helps... I'll do that.
I am using GIT but some of the techniques were a bit foreign to me a tricky to
get all together so I learned as I went along on this patch and I think I have
turned the corner but it definitely slowed me down an I backtracked a lot... I
think I got it now...
I must say I really did not intend any of the whitespace changes (in general)
they just sneak in there when I am not looking I guess. I'll try to keep it
tidier.
As to coding conventions, I'll comply... NBD.
And now on the the meat...
{quote}
I feel pretty strongly that a struct that can optionally do either, is the
wrong choice. Let's not make this implementation, or the client-facing
interface any more complicated than it needs to be.
{quote}
I completely agree and implemented the whole thing first using {{String}}.
But then I hated the idea of having to go to HEX to get in "blob"/bytes data.
This approach let me do both. It also allowed me to serialize Java Objects
nicely as bytes. Can you think of a clever way to handle byte streams
({{AbstractBytes}})? But I can live with just {{String}}. I agree it is the
most flexible.
I really don't see any real performance advantage and the loss of flexibility
on the server side is just too much in my opinion.
{quote}
Does remove_prepared_query support something particular in JDBC (or any other
standard)? How will that be used?
{quote}
It allows you to free the cached {CQLStatement} on the server side when a
client side {{PreparedStatement}} is closed. If not, you will accumulate dead
entries until the connection is closed. That could be a lot of dead wood.
Seemed the tidy thing to do.
{quote}
With regard to CqlPreparedResult:
What is the purpose of the count that is returned? How is that used?
What is the purpose of the CqlStatementType returned. How will that be used?
{quote}
The count is to know how many markers were actually encountered. This number
serves as the upper bound for {{Setxxx}} parameter indexes. Better than
regexing for it... it is exactly what the server side encountered.
The statement type is again a validation of what the server side saw. Remember
this happens in 2 steps {{prepare}} then {{execute}}, and the {{execute}} step
does not have the CQL text.
But I used it while debugging and I don't seem to use it any more so I guess it
could go, but it I thought I might find a use for is so I never did make it go
away.
{quote}
Is CQLStatement.cql only used for logging? If so, should we be keeping a copy
of the query string around for that? Maybe we could create a toString that
would descend to create the query (or something comparable).
{quote}
Another "seems useful" so I kept it around. If something goes wrong and you
need to go poking around its handy to have attached to the statement (I think).
{quote}
There are a few places where queries are being logged at DEBUG. That seems too
verbose for DEBUG.
{quote}
I think there was already an instance there at DEBUG and I just added some
more. I'll gladly move to TRACE.
{quote}
Why is Term.bindIndex marked as volatile?
{quote}
No good reason. I'll fix.
{quote}
Not a biggie, but how about using "bind" or "bound" instead of "mark" when
referring to term position? i.e. needsBind instead of isMarked, or
indexBindTerms instead of discoverMarkedTerms
{quote}
The short answer is because the question marks (?) are often referred to in the
spec as "bound variable markers". So I just propagated it. But NBD to change to
"bind" theme.
{quote}
QueryProcessor.prepare seems as though it could be folded into
CassandraServer.prepare_cql_query
{quote}
I guess it could but I liked the way it read better with it split up for all
the methods.
{quote}
It seems as though QueryProcessor.doTheStatement and QueryProcessor.process
could be merged into one method.{quote}
I factored it out because {{doTheStatement}} is used by both {{process}} and
{{process_prepared}}
----
So in summary, I'll start another pass and would welcome response to my excuses
:)
> Prepared statements
> -------------------
>
> Key: CASSANDRA-2475
> URL: https://issues.apache.org/jira/browse/CASSANDRA-2475
> Project: Cassandra
> Issue Type: New Feature
> Components: API, Core
> Affects Versions: 1.0.5
> Reporter: Eric Evans
> Assignee: Rick Shaw
> Priority: Minor
> Labels: cql
> Fix For: 1.1
>
> Attachments: 2475-v1.patch, 2475-v2.patch,
> v1-0001-CASSANDRA-2475-prepared-statement-patch.txt,
> v1-0002-regenerated-thrift-java.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
