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

Dry up Common Fields #2

Closed
original-brownbear opened this issue Aug 24, 2018 · 4 comments
Closed

Dry up Common Fields #2

original-brownbear opened this issue Aug 24, 2018 · 4 comments
Labels

Comments

@original-brownbear
Copy link
Contributor

There is a lot of code duplication across all schemata coming from the common fields. Since we don't have inheritance in AVRO their code is currently duplicated across all other schemata.

  • Question 1: Do we want to fix this?
  • Question 2: If yes, what kind of fix do we want?
    • I see two possible approaches here:
      • If we want to keep the 1:1 correspondence between JSON and AVRO field hierarchy, then we could try to generate the AVRO sources before actually compiling them with the AVRO compiler. Concretely, we could have a placeholder {{common_fields}} or so that we can put in an avsc file and have Maven replace it with the common field code before compilation.
      • Better option: If we can live with having the AVRO and JSON hierarchy no correspond 1:1 then we could simply create a record schema CommonFields that has all the common fields in it and put it at the top level of all the other schemata under a key common_fields or so.
@acmeguy
Copy link
Contributor

acmeguy commented Aug 27, 2018

I suggest a third option and would like to know what you think about it.

Let's consolidate the sub-type classes like you suggest for Video events and leave the redundancy as it is. (That way there are a lot fever files/sub-classes involved)

@original-brownbear
Copy link
Contributor Author

@acmeguy

If that's an appropriate level of validation for you still, I like that approach. As far as I can see we have no type collisions between properties of various kind of mobile events and such so that should work out fine.

@acmeguy
Copy link
Contributor

acmeguy commented Aug 27, 2018

great. I think that this is also more flexible and re-usable for anyone wanting to use this lib.

@original-brownbear
Copy link
Contributor Author

Closing here, will handle the remaining bits in #5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants