-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 45: Split serializers into format specific libraries #84
Conversation
…from artifacts Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
serializers/avro/src/test/java/io/pravega/schemaregistry/testobjs/User.java
Outdated
Show resolved
Hide resolved
serializers/json/src/test/java/io/pravega/schemaregistry/schemas/SchemasTest.java
Outdated
Show resolved
Hide resolved
...d/src/main/java/io/pravega/schemaregistry/serializers/MultiplexedAndGenericDeserializer.java
Outdated
Show resolved
Hide resolved
...d/src/main/java/io/pravega/schemaregistry/serializers/MultiplexedAndGenericDeserializer.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
@fpj i have renamed the packages to keep all serializers for avro, protobuf and json in following packages: |
@shiveshr one thing that occurred to me while reviewing the recent changes: will we ever want to have serializers for other systems or is it going to be for Pravega and that's it? If we consider that we can have serializers for other systems, then what would be the package structure in that case? |
Another question, is it worth having an uber artifact with everything in addition to the individual ones for the different serialization schemes? |
Never mind, you have answered this question in this other thread: https://github.com/pravega/schema-registry/pull/84/files#r460948568 |
Although the serializers interface that we implement is the interface that pravega applications expect, the interface itself is quite generic. |
...lizers/avro/src/test/java/io/pravega/schemaregistry/serializer/avro/impl/SerializerTest.java
Outdated
Show resolved
Hide resolved
...zers/avro/src/main/java/io/pravega/schemaregistry/serializer/avro/impl/AvroDeserializer.java
Show resolved
Hide resolved
...ro/src/main/java/io/pravega/schemaregistry/serializer/avro/impl/AvroGenericDeserializer.java
Show resolved
Hide resolved
...avro/src/main/java/io/pravega/schemaregistry/serializer/avro/impl/AvroSerializerFactory.java
Outdated
Show resolved
Hide resolved
...avro/src/main/java/io/pravega/schemaregistry/serializer/avro/impl/AvroSerializerFactory.java
Outdated
Show resolved
Hide resolved
...avro/src/main/java/io/pravega/schemaregistry/serializer/avro/impl/AvroSerializerFactory.java
Outdated
Show resolved
Hide resolved
...avro/src/main/java/io/pravega/schemaregistry/serializer/avro/impl/AvroSerializerFactory.java
Outdated
Show resolved
Hide resolved
|
||
public class SerializerTest { | ||
@Test | ||
public void testAvroSerializers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A number of initializations and unrelated assert statements indicate that this test has multiple responsibilities. Please break it down into separate tests.
serializers/src/test/java/io/pravega/schemaregistry/serializers/SerializerTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve this one assuming a couple of thigs:
1- We won't be making deep changes to the existing PR. If we end up making deep changes, then please reset my review so that I can review it again.
2- We will soon complete publishing snapshots so that we can see more clearly how it looks like.
Change log description
Split serializers into format specific libraries
Purpose of the change
Fixes #45
What the code does
Breaks down serializer library into 4 different libraries -
Then there is the existing serializers library which depends on each of these individual libraries.
One subtle thing we do is, we have kept the package structure identical for all three new modules as the original instead of putting avro serializers in an avro package.This allows us to keep our classes accessible at package private access level rather than make them public.Only the factories and the user interfaces are public.However, this also means the classes across these three modules cannot share the same name if they are to be referenced in the combined serializers module.Removed split package as it is not supported by more recent java versions.
How to verify it
All existing tests should pass.