Hi Dishara, You asked me offlist if I could to a code review. I think its better if I do it on list so others in the community can tell me if I am being too harsh or lenient. One of the benefits of being in a community; support for both of us ;)
Here is the review: Best Regards Ian https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraConstants.java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraConstants.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraConstants.java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraConstants.java#**newcode23<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraConstants.java#newcode23> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraConstants.java:23: public class CassandraConstants { Needs indentation and Javadoc please. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraIterable.java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraIterable.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraIterable.java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraIterable.java#**newcode1<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraIterable.java#newcode1> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraIterable.java:1: package org.apache.sling.cassandra.**resource.provider; ASF License header needed here please. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraIterable.java#**newcode7<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraIterable.java#newcode7> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraIterable.java:7: public class CassandraIterable implements Iterable { Some javadoc, even if its just going to say wraps an iterator. I wonder if something like IterableIterator wouldn't be a better name as there is nothing specific to cassandra about this class. ? https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResource.java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResource.java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResource.java#**newcode68<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java#newcode68> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResource.java:68: } catch (Exception e) { In general, try not to catch (Exception) as although its quick and easy it will also tend to hide what its going on. Also its helpful where you do catch anything and not rethrow it, to log the stack trace at at least debug level. eg log.debug(e.getMessage(),e); https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResource.java#**newcode81<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java#newcode81> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResource.java:81: }else if(column.getName().equals("**resourceType")) { Looks like there are some formatting issues here. I'll share with you my formatting setup that should put all of these right. It wont correct trailing spaces in the code, but you can do that with a regex find replace " $" as the regex "" as the replacement. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResourceProvider.java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResourceProvider.java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResourceProvider.**java#newcode80<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java#newcode80> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResourceProvider.**java:80: } catch (Exception ignore){ even if you ignore an exception, log it at debug level. It might be the one thing that helps a deployer work out what went wrong. If the exception is expected, then say so in the log message. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResourceProvider.**java#newcode98<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java#newcode98> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResourceProvider.**java:98: log.error("Error at Provider "+e.getMessage()); log the stack trace at info or debug, however if this is a rare error then think about logging a stack trace at error. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResourceProvider.**java#newcode104<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java#newcode104> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResourceProvider.**java:104: return resource.listChildren(); This might generate a cyclic recursion depending on how resource.listChildren is implemented. I think with many resources it goes back to the resource resolver which arrives here. Please verify if I am correct or not. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResourceResolver.java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceResolver.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResourceResolver.java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/**resource/provider/** CassandraResourceResolver.**java#newcode31<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceResolver.java#newcode31> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResourceResolver.**java:31: public class CassandraResourceResolver implements ResourceResolver { This class can be removed, you dont need to implement a ResourceResolver. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/mapper/**CassandraMapper.java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapper.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**CassandraMapper.java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/mapper/**CassandraMapper.java#newcode22<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapper.java#newcode22> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**CassandraMapper.java:22: public interface CassandraMapper { Javadoc on the interface would be good. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/mapper/**CassandraMapperException.java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapperException.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**CassandraMapperException.java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/mapper/**CassandraMapperException.java#**newcode5<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapperException.java#newcode5> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**CassandraMapperException.java:**5: public CassandraMapperException(**String s) { Think about implementing the other constructors so you can set the cause. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/mapper/**DefaultCassandraMapperImpl.**java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/DefaultCassandraMapperImpl.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**DefaultCassandraMapperImpl.**java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/mapper/**DefaultCassandraMapperImpl.**java#newcode29<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/DefaultCassandraMapperImpl.java#newcode29> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**DefaultCassandraMapperImpl.**java:29: public class DefaultCassandraMapperImpl implements CassandraMapper { Needs javadoc to say what the class does. In particular you might mention what the SHA1 is about. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/util/**CassandraResourceProviderUtil.**java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java> File main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/util/**CassandraResourceProviderUtil.**java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/util/**CassandraResourceProviderUtil.**java#newcode43<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java#newcode43> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/util/**CassandraResourceProviderUtil.**java:43: public class CassandraResourceProviderUtil { Be wary of putting code that is really service code in a utility class. A good test if if code interacts with or modifies an external resource it should not be in a utility class. Utility static methods should have no side effects. COmmunicating with cassandra causes cassandra to log things, so is a side effect. This is not a hard and fast rule, since commons-io is littered with really useful static utility methods that read from streams etc, so just think, is the code service or utility. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/util/**CassandraResourceProviderUtil.**java#newcode45<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java#newcode45> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/util/**CassandraResourceProviderUtil.**java:45: private static Log log = LogFactory.getLog(**CassandraResourceProviderUtil.**class); Dont use commons logging use slf4j Logger e.g.: Logger log = LogFactory.getLogger(**CassandraResourceProviderUtil.**class); Also, I prefer LOGGER since its a static constant, but thats just being pedantic. https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/main/java/org/**apache/sling/cassandra/** resource/provider/util/**CassandraResourceProviderUtil.**java#newcode90<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java#newcode90> main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/util/**CassandraResourceProviderUtil.**java:90: System.out.println(column.**getValue().toString()); Avoid System.out and System.err please https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/test/java/org/**apache/sling/cassandra/test/**data/populator/* *CassandraDataAddTest.java<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/test/java/org/apache/sling/cassandra/test/data/populator/CassandraDataAddTest.java> File main/cassandra/src/test/java/**org/apache/sling/cassandra/**test /data/populator/**CassandraDataAddTest.java (right): https://codereview.appspot.**com/11960043/diff/1/main/** cassandra/src/test/java/org/**apache/sling/cassandra/test/**data/populator/* *CassandraDataAddTest.java#**newcode25<https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/test/java/org/apache/sling/cassandra/test/data/populator/CassandraDataAddTest.java#newcode25> main/cassandra/src/test/java/**org/apache/sling/cassandra/**test /data/populator/**CassandraDataAddTest.java:25: import junit.framework.TestCase; you should be using JUnit 4 ie org.junit and not junit.framework which IIRC is 3. Then you dont have to extend classes and the correct test runner is used. Description: Sling GSoC2013 Code Review Please review this at https://codereview.appspot.**com/11960043/<https://codereview.appspot.com/11960043/> Affected files: A main/cassandra/pom.xml A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraConstants.java A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraIterable.java A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResource.java A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResourceProvider.java A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/**CassandraResourceResolver.java A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**CassandraMapper.java A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**CassandraMapperException.java A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/mapper/**DefaultCassandraMapperImpl.**java A main/cassandra/src/main/java/**org/apache/sling/cassandra/**reso urce/provider/util/**CassandraResourceProviderUtil.**java A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test /data/populator/**CassandraDataAddTest.java A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test /data/populator/**CassandraDataChildNodeIterable**Test.java A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test /data/populator/**CassandraDataChildNodeTest.**java A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test /data/populator/**CassandraDataParentNodeTest.**java A main/cassandra/src/test/java/**org/apache/sling/cassandra/**test /data/populator/**CassandraDataReadTest.java A scratch/test/hector-examples-**master/LICENSE A scratch/test/hector-examples-**master/README.mdown A scratch/test/hector-examples-**master/pom.xml A scratch/test/hector-examples-**master/src/main/java/org/test/**m e/BasicTest.java A scratch/test/hector-examples-**master/src/main/java/org/test/**m e/CqlQueryTest.java
