GORA-502 Implement Aerospike Data Store #111

nishadi · 2017-06-20T12:08:48Z

This is a work in progress branch

lewismc

This is very good @nishadi

Please consider my comments and additionally

Make sure that every license header is a comment only and not a documentation block e.g. use following

/*
 *
 *
 */

instead of

/**
  *
  *
  */

Also please begin to document EVERY public method as you make your way through the code. This will distinguish your code as of higher quality.

Thank you, this is an excellent start.

lewismc · 2017-06-20T14:45:59Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeMappingBuilder.java

+    SAXBuilder saxBuilder = new SAXBuilder();
+    InputStream inputStream = getClass().getClassLoader().getResourceAsStream(mappingFile);
+    if (inputStream == null) {
+      LOG.warn("Mapping file '" + mappingFile + "' could not be found!");


Please use parameterized logging.

lewismc · 2017-06-20T14:46:15Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeMappingBuilder.java

+    }
+    Document document = saxBuilder.build(inputStream);
+    if (document == null) {
+      LOG.warn("Mapping file '" + mappingFile + "' could not be found!");


Please use parameterized logging.

lewismc · 2017-06-20T14:47:34Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeParameters.java

@@ -0,0 +1,127 @@
+package org.apache.gora.aerospike.store;


Missing license header

Added the header

lewismc · 2017-06-20T14:48:30Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeStore.java

+import java.util.List;
+import java.util.Properties;
+
+import com.aerospike.client.*;


Remove wildcard imports please. Use explicit imports. Thank you.

lewismc · 2017-06-20T14:49:27Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeStore.java

+  }
+
+  /**
+   * Aerospike does not support Utf8 format returned from Avro.


What is bin? Can you elaborate and make it clearer?

lewismc · 2017-06-20T14:50:13Z

gora-tutorial/conf/gora.properties

@@ -16,7 +16,8 @@

 ##gora.datastore.default is the default detastore implementation to use 
 ##if it is not passed to the DataStoreFactory#createDataStore() method.
-gora.datastore.default=org.apache.gora.hbase.store.HBaseStore
+gora.datastore.default=org.apache.gora.aerospike.store.AerospikeStore


Please retain HBaseStore are default and merely add AerospikeStore to end of list.

lewismc · 2017-06-20T14:51:09Z

pom.xml

+        <groupId>org.apache.gora</groupId>
+        <artifactId>gora-aerospike</artifactId>
+        <version>${project.version}</version>
+      </dependency>


You should also include the test artifact e.g.

+ <dependency> + <groupId>org.apache.gora</groupId> + <artifactId>gora-aerospike</artifactId> + <version>${project.version}</version> + <type>test-jar</type> + </dependency>

Added the test artifact

lewismc · 2017-06-20T14:51:57Z

gora-aerospike/pom.xml

+  </ciManagement>
+
+  <properties>
+    <aerospike.version>3.3.2</aerospike.version>


There's no need to include aerospike.version here if you have already included in within parent pom.xml.

djkevincr · 2017-06-26T15:26:11Z

gora-aerospike/pom.xml

+      <artifactId>junit</artifactId>
+    </dependency>
+
+    <dependency>


Move this dependency to parent pom and define version separately.

djkevincr · 2017-06-26T15:36:08Z

gora-aerospike/src/test/java/org/apache/gora/aerospike/GoraAerospikeTestDriver.java

+ */
+public class GoraAerospikeTestDriver extends GoraTestDriver {
+
+  private GenericContainer aerospikeContainer;


Check whether you can make this test-containers code generic, this can be reused for other datastore which doesn't have java based embedded server.

For the GoraTestDriver class we can add this field( GenericContaine), and in the instances when we need to use the docker based setup environment, we can provide a separate constructor to handle the container. I will look further into generalizing this further to create the docker instance at the DataStoreTestBase level.

final variable?

djkevincr · 2017-06-26T15:41:14Z

@nishadi very good work, heading in right direction :)

nishadi · 2017-06-26T18:11:42Z

@lewismc @djkevincr Thank you for reviewing. Updated the code base as per the given feedback :)

kamaci · 2017-06-27T01:31:40Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeMappingBuilder.java

+        if (policy != null) {
+          if (policy.equals("write")) {
+            WritePolicy writePolicy = new WritePolicy();
+            if (policyElement.getAttributeValue("gen") != null)


Seems that my comment is not placed when I wrote it. I wrote about that:
Could you check this Apple bug: https://www.imperialviolet.org/2014/02/22/applebug.html

Please put curly braces even they are single line of if statements at your PR.

kamaci · 2017-06-27T02:01:20Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeMappingBuilder.java

+
+  private static final Logger LOG = LoggerFactory.getLogger(AerospikeMappingBuilder.class);
+
+  private AerospikeMapping aerospikeMapping;


final variable?

kamaci · 2017-06-27T02:01:57Z

gora-aerospike/src/test/java/org/apache/gora/aerospike/GoraAerospikeTestDriver.java

+public class GoraAerospikeTestDriver extends GoraTestDriver {
+
+  private GenericContainer aerospikeContainer;
+  private Properties properties = DataStoreFactory.createProps();


final variable?

kamaci · 2017-06-27T02:02:57Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/package-info.java

@@ -0,0 +1,20 @@
+/**


Updated the doc comment

kamaci · 2017-06-27T02:03:36Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/package-info.java

@@ -0,0 +1,20 @@
+/**


Updated the doc comment

kamaci · 2017-06-27T02:04:02Z

gora-aerospike/src/test/java/org/apache/gora/aerospike/GoraAerospikeTestDriver.java

+   * @param keyClass The key class.
+   * @param persistentClass The value class.
+   * @return A new store instance.
+   * @throws GoraException


@throws -> throws

Added the exception description

kamaci · 2017-06-27T02:22:35Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeStore.java

+      Schema.Type schemaType = currentSchema.getType();
+      if (instanceValue instanceof CharSequence && schemaType.equals(Schema.Type.STRING))
+        return unionSchemaPos;
+      else if (instanceValue instanceof ByteBuffer && schemaType.equals(Schema.Type.BYTES))


else after a return statement?

kamaci · 2017-06-27T02:27:38Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeMappingBuilder.java

+
+        String policy = policyElement.getAttributeValue("name");
+        if (policy != null) {
+          if (policy.equals("write")) {


Is there an enum for such policies?

There is no ENUM for policy names at this level (write policies or read policies). But when defining values for each policy, there are ENUMs which I have used in below sections while assigning the policy values.

kamaci · 2017-06-27T02:29:13Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeMappingBuilder.java

+        String mappingKeyClass = classElement.getAttributeValue("keyClass");
+        String mappingClassName = classElement.getAttributeValue("name");
+
+        if (mappingKeyClass != null && mappingClassName != null) {


This and below lines of if statements can be merged.

kamaci · 2017-06-27T02:31:55Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeParameters.java

+  // Property names
+  private static final String AS_SERVER_IP = "server.ip";
+
+  private static final String AS_SERVER_port = "server.port";


AS_SERVER_port -> AS_SERVER_PORT?

kamaci · 2017-06-27T02:32:38Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeParameters.java

+   * @param fields fields of the persistent class
+   */
+  public void validateServerBinConfiguration(Field[] fields) {
+    if (isSingleBinEnabled) {


This and below lines of if statements can be merged.

kamaci · 2017-06-27T02:45:06Z

gora-aerospike/src/main/java/org/apache/gora/aerospike/store/AerospikeStore.java

+      case MAP:
+        Map<Object, Object> newMap = new HashMap<>();
+        Map<?, ?> fieldMap = (Map<?, ?>) object;
+        for (Object key : fieldMap.keySet()) {


You can iterate over the entrySet.

kamaci · 2017-06-27T02:52:09Z

gora-aerospike/src/test/java/org/apache/gora/aerospike/GoraAerospikeTestDriver.java

+  public void setUpClass() throws Exception {
+
+    // Wait for the aerospike server to be started in the container
+    Thread.sleep(5000);


Can we check this condition with a framework like Awaitility?

I will check for a method to find out whether the server has been started in the docker container, so that we can use this framework in waiting for the status of the server

nishadi · 2017-06-29T06:19:35Z

@kamaci thank you for reviewing. Added changes as suggested

lewismc · 2017-07-26T16:53:23Z

gora-aerospike/pom.xml

+        <artifactId>maven-surefire-plugin</artifactId>
+        <version>2.4.2</version>
+        <configuration>
+          <skipTests>true</skipTests>


Please don't do this. Instead use the @ Ignore syntax for JUnit. Please see http://junit.sourceforge.net/javadoc/org/junit/Ignore.html

Hi Lewis, This was done basically as I have used Docker to start up the Aerospike server for the test cases. If the build environment does not have Docker environment, the test cases get failed due to the absence of the server. That is the reason why the test cases are made skipped by default as it is done in the couchdb store.
https://lists.apache.org/thread.html/d9816c1e3f3a73fe07a3bc19035c532ffde2163aca5cc43e6d854158@%3Cdev.gora.apache.org%3E

I have added a profile to activate the test cases when we need to run the test cases.

@lewismc Shall I proceed merging the PR to master? Are you done with reviewing/happy with the changes?

I am happy with this +1 from me

djkevincr · 2017-07-27T07:13:26Z

gora-aerospike/pom.xml

+      <groupId>org.testcontainers</groupId>
+      <artifactId>testcontainers</artifactId>
+      <version>1.1.0</version>
+      <scope>test</scope>


Please define the version separately in parent pom.

djkevincr · 2017-07-27T18:11:30Z

Build passes without any issue.

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Apache Gora ....................................... SUCCESS [1.370s]
[INFO] Apache Gora :: Compiler ........................... SUCCESS [2.063s]
[INFO] Apache Gora :: Compiler-CLI ....................... SUCCESS [1.380s]
[INFO] Apache Gora :: Core ............................... SUCCESS [1:17.004s]
[INFO] Apache Gora :: Accumulo ........................... SUCCESS [1:33.638s]
[INFO] Apache Gora :: Cassandra .......................... SUCCESS [53.152s]
[INFO] Apache Gora :: GoraCI ............................. SUCCESS [3.377s]
[INFO] Apache Gora :: HBase .............................. SUCCESS [2:38.696s]
[INFO] Apache Gora :: Infinispan ......................... SUCCESS [1:19.227s]
[INFO] Apache Gora :: JCache ............................. SUCCESS [1:11.505s]
[INFO] Apache Gora :: Dynamodb ........................... SUCCESS [4.452s]
[INFO] Apache Gora :: CouchDB ............................ SUCCESS [3.742s]
[INFO] Apache Gora :: Maven Plugin ....................... SUCCESS [3.379s]
[INFO] Apache Gora :: MongoDB ............................ SUCCESS [1:29.381s]
[INFO] Apache Gora :: Solr ............................... SUCCESS [3:00.093s]
[INFO] Apache Gora :: Aerospike .......................... SUCCESS [3.017s]
[INFO] Apache Gora :: Tutorial ........................... SUCCESS [7.266s]
[INFO] Apache Gora :: Sources-Dist ....................... SUCCESS [0.501s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 13:54.015s
[INFO] Finished at: Thu Jul 27 23:40:39 IST 2017
[INFO] Final Memory: 80M/710M
[INFO] ------------------------------------------------------------------------

djkevincr · 2017-07-27T18:43:17Z

Locally tested Aerospike datastore tests with activating maven profile - mvn clean install -P aerospike-with-test

Results :

Tests run: 33, Failures: 0, Errors: 0, Skipped: 8

djkevincr · 2017-07-27T19:24:19Z

@nishadi This is really awesome work :) I am really happy to merge your work to the master. As we discussed please create tickets for any remaining work. Once again we are really thankful for your awesome contributions.
Thanks all for reviewing the PR. @lewismc @kamaci @renato2099

lewismc · 2017-07-27T19:29:57Z

Can you guys possibly upgrade aerospike-client to most recent?
http://search.maven.org/#artifactdetails|com.aerospike|aerospike-client|4.0.6|jar

djkevincr · 2017-07-27T19:31:51Z

+1

nishadi · 2017-07-27T19:38:06Z

I will update the client to the latest.

https://issues.apache.org/jira/browse/GORA-515

nishadi · 2017-07-27T20:18:32Z

I have sent the PR on the aerospike client update. Thanks a lot @djkevincr for merging to the master. Thanks a lot to the great the support given. :)

lewismc requested changes Jun 20, 2017

View reviewed changes

djkevincr requested changes Jun 26, 2017

View reviewed changes

kamaci reviewed Jun 27, 2017

View reviewed changes

lewismc reviewed Jul 26, 2017

View reviewed changes

djkevincr reviewed Jul 27, 2017

View reviewed changes

nishadi force-pushed the issue-502 branch from df384ee to 38037ca Compare July 27, 2017 17:04

Remove unwanted container stopping

d555d56

nishadi force-pushed the issue-502 branch from 7a56d24 to d555d56 Compare July 27, 2017 18:24

djkevincr approved these changes Jul 27, 2017

View reviewed changes

nishadi changed the title ~~[WIP] GORA-502 Implement Aerospike Data Store~~ GORA-502 Implement Aerospike Data Store Jul 27, 2017

Merge remote-tracking branch 'upstream/master' into issue-502

0e8b046

asfgit merged commit 0e8b046 into apache:master Jul 27, 2017


		private static final Logger LOG = LoggerFactory.getLogger(AerospikeMappingBuilder.class);

		private AerospikeMapping aerospikeMapping;

GORA-502 Implement Aerospike Data Store #111

GORA-502 Implement Aerospike Data Store #111

Conversation

nishadi commented Jun 20, 2017

lewismc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djkevincr commented Jun 26, 2017

nishadi commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishadi commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djkevincr commented Jul 27, 2017

djkevincr commented Jul 27, 2017

djkevincr commented Jul 27, 2017

lewismc commented Jul 27, 2017

djkevincr commented Jul 27, 2017

nishadi commented Jul 27, 2017

nishadi commented Jul 27, 2017