Skip to content
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

Merged
merged 16 commits into from
Aug 19, 2020

Conversation

shiveshr
Copy link
Contributor

@shiveshr shiveshr commented Jul 24, 2020

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 -

  1. shared -- contains all the base classes and interfaces and codecs.
  2. avro -- serializers for avro
  3. protobuf -- serializers for protobuf
  4. json -- serializers for json

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.

@shiveshr shiveshr requested a review from fpj July 24, 2020 13:47
shiveshr added 3 commits July 29, 2020 03:28
Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
@shiveshr
Copy link
Contributor Author

@fpj i have renamed the packages to keep all serializers for avro, protobuf and json in following packages:
io.pravega.schemaregistry.serializer.<module-name>.impl

@fpj
Copy link
Contributor

fpj commented Jul 31, 2020

@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?

@fpj
Copy link
Contributor

fpj commented Jul 31, 2020

Another question, is it worth having an uber artifact with everything in addition to the individual ones for the different serialization schemes?

@fpj
Copy link
Contributor

fpj commented Jul 31, 2020

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

@shiveshr
Copy link
Contributor Author

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?

Although the serializers interface that we implement is the interface that pravega applications expect, the interface itself is quite generic.
It has serialize and deserialize methods and can actually be used outside of pravega applications too.

@shiveshr shiveshr requested a review from ravisharda August 11, 2020 13:28
build.gradle Outdated Show resolved Hide resolved
gradle/maven.gradle Show resolved Hide resolved
gradle/maven.gradle Show resolved Hide resolved
build.gradle Show resolved Hide resolved
build.gradle Show resolved Hide resolved

public class SerializerTest {
@Test
public void testAvroSerializers() {

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.

Signed-off-by: Shivesh Ranjan <[email protected]>
Signed-off-by: Shivesh Ranjan <[email protected]>
Copy link
Contributor

@fpj fpj left a 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.

@ravisharda ravisharda merged commit 6067dc5 into pravega:master Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants