Feature/concurrency tests #6

frankwis · 2016-11-17T21:28:27Z

Added custom types and conversion for UUID, Date, BigInteger and BigDecimal
Fixed initialization of custom type converters
Added types for custom values and operations, fixed conversion
CustomTypeSerializer and corresponding Builder
Added headers to Patch operation
Concurrency tests for serialization

…ecimal.

dhiller · 2016-12-14T09:29:55Z

src/main/java/io/datanerds/avropatch/Patch.java

    private final List<Operation> operations;

    public Patch() {
        operations = new ArrayList<>();
+        headers = new HashMap<>();


Perhaps we should align on whether to use this.....

btw why don't we use

this(new ArrayList<>()):

?

dhiller · 2016-12-14T09:54:57Z

src/main/java/io/datanerds/avropatch/operation/DefaultSchema.java

+ * This interface holds the schema for all valid value types.
+ */
+public interface DefaultSchema {
+    String VALUE =


Eventually move to file and load with getClass().getResource(...)

Would probably make it more readable?

This would not compile since the value is used in an annotation (only constants allowed there). Would need an indirection for compilation. I am adding a card to the backlog

This would not compile per se since the value is used in an annotation (only constants allowed there). Would need an indirection for compilation. I am adding a card to the backlog

In that case generating that file from a json is probably the easiest?

dhiller · 2016-12-14T10:07:15Z

src/main/java/io/datanerds/avropatch/serialization/CustomTypeSerializer.java

+    }
+
+    public static class Builder {
+        final List<Schema> types = new ArrayList<>();


Should be private

dhiller · 2016-12-14T10:15:01Z

src/main/java/io/datanerds/avropatch/value/conversion/BigDecimalConversion.java

+import java.math.BigInteger;
+
+/**
+ * This class converts an {@link BigDecimal} value into am Avro {@link IndexedRecord} and back. It depends on


s/am/an/g

dhiller · 2016-12-14T10:23:35Z

src/test/java/io/datanerds/avropatch/PatchTest.java

+    public void serializesDefaultValueTypes() throws IOException {
+        Date date = new Date();
+        UUID uuid = UUID.randomUUID();
+        Patch patch = new Patch(ImmutableList.of(new Add<>(Path.of("some", "value"), "John Doe"),


Perhaps insert line break before new Add...

dhiller · 2016-12-14T11:53:21Z

src/test/java/io/datanerds/avropatch/serialization/Bimmel.java

+
+    @Override
+    public int hashCode() {
+        return Objects.hash(name, number, id, bommel);


dhiller · 2016-12-14T11:55:33Z

src/test/java/io/datanerds/avropatch/serialization/ConcurrencyTest.java

+        futures.stream().forEach(future -> {
+            try {
+                verify(PATCHES, future.get());
+            } catch (InterruptedException ex) {


Use Java 8 multicatch?

dhiller · 2016-12-14T11:57:43Z

src/test/java/io/datanerds/avropatch/serialization/ConcurrencyTest.java

+
+        public Operation generateOperation(Type type) {
+            switch (type) {
+                case ADD:


Possibly refactor to enum strategy pattern?

dhiller · 2016-12-14T11:58:24Z

src/test/java/io/datanerds/avropatch/serialization/ConcurrencyTest.java

+        }
+
+        public Object generate(Type type) {
+            switch (type) {


Refactor to enum strategy pattern.

dhiller · 2016-12-14T12:00:15Z

src/test/java/io/datanerds/avropatch/value/conversion/BigDecimalConversionTest.java

+                .withSchemata(Schema.createArray(BigDecimalType.SCHEMA))
+                .withConverters(new BigIntegerConversion(), new BigDecimalConversion())
+                .reserializeAndAssert(ImmutableList.of(BigDecimal.valueOf(1234567890L),
+                        new BigDecimal("12345678901234567.8901234567890123456789012345678901234567890"),


Extract constants.

thgreiner · 2016-12-14T14:09:38Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+
+    interface PatchType {
+        String NAME = Patch.class.getSimpleName();
+        String DOC = "This record represents a JSON PATCH operation holding a sequence of operations to apply to a"


JSON PATCH - does this description fit?

…attern.

thgreiner · 2016-12-15T13:03:02Z

src/main/java/io/datanerds/avropatch/Patch.java

    }

    public Patch(List<Operation> operations) {
        this.operations = new ArrayList<>(operations);
+        this.headers = new HashMap<>();
+    }


Why not call the two-argument constructor here? This would follow the same principle as the no-argument constructor...

Correct :-)

dhiller · 2016-12-15T12:43:26Z

src/main/java/io/datanerds/avropatch/value/conversion/AvroData.java

+            new DateConversion(),
+            new BigIntegerConversion(),
+            new BigDecimalConversion(),
+            new UUIDConversion());


I'd say according to K&R style we would need to insert a line break before the last closing brace...

dhiller · 2016-12-15T12:45:31Z

src/test/java/io/datanerds/avropatch/operation/matcher/OperationMatchers.java

-
-        return allOf(all);
+        return allOf(items.stream()
+                .map(item -> hasItem(item))


.map(this::hasItem)

dhiller · 2016-12-15T12:45:53Z

src/test/java/io/datanerds/avropatch/operation/matcher/OperationMatchers.java

-        }
-
-        return allOf(all);
+        return allOf(items.stream()


Insert line break before items.stream()

dhiller · 2016-12-15T12:47:54Z

src/test/java/io/datanerds/avropatch/operation/matcher/PatchMatcherTest.java

+                ImmutableMap.of("header", UUID.randomUUID()));
+
+        @Test
+        public void equality() throws IOException {


Split the two asserts into two tests. Each assertion should be in a separate test to avoid failing assertions being hidden by assertions already failing before.

Best rule would be one assertion per test method.

Perhaps you could elaborate a bit on the test methods by naming them so they explain what is tested exactly.

Since we're considering to get rid of these classes (see #7), I'll ignore it for now.

dhiller · 2016-12-15T12:52:59Z

src/test/java/io/datanerds/avropatch/operation/matcher/PatchMatcherTest.java

+
+        @Test
+        public void equality() throws IOException {
+            assertThat(copyMovePatch, is(equalTo(copyMovePatchSibling)));


BTW: this equalTo() is an own matcher that would have been unnecessary if equals had been overridden in the first place. Then you would have been able to just use is as that is equivalent to is(equalTo(...) using the default hamcrest matcher

I am going to add a card to the backlog in order to remove the boilerplate test code

I created issue #7 for future discussions and reference on this topic

dhiller · 2016-12-15T12:59:18Z

src/test/java/io/datanerds/avropatch/serialization/OperationGenerator.java

+    MOVE(() -> new Move(generatePath(), generatePath())),
+    REMOVE(() -> new Remove(generatePath())),
+    REPLACE(() -> new Replace<>(generatePath(), generateValue())),
+    @SuppressWarnings("unchecked")


Add comment why this was necessary?

I dug a little bit deeper and figured out it's not necessary 👍

dhiller · 2016-12-15T13:00:53Z

src/test/java/io/datanerds/avropatch/serialization/OperationGenerator.java

+        this.operationSupplier = operationSupplier;
+    }
+
+    public static Operation generate() {


Rename method to reflect its random nature.

dhiller · 2016-12-15T13:01:23Z

src/test/java/io/datanerds/avropatch/serialization/OperationGenerator.java

+    }
+
+    public static Operation generate() {
+        OperationGenerator operationType = values()[random.nextInt(values().length)];


Inline variable as it's only used once.

IMHO a variable should only be declared if it's used more than once.

Agreed. It just became a one liner during the last refactorization :-)

dhiller · 2016-12-15T13:06:32Z

src/test/java/io/datanerds/avropatch/serialization/OperationGenerator.java

+        }
+
+        private static List<?> generateList(ValueType type) {
+            List<Object> data = new ArrayList<>();


Maybe replace with

return IntStream .range(0, MAX_LIST_SIZE) .mapToObject(i -> type.generate()) .collect(toList());

dhiller · 2016-12-15T13:07:10Z

src/test/java/io/datanerds/avropatch/serialization/OperationGenerator.java

+        }
+
+        private static BigDecimal randomBigDecimal() {
+            BigInteger unscaledValue= new BigInteger(random.nextInt(256), random);


Space missing before =

obiwen · 2016-12-15T13:27:05Z

src/main/java/io/datanerds/avropatch/serialization/CustomTypeSerializer.java

+        }
+
+        public Builder withCustomTypes() {
+            types.addAll(Arrays.asList(BigDecimalType.SCHEMA, BigIntegerType.SCHEMA, DateType.SCHEMA, UuidType.SCHEMA));


Hmm just wondering .. we have been using a special formatter with the ISO8601 format for dates. Would we need such a format as well for the DateTypes here?

No. Date is just converted to a long value and is defined accurate as a timestamp representing the number of milliseconds since January 1, 1970, 00:00:00 GMT. (See io.datanerds.avropatch.value.conversion.DateConversion)

obiwen · 2016-12-15T13:34:32Z

src/main/java/io/datanerds/avropatch/value/conversion/DateConversion.java

+import org.apache.avro.generic.GenericFixed;
+
+import java.nio.ByteBuffer;
+import java.util.Date;


Hm .. just a general question .. could we built a DateConversion for the yodatime DateTime?

Sure, but do we need it? ;-)

dhiller · 2016-12-15T15:21:13Z

src/test/java/io/datanerds/avropatch/serialization/OperationGenerator.java

-    public static Operation generate() {
-        OperationGenerator operationType = values()[random.nextInt(values().length)];
-        return operationType.generateOperation();
+    public static Operation generateOperation() {


dguenms

Please fix the typos.

Otherwise only formatting / minor refactoring suggestions.

dguenms · 2016-12-16T14:48:00Z

src/main/java/io/datanerds/avropatch/Patch.java

+        this(new ArrayList<>(operations), new HashMap<>());
+    }
+
+    public Patch(List<Operation> operations, Map<String, ?> headers) {
        this.operations = new ArrayList<>(operations);


Following the pattern above, this(operations);

dguenms · 2016-12-16T15:04:22Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+
+    interface BigDecimalType {
+        String NAME = "big-decimal";
+        String DOC = "BigDecimal value represented via it's scale and unscaled value.";


"its" (typo)

dguenms · 2016-12-16T15:05:18Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+
+    interface UuidType {
+        String NAME = LogicalTypes.uuid().getName();
+        String DOC = "UUID serialized via two long values: It's most significant and least significant 64 bits.";


"Its" (see above)

dguenms · 2016-12-16T15:07:42Z

src/main/java/io/datanerds/avropatch/schema/OperationTypes.java

+        static Schema create(Schema valueSchema) {
+            Field path = new Field("path", Path.SCHEMA, "Path pointing out where the JSON value should be replaced",
+                    NO_DEFAULT);
+            Field value = new Field("value", valueSchema, "Actual value to replaced in patched object", NO_DEFAULT);


"to be replaced"

dguenms · 2016-12-16T15:08:06Z

src/main/java/io/datanerds/avropatch/schema/OperationTypes.java

+        static Schema create(Schema valueSchema) {
+            Field path = new Field("path", Path.SCHEMA, "Path pointing out which JSON value should be tested against",
+                    NO_DEFAULT);
+            Field value = new Field("value", valueSchema, "Actual value to test< against", NO_DEFAULT);


dguenms · 2016-12-16T16:27:41Z

src/test/java/io/datanerds/avropatch/serialization/ConcurrencyTest.java

+
+        List<Future<List<Patch>>> futures = new ArrayList<>();
+        for (int i = 0; i < NUMBER_OF_THREADS; i++) {
+            futures.add(executorService.submit(() -> {


It might improve readability to give this function a name so we can only do something like executorService.submit(this::serialize) or whatever name is appropriate.

abhishekjain88

Have only reviewed a few files. Plan to continue reviewing. Please take your time to address existing comments till then! :)

abhishekjain88 · 2016-12-17T00:43:48Z

src/main/java/io/datanerds/avropatch/Patch.java

    }

    public Patch(List<Operation> operations) {
+        this(new ArrayList<>(operations), new HashMap<>());


This would potentially result in copying the list twice. Perhaps, makes more sense to just pass "operations" as is to the other constructor where this list would be copied again into a new list.

abhishekjain88 · 2016-12-17T02:18:55Z

src/main/java/io/datanerds/avropatch/Patch.java

        this.operations = new ArrayList<>(operations);
+        this.headers = new HashMap<>(headers);
    }

    public boolean addOperation(Operation operation) {


Thread safety will become an issue here. You are using a thread unsafe list for "operations". If there are 2 competing threads one of which does a getOperations and tries to iterate on it where as the other tries to add an operation at the same time, it will result in a ConcurrentModificationException. Would be better to use either a CopyOnWriteArrayList if operations are not expected to be added too frequently or synchronize getOperations and create a copy of the list before returning. AFAIK Collections.unmodifiableList is only a shallow copy and not a deep copy, so that would not handle it.

Or explicitly point out the Thread unsafety in the class documentation. This might well be desired in favor of performance as a tradeoff. I think Effective Concurrency In Java notes mentioning something like

<b>This method is not thread-safe</b>

in the java docs of a class.

See here "Document Thread-Safety"

Originally the class wasn't designed to be thread-safe, because there would only be one thread processing a PATCH call, but since removing this convenient method results in thread-safety I will do that.

abhishekjain88 · 2016-12-17T02:21:37Z

src/main/java/io/datanerds/avropatch/Patch.java

@@ -35,16 +45,29 @@ public boolean addOperation(Operation operation) {
        return Collections.unmodifiableList(operations);
    }

+    public Map<String, ?> getHeaders() {


Not a concurrency issue with the map as long as the map is not open to modification. It might not be the case right now but leaves the code open to errors in case there is a write interface exposed to the map erroneously in the future. Would suggest initializing the map as immutable during initialization in the constructor.

the map should not be open to modifications

It's not:

public Map<String, ?> getHeaders() { return Collections.unmodifiableMap(headers); }

Agreed that it is not open to modifications to external modifiers but from a code extensibility perspective, if there is a method in this class which tries to modify this map, extra care would need to be taken to ensure that the map is made immutable then. By making it immutable now, we can future proof it so that we don't run the risk of somebody not making it immutable/thread safe while adding code modifying it in future.

abhishekjain88 · 2016-12-17T02:23:23Z

src/main/java/io/datanerds/avropatch/Patch.java

    private final List<Operation> operations;

    public Patch() {
-        operations = new ArrayList<>();
+        this(new ArrayList<>(), new HashMap<>());
    }

    public Patch(List<Operation> operations) {


Always a good idea to declare method arguments as final (especially in case of collections) which makes it explicit to the calling function that the list would not be modified in the called method.

Disagree. The final only states that the reference can't be reassigned within the block. The list is still mutable.

Agree though on parameter declarations being implicitly final would be a good thing :) Though I think that the boilerplate doesn't satisfy the improvement, would support a source code check rather for a variable being reassigned, as i.e. Findbugs supports AFAIR.

abhishekjain88 · 2016-12-17T02:27:06Z

src/main/java/io/datanerds/avropatch/Patch.java

        return outputStream.toByteArray();
    }

    public static Patch of(byte[] bytes) throws IOException {
-        DatumReader<Patch> reader = new ReflectDatumReader<>(SCHEMA);
-        return reader.read(null, DecoderFactory.get().binaryDecoder(bytes, null));
+        return SERIALIZER.reader.read(null, DecoderFactory.get().binaryDecoder(bytes, null));


I assume we are not reusing the read "Patch" object here because it is being returned to an external method as it is, correct?

Yes and we'd like to have thread-safety

abhishekjain88 · 2016-12-17T02:29:58Z

src/main/java/io/datanerds/avropatch/value/conversion/AvroData.java

+        CONVERTERS.forEach(this::addLogicalTypeConversion);
+    }
+
+    public static AvroData get() {


getInstance() sounds like a more intuitive method name to me. Just a good to have. We can agree or disagree. :)

That is a matter of taste IMHO. There are variations of this like instance() or getSingleton() or whatever :) As far as we agree on what it means this should be OK. Although I generally would favor a more descriptive method name to support static import of the method...

It hides ReflectData::get. Since it's static I cannot annotate with @Override. Will add javadoc instead.

abhishekjain88 · 2016-12-17T02:43:27Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+    interface PatchType {
+        String NAME = Patch.class.getSimpleName();
+        String DOC = "This record represents a PATCH holding a sequence of operations to apply to a given object.";
+        static Schema create(Schema valueSchema) {


Defining an interface method. This is new to me. Would love to understand more.

abhishekjain88 · 2016-12-17T02:44:56Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+        String NAME = Patch.class.getSimpleName();
+        String DOC = "This record represents a PATCH holding a sequence of operations to apply to a given object.";
+        static Schema create(Schema valueSchema) {
+            Schema.Field operations = new Schema.Field("operations",


Better to move the literal ("operations") to a constant before using it here and other places.

abhishekjain88 · 2016-12-17T02:45:17Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+        String NAME = "big-decimal";
+        String DOC = "BigDecimal value represented via it's scale and unscaled value.";
+        LogicalType LOGICAL_TYPE = new LogicalType(NAME);
+        Schema RECORD = SchemaBuilder.record("decimal").doc(DOC).fields()


Better to move the literal ("decimal") to a constant before using it here and other places.

abhishekjain88 · 2016-12-17T02:46:37Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+        String DOC = "BigDecimal value represented via it's scale and unscaled value.";
+        LogicalType LOGICAL_TYPE = new LogicalType(NAME);
+        Schema RECORD = SchemaBuilder.record("decimal").doc(DOC).fields()
+                .name("unscaledValue").type(BigIntegerType.SCHEMA).noDefault()


The same comment for all literals. I guess these are the same literals used in the DefaultSchema class. It would be better for code coupling to put them in a common class of constants and use the constants at both places.

larsp · 2016-12-19T20:30:49Z

AvroPatchException could become abstract

larsp · 2016-12-14T21:40:26Z

project.gradle

@@ -16,8 +16,9 @@ project.ext {
 dependencies {

    compile([
-            "org.apache.avro:avro:1.8.0",
-            "org.slf4j:slf4j-api:1.7.18"


slf4j version should become a variable since it is also used in testCompile

larsp · 2016-12-14T21:46:49Z

src/main/java/io/datanerds/avropatch/operation/DefaultSchema.java

+ * This interface holds the schema for all valid value types.
+ */
+public interface DefaultSchema {
+    String VALUE =


Would probably make it more readable?

larsp · 2016-12-15T01:30:50Z

src/main/java/io/datanerds/avropatch/Patch.java

 public class Patch {

-    public static final Schema SCHEMA = ReflectData.get().getSchema(Patch.class);
-    private static final DatumWriter<Patch> writer = new ReflectDatumWriter<>(SCHEMA);
+    private static final PatchSerializer SERIALIZER = new PatchSerializer();


Why is domain and (de)-serialization combined?

larsp · 2016-12-19T18:38:58Z

src/main/java/io/datanerds/avropatch/Patch.java

+    private static final PatchSerializer SERIALIZER = new PatchSerializer();
+
+    @AvroSchema(DefaultSchema.HEADERS)
+    private final Map<String, ?> headers;


Shouldn't headers be a multimap?

larsp · 2016-12-19T18:45:08Z

src/main/java/io/datanerds/avropatch/Patch.java

+ *
+ * @see <a href="https://tools.ietf.org/html/rfc6902">https://tools.ietf.org/html/rfc6902</a>
+ */
+@ThreadSafe


I think helper methods for getFirstOperation and getSingle/FirstHeader(somekey) would be nice

larsp · 2016-12-19T19:36:38Z

src/main/java/io/datanerds/avropatch/value/conversion/UUIDConversion.java

+        ByteBuffer buffer = ByteBuffer.wrap(value.bytes());
+        long leastSignificantBits = buffer.getLong();
+        long mostSignificantBits = buffer.getLong();
+        return new UUID(mostSignificantBits, leastSignificantBits);


nice, just the two longs! 👍

larsp · 2016-12-19T19:37:18Z

src/test/java/io/datanerds/avropatch/PatchTest.java

    }

    @Test
    public void serializesCopy() throws IOException {
-        Patch patch = new Patch();
-        patch.addOperation(new Copy(Path.parse("/person/firstName"), Path.parse("/person/lastName")));
+        Patch patch = patchOf(new Copy(Path.parse("/person/firstName"), Path.parse("/person/lastName")));


maybe copy,move etc need a method where you can pass a string which will be parsed?

larsp · 2016-12-19T20:17:48Z

src/main/java/io/datanerds/avropatch/Patch.java

    }

-    public boolean addOperation(Operation operation) {
-        return operations.add(operation);
+    public Patch(List<Operation> operations, Map<String, ?> headers) {


Should we do a sanity check that the value instances are of a supported type?

I created issue #10 for this feature

larsp · 2016-12-19T20:22:19Z

src/main/java/io/datanerds/avropatch/operation/DefaultSchema.java

+ * This interface holds the schema for all valid value types.
+ */
+public interface DefaultSchema {
+    String VALUE =


In that case generating that file from a json is probably the easiest?

larsp · 2016-12-19T20:33:46Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+                .endRecord();
+    }
+
+    Schema VALUE_TYPE_UNION = createUnion(BOOLEAN, DOUBLE, FLOAT, INTEGER, LONG, NULL, STRING,


Would appreciate a comment what it does :D

utobi

Would be great if we could do something about the withArray() endArray() stuff. Does not seem like the best solution for what we want to solve there.

utobi · 2016-12-19T13:40:52Z

src/main/java/io/datanerds/avropatch/serialization/CustomTypeSerializer.java

+            return this;
+        }
+
+        public Builder.ArrayBuilder withArray() {


Not a particularly good choice regarding naming IMO. Not obvious that you are starting a subbuilder here that requires endArray to be called to close it. Same goes for endArray. It will be very hard looking at code using this that there is a nested structure in there.

How about something like public Builder withArray(Builder.ArrayBuilder arrayBuilder) and then go from there? Having the ArrayBuilder explicitly as a method parameter would emphasise the nesting.

utobi · 2016-12-19T13:41:11Z

src/main/java/io/datanerds/avropatch/serialization/CustomTypeSerializer.java

+                this.parentBuilder = parentBuilder;
+            }
+
+            public Builder endArray() {


See comment regarding withArray

utobi · 2016-12-19T13:49:36Z

src/main/java/io/datanerds/avropatch/schema/CustomTypes.java

+    }
+
+    interface DateType {
+        String NAME = "timestamp";


Date or timestamp? What is it? :D

utobi · 2016-12-19T14:12:28Z

src/test/java/io/datanerds/avropatch/PatchTest.java

+        Date date = new Date();
+        UUID uuid = UUID.randomUUID();
+        Patch patch = new Patch(ImmutableList.of(
+                new Add<>(Path.of("some", "value"), "John Doe"),


API wise this seems to be strange and very cumbersome. Path.of seems to be a looooot of boilerplate. Especially when it is not possible (maybe I'm missing something?) to create a base path and then just add a child to it.

IMO Path either needs a possibility to have a base path and then add children or otherwise people will likely start using it like this which does not seem to be right:

String base = "/some/namespace/here/"; [...] new Add<>(Path.parse(base+"foobar"), "Knackwurst");

You could do new Add<>(Path.of(base, "foobar"), but it needs documentation. A method append(...) makes sense either way :-)

utobi · 2016-12-19T14:17:13Z

src/test/java/io/datanerds/avropatch/operation/matcher/PatchMatcher.java

+    }
+
+    private static Matcher<Patch> operationsEqualTo(Patch expected) {
+        return new CustomTypeSafeMatcher<Patch>(Joiner.on(",").join(expected.getOperations())) {


Java 8 has String.join() in case you haven't seen that one yet :)

utobi · 2016-12-19T14:19:18Z

src/test/java/io/datanerds/avropatch/serialization/ConcurrencyTest.java

+                .withCustomTypes()
+                .nullable()
+                .with(Bimmel.class)
+            .endArray()


This is what I meant with withArray() and endArray(). Very hard to see whats going on here, especially with the next line being indented one more level :)

utobi · 2016-12-20T12:05:22Z

src/main/java/io/datanerds/avropatch/serialization/CustomTypeSerializer.java

+            return this;
+        }
+
+        public Builder with(Schema schema) {


withSchema()?

Schema seems to be the most simple building block of the List this builder is creating, so having a simple method name for this case might be okay.

utobi · 2016-12-20T12:05:28Z

src/main/java/io/datanerds/avropatch/serialization/CustomTypeSerializer.java

+            return this;
+        }
+
+        public Builder with(Type type) {


withType()?

dguenms · 2016-12-20T12:42:40Z

src/main/java/io/datanerds/avropatch/serialization/CustomTypeSerializer.java

+    }
+
+    public static class Builder {
+        protected final List<Schema> types;


Considering that both a method for adding a type and a schema exist, the combination of declaration and variable name is a little confusing.

larsp

Oh... one can set this one here. 😃

jckuester · 2017-01-03T12:08:08Z

src/main/java/io/datanerds/avropatch/Patch.java

    private final List<Operation> operations;

-    public Patch() {
-        operations = new ArrayList<>();
+    @SuppressWarnings("unused") // no arg constructor needed by Avro


rename to: no-arg constructor, otherwise the meaning is that an arg constructor is not needed. Change everywhere else, too.

jckuester · 2017-01-03T12:09:47Z

src/main/java/io/datanerds/avropatch/operation/Copy.java

@@ -13,9 +13,9 @@
    public final Path from;
    public final Path path;

+    @SuppressWarnings("unused") // no arg constructor needed by Avro


jckuester · 2017-01-03T12:21:58Z

src/main/java/io/datanerds/avropatch/operation/Replace.java

@@ -12,20 +12,20 @@
    @AvroIgnore
    public static final String op = "replace";


Why/where is this string needed anyway? Is it not redundant information?

jckuester · 2017-01-03T13:58:21Z

src/test/java/io/datanerds/avropatch/PatchTest.java

+                        "header 2", new Date(),
+                        "header 3", 1234L,
+                        "header 4", new BigDecimal("3214123453.123512345")));
+        byte[] bytes = patch.toBytes();;


jckuester · 2017-01-03T14:10:56Z

src/main/java/io/datanerds/avropatch/Patch.java

+    public Map<String, ?> getHeaders() {
+        return Collections.unmodifiableMap(headers);
+    }
+
    public byte[] toBytes() throws IOException {
        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
        Encoder binaryEncoder = EncoderFactory.get().directBinaryEncoder(outputStream, null);


this local variables binaryEncoder and outputStream can probably shortened for readability, as they are used only once and right after declaration. (see heuristic N5 in Clean Code).

jckuester · 2017-01-03T14:59:48Z

src/main/java/io/datanerds/avropatch/schema/OperationTypes.java

+        String NAME = io.datanerds.avropatch.operation.Add.class.getSimpleName();
+        String DOC = "This record represents the add operation of RFC 6902 for JSON Patch'.";
+
+        static Schema create(Schema valueSchema) {


IMHO, the name valueSchema is not concise enough. what is it really, the allowed types of a field in the schema?

is the name valueSchema only used because value is used already below?

jckuester · 2017-01-03T15:08:10Z

src/test/java/io/datanerds/avropatch/serialization/ConcurrencyTest.java

+                .nullable()
+                .with(Bimmel.class)
+            .endArray()
+                .withPrimitives()


is the indentation correct here?

jckuester · 2017-01-03T15:20:11Z

src/test/java/io/datanerds/avropatch/value/conversion/UUIDConversionTest.java

+                .withSchemata(UuidType.SCHEMA)
+                .withConverters(new UUIDConversion())
+                .reserializeAndAssert(UUID.randomUUID())
+                .reserializeAndAssert(UUID.randomUUID());


is testing one randomUUID not enough?

It should be ;-)

jckuester · 2017-01-03T15:27:06Z

src/main/java/io/datanerds/avropatch/serialization/CustomTypeSerializer.java

+    }
+
+    public byte[] toBytes(Patch value) throws IOException {
+        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();


shorten local variable names (see comment somewhere above).

frankwis added 6 commits November 4, 2016 12:04

Added custom types and conversion for UUID, Date, BigInteger and BigD…

66fa8b6

…ecimal.

Fixed initialization of custom type converters.

1ff8078

Added types for custom values and operations, fixed conversion.

9138f96

Added CustomTypeSerializer and corresponding Builder.

daf2c25

Added headers to Patch operation.

a477769

Added concurrency tests for serialization.

04d8e9c

This was referenced Nov 17, 2016

Feature/custom types #3

Closed

Feature/introducing headers to patch #5

Closed

Feature/custom value type serialization #4

Closed

frankwis requested review from utobi, larsp, obiwen, dguenms, dhiller and jckuester December 8, 2016 22:04

dhiller suggested changes Dec 14, 2016

View reviewed changes

frankwis requested a review from thgreiner December 14, 2016 13:58

thgreiner reviewed Dec 14, 2016

View reviewed changes

frankwis added 2 commits December 14, 2016 15:40

PRR (@dhiller): Refactored OperationGenerator.Type to enum strategy p…

8116216

…attern.

PRR (@dhiller): Refactored value generator.

22315e5

frankwis requested a review from abhishekjain88 December 15, 2016 08:14

frankwis added 3 commits December 15, 2016 09:48

PRR (@dhiller): Some code enhancements.

79d4221

Suppress unused warning on private Avro constructors.

65adfdf

PRR (@dhiller): Cleaned up test classes.

41b78f4

thgreiner reviewed Dec 15, 2016

View reviewed changes

dhiller suggested changes Dec 15, 2016

View reviewed changes

obiwen reviewed Dec 15, 2016

View reviewed changes

frankwis mentioned this pull request Dec 15, 2016

Add equals and hashCode to Operation classes #7

Closed

PRR (@dhiller): Incorporated some suggestions.

f741adf

obiwen approved these changes Dec 15, 2016

View reviewed changes

dhiller reviewed Dec 15, 2016

View reviewed changes

dhiller approved these changes Dec 15, 2016

View reviewed changes

thgreiner approved these changes Dec 16, 2016

View reviewed changes

dguenms suggested changes Dec 16, 2016

View reviewed changes

abhishekjain88 suggested changes Dec 17, 2016

View reviewed changes

PRR @abhishekjain88 & @dguenms

e29f313

dguenms approved these changes Dec 19, 2016

View reviewed changes

larsp reviewed Dec 19, 2016

View reviewed changes

utobi requested changes Dec 20, 2016

View reviewed changes

dguenms reviewed Dec 20, 2016

View reviewed changes

larsp requested changes Dec 20, 2016

View reviewed changes

jckuester reviewed Jan 3, 2017

View reviewed changes

frankwis added 6 commits January 5, 2017 13:49

Removed operation literal for now.

4c0754b

Reorganized value type schemata.

52f8728

Seperated serialization from domain.

c6777a6

Refactored builders for the PatchMapper.

d5189d3

Abstraced builder for PatchMapper, array schema and map schema.

8a10dc2

Added convinience methods to Path.

b9f9eed

utobi approved these changes Jan 12, 2017

View reviewed changes

larsp approved these changes Jan 12, 2017

View reviewed changes

frankwis merged commit 32f65d0 into develop Jan 13, 2017

frankwis deleted the feature/Concurrency_tests branch January 13, 2017 04:21

		@@ -12,20 +12,20 @@
		@AvroIgnore
		public static final String op = "replace";

Feature/concurrency tests #6

Feature/concurrency tests #6

Conversation

frankwis commented Nov 17, 2016

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

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

dhiller Dec 15, 2016 • edited Loading

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

dguenms 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

dguenms Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekjain88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhiller Dec 19, 2016 • edited Loading

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

dhiller Dec 19, 2016 • edited Loading

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

larsp commented Dec 19, 2016

dhiller Dec 15, 2016 •

edited

Loading

dguenms Dec 16, 2016 •

edited

Loading

dhiller Dec 19, 2016 •

edited

Loading

dhiller Dec 19, 2016 •

edited

Loading