https://codereview.appspot.com/11960043/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/11960043/diff/1/main/cassandra/src/main/java/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraConstants.java:23: public class CassandraConstants { Needs indentation and Javadoc please. https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/o... File main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraIterable.java (right): https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... File main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResource.java (right): https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... File main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceProvider.java (right): https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... File main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/CassandraResourceResolver.java (right): https://codereview.appspot.com/11960043/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 { This class can be removed, you dont need to implement a ResourceResolver. https://codereview.appspot.com/11960043/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/11960043/diff/1/main/cassandra/src/main/java/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... File main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/CassandraMapperException.java (right): https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... File main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/mapper/DefaultCassandraMapperImpl.java (right): https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... File main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/util/CassandraResourceProviderUtil.java (right): https://codereview.appspot.com/11960043/diff/1/main/cassandra/src/main/java/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... main/cassandra/src/main/java/org/apache/sling/cassandra/resource/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/o... 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/o... 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.
