[
https://issues.apache.org/jira/browse/CASSANDRA-2475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165645#comment-13165645
]
Eric Evans commented on CASSANDRA-2475:
---------------------------------------
Alright. This one is sort of a monster, so bare with me if any of this seems
disorganized, or jumps around. Also, if it seems long(ish), don't be
disheartened, again, it's a bit of a monster; All-in-all it looks pretty good.
First, on the subject of patch submission, coding conventions, etc:
If at all possible, try to break a large change like this into more than one
changeset, with logically separate changes in separate patches. A work-flow
based on patch submission sucks, I know, but Git can make this fairly easy (you
mentioned you were using Git). It definitely makes review easier and is much
appreciated.
That said, don't worry about breaking this one into smaller patches (something
to keep in mind for next time).
Also, avoid unnecessary changes to whitespace, or unrelated/cosmetic changes.
They add noise to the review and increase the likelihood of collisions when
merging or rebasing. A bit here and there is fine, but if you do any sort of
substantive cleanup, roll that up into a separate patch.
Some specific code-style/convention nits:
* Consensus seems to be against {{SuppressWarnings}} annotations, or the use of
{{Override}} for interfaces
* Put a space between method arguments
* When wrapping a method call, align the arguments with the open paren
OK, on to the bigger fish.
There was some earlier discussion in this ticket about using preserialized
binary parameters, or strings that would be parsed node-side. Which of those
two views is "right" notwithstanding, 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.
My opinion on string vs. binary is largely unchanged here. String parameters
is the path of greatest abstraction, eliminating a proven vector for bugs, and
it keeps our query interface as friendly as possible.
That said, if the performance advantage to preserialized values were known, and
turned out to be significant enough, I'd happily reconsider (I like fast as
much as the next guy). My suggestion then would be this: Let's refactor this
patch to type the variables as {{String}} and get it in-tree. From there, it's
a simple matter of a patch to change from {{String}} to {{ByteBuffer}} (and of
course to drop the {{AT.fromString}}), and we can run some benchmarks. I will
commit to working up this latter patch, and to the benchmarking, within the
time remaining to 1.1, if that helps.
Other questions and concerns in no particular order:
* Does {{remove_prepared_query}} support something particular in JDBC (or any
other standard)? How will that be used?
* 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?
* 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).
* There are a few places where queries are being logged at DEBUG. That seems
too verbose for DEBUG.
* Why is {{Term.bindIndex}} marked as volatile?
* In {{CassandraServer.prepare_cql_query}}, don't create a separate variable
for state
* 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}}
* {{QueryProcessor.prepare}} seems as though it could be folded into
{{CassandraServer.prepare_cql_query}}
* It seems as though {{QueryProcessor.doTheStatement}} and
{{QueryProcessor.process}} could be merged into one method.
> 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
>
>
--
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
