Apache Sling GSOC2013 Initial Code Review

Hi,
I've created this code review of the code so far, more info to follow on dev@sling.apache.org

[Ian Boston, 2013-06-30 23:50:42]

Hi Dishara,
Below is a code review done on codereview.appspot.com. Its off all the changes
in the repo. In future I would like to pull in smaller ranges of commits as
necessary. The first code review is always a bit frustrating for the developer
as it often contains loads of comments. Don't see this as a sign of problems,
and remember the comments are intended to guide you towards code that is closer
to much of the code in Sling. That doesn't mean that the code in Sling is right,
and your code is wrong, it just means that the code in Sling is the way the code
in Sling is done.

I've kept this message private. I'll send an update on sling dev. I would like
to do later code reviews more on list. 

Feel free to question my comments, ask for clarification and start a
conversation.

Also, feel free to create your own reviews on this appspot instance and ask for
review. Its often easier to discuss code, than it is to discuss abstract english
concepts.

Best Regards
Ian

https://codereview.appspot.com/10811044/diff/1/main/cassandra/pom.xml
File main/cassandra/pom.xml (right):

https://codereview.appspot.com/10811044/diff/1/main/cassandra/pom.xml#newcode31
main/cassandra/pom.xml:31: <version>1.1.3</version>
Should uses 0.0.1-SNAPSHOT as this hasnt been released. See 
http://sling.apache.org/documentation/development/version-policy.html for more
information on versioning.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/pom.xml#newcode64
main/cassandra/pom.xml:64: <!--</scm>-->
When you come to do a release, you will need to add the svn location. Using the
current Google Code SVN repo is ok.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/pom.xml#newcode102
main/cassandra/pom.xml:102: </instructions>
Try removing these line. I suspect you wont initially want to export any classes
and will want to just have a default import *

https://codereview.appspot.com/10811044/diff/1/main/cassandra/pom.xml#newcode148
main/cassandra/pom.xml:148: <!--</dependency>-->
Do use src annotations like @Service and @Component when building components in
OSGi. It eliminates much of the code required to make OSGi work, and will make
your life a lot easier.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
File
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraConstants.java
(right):

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraConstants.java:25:
public static final String CASSANDRA_RESOURCE_TYPE="nt:cassandra";
JCR node types start often start with nt: and I dont think thats what you
intended. If you need a cassandra resource type, then use sling:cassandra
This will make it clear that we are not talking about a JCR node type, but we
are talking about a resource type. 

I think the only reason you should need a resource type for cassandra is if you
want to register the parent node of a cassandra subtree in the JCR repository.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
File
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java
(right):

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java:27:
import org.apache.sling.api.resource.*;
Avoid * imports at all times.
It makes it imposible to work out what its really being imported except with an
IDE

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java:30:
public class CassandraResource extends AbstractResource implements Resource {
I think AbstractResource implements Resource already, no need for the implements

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
File
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java
(right):

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java:41:
public class CassandraResourceProvider implements ResourceProvider {
Dont forget to use SCR annotations here.
@Component(immediate=true, metatype=true)
@Service(ResourceProvider.class)

Also check the ResourceProvider API to see what other properties are required to
register it correctly.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java:48:
private String CASSANDRA_EP="localhost:9160";
All of the above can become OSGi properties. Look through the Sling code base
for the annotation @Property applied to a static string.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java:50:
= new ConcurrentHashMap<String, DefaultCassandraMapperImpl>();
DefaultCassandraMapperImpl should be replaced by the CassandraMapper API.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java:66:
cluster =
HFactory.getOrCreateCluster(CassandraConstants.CASSANDRA_DEFAULT_CLUSTER_NAME,
CASSANDRA_EP);
Check that cluster is threadsafe, it will be used by multiple threads.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java:87:
return CassandraResourceProviderUtil.isResourceExists(this,keyspace,s) ? new
CassandraResource(this,resourceResolver, s) : null;
Using Utils for service implementations feels like a bit of an anti-pattern, you
might want to consider bringing whatis really service implementation into the
service and leaving things that are really Utils in the Util class. Have a look
at commons-lang StringUtils to see what I mean about what really is a utility.
StringUtils has no side effects and accesses nothing other than what is passed
in.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java:88:
} catch (Exception e) {
Catching exceptions is ok to get code running, but dont forget to capture more
specific exceptions later.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java:98:
private void createSchema(Cluster myCluster) {
This needs to be thread safe, as OSGi components can be initialised by multiple
theads. quick solution is to synchronize the method.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
File
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceResolver.java
(right):

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceResolver.java:31:
public class CassandraResourceResolver implements ResourceResolver {
You wont need this implementation. ResourceResolver is implemented for you, all
you need to implement is the resource provider.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
File
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapper.java
(right):

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapper.java:29:
String getCQL(String columnFamilySelector, String path) throws Exception;
Avoid throwing Exceptions. you could use throws CassandraMapperException

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
File
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java
(right):

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java:36:
public static boolean isResourceExists(CassandraResourceProvider
resourceProvider,Keyspace keyspace,String path) throws Exception {
Put this in the CassandraResourceProvider. (imho :))

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java:43:
public static QueryResult<CqlRows<String,String,String>> executeQuery(String
query, Keyspace keyspace, StringSerializer se) {
Probably ok here.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java:52:
if (path.startsWith("/") && path.split("/").length > 4) {
This is Ok here.
There might be a Util class in Sling already to do this for you.

https://codereview.appspot.com/10811044/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java:66:
public static String getColumnFamilySector(String path) {
Again, this is Ok here.
commons-lang StringUtil will help you here.
String[] parts = StringUtil.split(path,"/"); might be better as path.split
creates a regular expressions each time and can be slow.
Also

StringUtil.join(parts,"/") can be used to reconstruct the path.

Whatever you do, cover this sort of class with unit tests and every pattern you
can possibly think of including nasty paths like /////