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

Schema-registry integration #593

Merged
merged 2 commits into from
Oct 14, 2016
Merged

Schema-registry integration #593

merged 2 commits into from
Oct 14, 2016

Conversation

dankraw
Copy link
Contributor

@dankraw dankraw commented Oct 6, 2016

This PR introduces support for Confluent Schema Registry as the next default Avro schema repository.

Important changes:

  • removed support for JSON schema, JSON topics validation & validationDryRun (including changes in hermes-console) - from now on schemas are supported for Avro topics only
  • removed Zookeeper schema repository implementation (didn't support all needed features)
  • upgraded kafka library from 0.8.2.1 to 0.10.0.1 (dependency upgrade only)
  • separated hermes-schema module for schema-related content (note: DI-factory classes, Avro message wrappers / SerDe are left in hermes-common)

@dankraw dankraw force-pushed the schema-registry branch 3 times, most recently from a0fbe68 to 016b344 Compare October 7, 2016 13:10
Copy link
Contributor

@druminski druminski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🏆

@dankraw dankraw force-pushed the schema-registry branch 2 times, most recently from 9f1e3f9 to 330b1ae Compare October 12, 2016 06:49

import java.util.Optional;

import static pl.allegro.tech.hermes.api.ContentType.AVRO;
import static pl.allegro.tech.hermes.api.TopicName.fromQualifiedName;

@Component
public class SchemaSourceService {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder - we use term schema source everywhere and i don't know if it's a proper one - schema source sounds like source of schema aka schema repository, while what we mean is schema itself (aka raw schema in text format). Maybe it's a good time to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll rename it to RawSchema.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming done.

@@ -0,0 +1,240 @@
package pl.allegro.tech.hermes.schema.schemarepo;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it in java and not groovy? this is weird, as other tests are in groovy in this module

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been reusing old tests, transforming them to groovy shouldn't take long. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, all hermes-schema tests are now groovy :)

@adamdubiel
Copy link
Collaborator

adamdubiel commented Oct 12, 2016

I'm mostly confused with the naming, but all in all - good job :)

@dankraw dankraw merged commit d715ecc into develop Oct 14, 2016
@dankraw dankraw deleted the schema-registry branch October 14, 2016 14:07
@adamdubiel adamdubiel added this to the 0.9.0 milestone Oct 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants