[Proposal] Fix #35 #38

guizmaii · 2019-11-07T22:44:25Z

Problems

As I understand it, the issue #35 expresses two different problems.

As I understand it, the first problem is the lack of extensibility of the error model in SimpleSource.

Currently, there's a finite set of possible error Reasons that the user has to use to produce a CommandError.
When a user want its own error to express his business error, he can only specify a message, which is a String, to customize the CommandError.
So, for now, the user business errors are expressed as aString combined with a Reason that, hopefully, matches the business error expressed in the message.

The second problem expressed in #35, is that currently the errors have to be handled in two different places:

The CommandHandler for business errors
Outside the CommandHandler for NotFoundErrors

To keep things simple, in this PR I only try to fix the first problem.

Proposal

My proposition in this PR follows these constraints.

I want the user to be able to extend our error model to express its own business errors.
I want composability.
I want something that a Java developer can understand.
I want as much as type safety as possible.
I want to stay close to the previous error management model because I can't spend 2 weeks to work on that.

My proposition in this PR to fix that problem is the following.

The typical Java error management is done via exceptions.
Exceptions are extensible (inheritance) and composable (thanks to their constructor magic).
So, in this PR I explore a solution based on exceptions.

The solution is simple.
Now, the CommandError extends RuntimeException.
The Reasons become (final) classes that extends CommandError.
If the user want to express his own errors, he can create classes extending CommandError.

Limitations

1. Exceptions serialization

SimpleSource is encoding the CommandErrors as a product of a String and a Reason (which is an enum) for a good reason: it serializes them.

As explained in this comment #38 (comment), the problem with exceptions is that, apparently, no one (except maybe Akka, I still have to check that) knows how to correctly and efficiently serialize them.

As explained in the previous comment, I implemented a serialization mechanism but:

it's not an efficient one
it produces potentially huge messages.
I think that it's unsafe because, IIRC, Java can only serialize classes that extend Serializable AND that contain fields that are themselves serializable. Otherwise, I don't know what happen. My hypothesis is that it crashes at runtime?
I don't think that it's very robust in regards of error modeling evolutions. If the user changes his encoding of one of its 'CommandError', I don't think that Java will be able to deserialize old errors.

The advantage of that mechanism is that it's generic and the user doesn't need to provide any custom serializer.

2. Java Serialization

The serialization mechanism provided uses the Java serialization mechanism which is known to be awful.

Example of use

I adapted the simplesource-examples with this solution here: https://github.com/simplesourcing/simplesource-examples/pull/21/files

guizmaii · 2019-11-07T23:11:09Z

...command-serialization/src/main/java/io/simplesource/kafka/serialization/avro/AvroSerdes.java

@@ -207,7 +198,7 @@ private static Schema aggregateUpdateResultSchema(final Schema aggregateSchema)
        private static GenericRecord fromReason(final Schema schema, final CommandError commandError) {
            return new GenericRecordBuilder(schema)
                    .set(ERROR_MESSAGE, commandError.getMessage())
-                    .set(ERROR_CODE, commandError.getReason().name())
+                    .set(ERROR, Base64.getEncoder().encodeToString(SerializationUtils.serialize(commandError)))


With Avro, we can maybe be more efficient in terms of space use by using Base64.getEncoder().encode in place of Base64.getEncoder().encodeToString and declare the field as a byteType here https://github.com/simplesourcing/simplesource/pull/38/files#diff-717500181ed887fe65e9940056d96a5cR236

but the problem is still the same in terms of computation time and doesn't help for the JSON serialization.

I tested. I can't make the previous proposition work. I don't know why.

That would definitely be better for Avro serialization.

Making the field a bytesType should work. You may need to wrap the byte[] as a ByteBuffer
ByteBuffer.wrap(SerializationUtils.serialize(commandError)) or something like that.

For Json, a Base64 encoded string would be acceptable.

guizmaii · 2019-11-08T06:27:18Z

...command-serialization/src/main/java/io/simplesource/kafka/serialization/avro/AvroSerdes.java

        private static final String WRITE_SEQUENCE = "writeSequence";
        private static final String AGGREGATION = "aggregate_update";

        private static GenericRecord fromReason(final Schema schema, final CommandError commandError) {
            return new GenericRecordBuilder(schema)
                    .set(ERROR_MESSAGE, commandError.getMessage())
-                    .set(ERROR_CODE, commandError.getReason().name())
+                    .set(ERROR_CLASS, commandError.getClass().getCanonicalName())


ERROR_MESSAGE and ERROR_CLASS are here for debug purpose. They're not used in the deserialization mechanism.

Because we are embedding Java serialised classes in Avro, we won't otherwise be able to view the exception on via the avro console viewer, so we probably still need these, much as I'd like to remove them.

guizmaii · 2019-11-08T06:27:24Z

...command-serialization/src/main/java/io/simplesource/kafka/serialization/avro/AvroSerdes.java

        }

        private static Schema aggregateUpdateResultSchema(final Schema aggregateSchema) {
            final Schema reasonSchema = SchemaBuilder
                    .record(aggregateSchema.getName() + "Reason")
                    .fields()
                    .name(ERROR_MESSAGE).type().stringType().noDefault()
-                    .name(ERROR_CODE).type().stringType().noDefault()
+                    .name(ERROR_CLASS).type().stringType().noDefault()


ERROR_MESSAGE and ERROR_CLASS are here for debug purpose. They're not used in the deserialization mechanism.

guizmaii · 2019-11-08T06:28:02Z

...command-serialization/src/main/java/io/simplesource/kafka/serialization/avro/AvroSerdes.java

@@ -207,7 +202,8 @@ private static Schema aggregateUpdateResultSchema(final Schema aggregateSchema)
        private static GenericRecord fromReason(final Schema schema, final CommandError commandError) {
            return new GenericRecordBuilder(schema)
                    .set(ERROR_MESSAGE, commandError.getMessage())
-                    .set(ERROR_CODE, commandError.getReason().name())
+                    .set(ERROR_CLASS, commandError.getClass().getCanonicalName())


ERROR_MESSAGE and ERROR_CLASS are here for debug purpose. They're not used in the deserialization mechanism.

guizmaii · 2019-11-08T06:29:16Z

...erialization/src/main/java/io/simplesource/kafka/serialization/json/JsonAggregateSerdes.java

@@ -272,7 +273,8 @@ public JsonElement serialize(
        private JsonElement serializeReason(final CommandError commandError) {
            final JsonObject wrapper = new JsonObject();
            wrapper.addProperty(ERROR_MESSAGE, commandError.getMessage());
-            wrapper.addProperty(ERROR_CODE, commandError.getReason().name());
+            wrapper.addProperty(ERROR_CLASS, commandError.getClass().getCanonicalName());


ERROR_MESSAGE and ERROR_CLASS are here for debug purpose. They're not used in the deserialization mechanism.

guizmaii · 2019-11-08T06:29:32Z

...command-serialization/src/main/java/io/simplesource/kafka/serialization/json/JsonSerdes.java

@@ -126,7 +129,8 @@ public JsonElement serialize(
        private JsonElement serializeReason(final CommandError commandError) {
            final JsonObject wrapper = new JsonObject();
            wrapper.addProperty(ERROR_MESSAGE, commandError.getMessage());
-            wrapper.addProperty(ERROR_CODE, commandError.getReason().name());
+            wrapper.addProperty(ERROR_CLASS, commandError.getClass().getCanonicalName());


ERROR_MESSAGE and ERROR_CLASS are here for debug purpose. They're not used in the deserialization mechanism.

guizmaii · 2019-11-08T06:31:11Z

simplesource-data/src/main/java/io/simplesource/data/OptionalExtension.java

+import java.util.function.Function;
+import java.util.function.Supplier;
+
+public class OptionalExtension {


TODO: Remove this class

guizmaii · 2019-11-08T06:32:15Z

simplesource-data/src/test/java/io/simplesource/data/FutureResultTest.java

-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
-class FutureResultTest {


I removed these tests just because I didn't want to bother to make them compile again.

TODO: Put back these tests

guizmaii · 2019-11-08T06:32:39Z

simplesource-data/src/main/java/io/simplesource/data/Pair.java

+/**
+ * https://stackoverflow.com/a/28526052/2431728
+ */
+@Value(staticConstructor = "of")


TODO: Remove this class

guizmaii · 2019-11-08T06:37:10Z

...command-serialization/src/main/java/io/simplesource/kafka/serialization/avro/AvroSerdes.java

        private static final String WRITE_SEQUENCE = "writeSequence";
        private static final String AGGREGATION = "aggregate_update";

        private static GenericRecord fromReason(final Schema schema, final CommandError commandError) {
            return new GenericRecordBuilder(schema)
                    .set(ERROR_MESSAGE, commandError.getMessage())
-                    .set(ERROR_CODE, commandError.getReason().name())
+                    .set(ERROR_CLASS, commandError.getClass().getCanonicalName())
+                    .set(ERROR, Base64.getEncoder().encodeToString(SerializationUtils.serialize(commandError)))


Inefficient in terms of computation time and space taken by the produced message.

The question is: How do you correctly and efficiently serialize an Exception?

Kryo still does not provide a solution for that apparently. See EsotericSoftware/kryo#532
Chill doesn't help either: twitter/chill#327

Maybe Akka can help us here. They have a Status type that is a kind of serializable Try. See https://github.com/akka/akka/blob/b006567751f1ef63588b00e20ec51c905e8562d3/akka-actor/src/main/scala/akka/actor/Actor.scala#L310

I don't know how they serialize it, for now.

Off topic: Interesting read: https://lemire.me/blog/2019/01/30/what-is-the-space-overhead-of-base64-encoding/

Yeah, this is the big problem here.
Whatever we use as exception / error type has to be serializable.
I wouldn't want to bring in any library dependencies for this either.

szoio

Happy with this change. Big improvement on what we have right now, more type safe, I suppose idiomatic, and in the end cleaner and simpler.
Would like to see the error being serialized as a ByteArray for Avro serialization.
The one downside is the errors are not viewable as Avro, so we have to keep the string message as well to make it debuggable. I can live with that.
Biggest downside/risk is we are tied an opaque exception serialization mechanism that may conceivable need to change, and we have no easy way of evolving this.

szoio · 2019-11-11T01:39:29Z

...command-serialization/src/main/java/io/simplesource/kafka/serialization/avro/AvroSerdes.java

@@ -207,7 +198,7 @@ private static Schema aggregateUpdateResultSchema(final Schema aggregateSchema)
        private static GenericRecord fromReason(final Schema schema, final CommandError commandError) {
            return new GenericRecordBuilder(schema)
                    .set(ERROR_MESSAGE, commandError.getMessage())
-                    .set(ERROR_CODE, commandError.getReason().name())
+                    .set(ERROR, Base64.getEncoder().encodeToString(SerializationUtils.serialize(commandError)))


That would definitely be better for Avro serialization.

Making the field a bytesType should work. You may need to wrap the byte[] as a ByteBuffer
ByteBuffer.wrap(SerializationUtils.serialize(commandError)) or something like that.

For Json, a Base64 encoded string would be acceptable.

szoio · 2019-11-12T23:37:35Z

Thinking about this a bit more, for me, for me Java serialisation is a bit of a deal breaker.
As much as Simple Sourcing is Java end-to-end, all the data it produces should ideally be readable by any supported Kafka client in any language.

andybritz · 2019-11-19T21:10:24Z

Tbh not sure this yet achieve its primary goals.

Removal of string comparisons on custom errors.
Simplification

I think while this adds some typing, it comes at a high complexity and I suspect potentially with no backward compatibility. I think we need spend more time on this on.

guizmaii · 2019-11-19T22:20:42Z

@szoio @andybritz Yes, after a second thought, I also think that this proposal is not good enough. The serialization problem is important. We can't solve it with a simple solution like this.

I don't remember how Lagom is handling this problem, especially with Java since I always used it with Scala, but it may be interesting to see how they solve it.

The problem exposed in #35, to be fixed, requires to first find how to serialize errors.

guizmaii added 2 commits November 8, 2019 09:43

Simplify the error management: Embrace the Java way of error management

aaafda4

Clean

9305709

guizmaii commented Nov 7, 2019

View reviewed changes

guizmaii mentioned this pull request Nov 7, 2019

[WIP] Work on https://github.com/simplesourcing/simplesource/pull/38 simplesourcing/aws-demo#2

Closed

guizmaii added 3 commits November 8, 2019 11:08

Provided errors must be final

d58a303

Add some functional helpers

ddbfdc1

Make fold lazy

8396956

guizmaii mentioned this pull request Nov 8, 2019

Use the new error management introduced by https://github.com/simplesourcing/simplesource/pull/38 simplesourcing/simplesource-examples#21

Closed

guizmaii added 4 commits November 8, 2019 15:28

Set JD versions in Travis

00df4da

Clean

081245a

Clean

d70b5da

Improve debuggability

549657e

guizmaii commented Nov 8, 2019

View reviewed changes

guizmaii changed the title ~~[WIP] Fix #35~~ [Proposal] Fix #35 Nov 8, 2019

szoio reviewed Nov 11, 2019

View reviewed changes

guizmaii closed this Nov 19, 2019

szoio mentioned this pull request Nov 20, 2019

Better client API for error handling #35

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proposal] Fix #35 #38

[Proposal] Fix #35 #38

guizmaii commented Nov 7, 2019 •

edited

Loading

guizmaii Nov 7, 2019 •

edited

Loading

guizmaii Nov 8, 2019

szoio Nov 11, 2019

guizmaii Nov 8, 2019 •

edited

Loading

szoio Nov 11, 2019

guizmaii Nov 8, 2019 •

edited

Loading

guizmaii Nov 8, 2019

guizmaii Nov 8, 2019

guizmaii Nov 8, 2019

guizmaii Nov 8, 2019 •

edited

Loading

guizmaii Nov 8, 2019 •

edited

Loading

guizmaii Nov 8, 2019

guizmaii Nov 8, 2019 •

edited

Loading

szoio Nov 11, 2019

szoio left a comment

szoio Nov 11, 2019

szoio commented Nov 12, 2019

andybritz commented Nov 19, 2019

guizmaii commented Nov 19, 2019 •

edited

Loading

[Proposal] Fix #35 #38

[Proposal] Fix #35 #38

Conversation

guizmaii commented Nov 7, 2019 • edited Loading

Problems

Proposal

Limitations

1. Exceptions serialization

2. Java Serialization

Example of use

guizmaii Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guizmaii Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guizmaii Nov 8, 2019 • 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

guizmaii Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

guizmaii Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guizmaii Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szoio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szoio commented Nov 12, 2019

andybritz commented Nov 19, 2019

guizmaii commented Nov 19, 2019 • edited Loading

guizmaii commented Nov 7, 2019 •

edited

Loading

guizmaii Nov 7, 2019 •

edited

Loading

guizmaii Nov 8, 2019 •

edited

Loading

guizmaii Nov 8, 2019 •

edited

Loading

guizmaii Nov 8, 2019 •

edited

Loading

guizmaii Nov 8, 2019 •

edited

Loading

guizmaii Nov 8, 2019 •

edited

Loading

guizmaii commented Nov 19, 2019 •

edited

Loading