[ 
https://issues.apache.org/jira/browse/CASSANDRA-6311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13920786#comment-13920786
 ] 

Piotr Kołaczkowski commented on CASSANDRA-6311:
-----------------------------------------------

org/apache/cassandra/hadoop/cql3/CqlConfigHelper.java:275
{noformat}
Optional<SSLOptions> ssLOptions = getSSLOptions(conf);
{noformat}
typo: ssL -> ssl
--------------------------------------------------------------
org/apache/cassandra/hadoop/cql3/CqlConfigHelper.java:398:
{noformat}
Optional<Integer> maxSimultaneousRequests = 
getInputMinSimultReqPerConnections(conf);
Optional<Integer> minSimultaneousRequests = 
getInputMaxSimultReqPerConnections(conf);
{noformat}
min and max swapped?       
--------------------------------------------------------------
org/apache/cassandra/hadoop/cql3/CqlConfigHelper.java:549:
{noformat}
Optional<String> keystorePassword = getInputNativeSSLTruststorePassword(conf);
{noformat}
should be:
{noformat}
Optional<String> keystorePassword = getInputNativeSSLKeystorePassword(conf);
{noformat}
--------------------------------------------------------------
org/apache/cassandra/hadoop/cql3/CqlConfigHelper.java:524:
{noformat}
              return new AbstractIterator<Host>()
                        {
                            protected Host computeNext()
                            {
                                return origHost;
                            }  
                        };
{noformat}
Not sure if it was the intent to create an infinite iterator returning nulls or 
the same host over and over again here. According to the docs, guava iterator 
implementations *must* invoke endOfData() to terminate iteration. Don't we need 
here an iterator returning just one item stickHost and let the driver handle 
the rest?

Also, not sure if returning nulls here is allowed at all (the driver docs isn't 
explicit on that).
I guess very likely it is going to NPE if there is a connection problem which 
might cause confusion. Probably a better solution would be to just return 
stickHost and let the driver attempt connecting and throwing a meaningful error 
message upon failure.

BTW the implementation of the LoadBalancingPolicy, having two fields origHost 
and stickHost is redundant and using null on one of those for marking the host 
is down / unreachable does not convey the intent clearly to me. Can't we just 
use stickHost and a direct boolean flag for denoting whether it is reachable or 
not?

--------------------------------------------------------------
org/apache/cassandra/hadoop/cql3/CqlConfigHelper.java:591:
{noformat}
    private static Optional<String> getStringSetting(String parameter, 
Configuration conf)
    {
        String setting = conf.get(parameter);
        if (setting == null || setting.isEmpty())
            return Optional.absent();
        return Optional.of(setting);  
    }
{noformat}
In getStringSetting, setting an empty string is considered an absent option - 
so it is not possible to have an empty string setting (not sure if it would be 
useful - just double checking if it was on purpose or by omission)
--------------------------------------------------------------
{noformat}
     *  2) where clause must include token(partition_key1 ... partition_keyn) > 
? and 
     *     token(partition_key1 ... partition_keyn) <= ?
{noformat}

Would be nice to have at least some basic validation of the WHERE clause, so 
the user gets a nice error message when one screws it up. 
--------------------------------------------------------------
org/apache/cassandra/hadoop/cql3/CqlRecordReader.java:230
{noformat}
       public RowIterator(Configuration conf)
{noformat}
conf not used
--------------------------------------------------------------
org/apache/cassandra/hadoop/cql3/CqlRecordReader.java:268
{noformat}
            return Pair.create(Long.valueOf(keyId), row);
{noformat}
Boxing is not needed here.




> Add CqlRecordReader to take advantage of native CQL pagination
> --------------------------------------------------------------
>
>                 Key: CASSANDRA-6311
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-6311
>             Project: Cassandra
>          Issue Type: New Feature
>          Components: Hadoop
>            Reporter: Alex Liu
>            Assignee: Alex Liu
>             Fix For: 2.0.6
>
>         Attachments: 6311-v3-2.0-branch.txt, 6311-v4.txt, 
> 6311-v5-2.0-branch.txt, 6331-2.0-branch.txt, 6331-v2-2.0-branch.txt
>
>
> Since the latest Cql pagination is done and it should be more efficient, so 
> we need update CqlPagingRecordReader to use it instead of the custom thrift 
> paging.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to