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

Maven plugin #379

Merged
merged 8 commits into from Jul 28, 2016

Conversation

Projects
None yet
2 participants
@jcustenborder
Copy link
Member

jcustenborder commented Jul 19, 2016

I have conversations all the time with users about the schema registry in their environment. One of the things I've said on more than one occasion is that integrating a new application is as simple as pulling down the schema and using the avro-maven-plugin. This is an attempt to make that process much easier. I added a few plugins.

The download mojo allows a developer to connect to schema registry and pull down all of the avro schemas to their local file system filtering by regular expressions. As schemas are pulled down they are parsed by an Avro parser to ensure they are valid, then written to disk.

The register mojo allows a developer to use maven to register schemas with their schema registry instance.

The test-compatibility mojo can be used to ensure that changes to schemas as checked against schema registry to ensure compatibility.

jcustenborder added some commits Jul 19, 2016

README.md Outdated
@@ -87,6 +87,109 @@ with Maven.
This project uses the [Google java code style](https://google-styleguide.googlecode.com/svn/trunk/javaguide.html)
to keep code clean and consistant.

Maven Plugin

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Do you want this in the docs/ as well? I actually don't know that maintaining an extensive README makes much sense given that we have the docs -- they are partly historical. I'd suggest treating the README more for developers (i.e. what is this repo? how do I build it? how do I contribute back changes) and leave usage stuff like this to the docs (i.e. I think some of the quickstart stuff could even be removed, in favor of a link to our generated docs).

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

That works for me. I'll move it to docs.

README.md Outdated
| schemaRegistryUrls | Urls for the schema registry instance to connect to | List | yes | |
| outputDirectory | Output directory to write the schemas to. | String | yes | |
| schemaExtension | The file extension to use for the output file name. This must begin with a '.' character. | String | no | .avsc |
| subjectPatterns | The subject patterns to download. This is a list of regular expressions. Patterns must match the entire subject name. | List | no | ^.+$ |

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Hmm, so this seems convenient, but I assume you're getting the latest from each subject? Doesn't that mean builds can't possibly be repeatable?

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

My thoughts were not necessarily to have this integrated into the phases of a build. More to be used adhoc during development to pull down the schemas and version them with your current project.

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 26, 2016

Member

Ok, that makes sense. I just like everything to be automated :)

README.md Outdated
<version>3.1.0-SNAPSHOT</version>
<configuration>
<schemaRegistryUrls>
<param>http://192.168.99.100:8081</param>

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Will it be possible to use profiles or something to customize this by dev/staging/prod?

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

I believe that is a standard feature of maven. I'm not sure I'll have to write anything for a user to implement profiles that are environment specific.

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 26, 2016

Member

Ack, I just don't know the details since I've not written maven plugins before

README.md Outdated
<param>http://192.168.99.100:8081</param>
</schemaRegistryUrls>
<subjects>
<TestSubject000-Key>src/main/avro/TestSubject000-Key.avsc</TestSubject000-Key>

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

These should use lowercase -key and -value to match the schema reg, right?

README.md Outdated
</plugin>
```


This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Overall, I think what this might need is a guide for how you might use this in practice, e.g. what stages do you attach each of these to? test-compatibility makes sense before a normal compile, I'm not sure when downloading makes sense, and register probably makes sense upon some deployment step (or a custom on you might use via a separate CI step?).

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

My thoughts were during CI steps. This could be something that happens where during a pull request builder it verifies the schema and rejects the pull request if it isn't compatible. I was thinking download for developers, CI for test-compatibility and register-schema for CD.

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 26, 2016

Member

Makes sense, maybe just add a very brief section explaining that setup as a reasonable starting point for how to use this?

<dependency>
<groupId>org.apache.maven.plugin-tools</groupId>
<artifactId>maven-plugin-annotations</artifactId>
<version>3.2</version>

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Why does this version differ from the previous one? And how do we select these versions -- minimum version with the features we need so it works with as broad a range of maven versions as possible? There are newer versions of both, and matching versions at 3.0 or 3.2.

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

It's confusing. It's not directly tied to maven versions.

@@ -0,0 +1,165 @@
/**
* Copyright 2015 Confluent Inc.

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Date is wrong (other files too)


@Parameter(required = false, defaultValue = ".avsc")
String schemaExtension;
@Parameter(required = false)

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Is it really ever reasonable to download a schema for every subject? I get that it probably makes it slightly easier to get started, but seems like bad practice....

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

I guess that is dependant upon the organization. If you're in a big shared cluster it will not make sense but if it's a use case specific cluster it will make sense. I went ahead and moved it to required with no defaults.

File outputDirectory;

@Parameter(required = false, defaultValue = "true")
boolean prettyPrintSchemas;

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

I don't think this was documented?

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

Got it thanks!

);
}

try {

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Not really a big deal, but you can put this and the previous try/catch in one block -- SchemaParseException isn't a RestClientException or IOException, so there shouldn't be any conflicts.

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

That'll work. I really wanted to be able to differentiate a network problem from a busted schema.

@Override
public void execute() throws MojoExecutionException, MojoFailureException {
try {
getLog().debug(String.format("Checking if '%s' and is not a directory.", this.outputDirectory));

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

I think you meant if '%s' exists

Map<String, File> subjects = new HashMap<>();


Map<String, Integer> schemaVersions;

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Is this just for testing or is it possible to use this info elsewhere?

This comment has been minimized.

Copy link
@jcustenborder

jcustenborder Jul 20, 2016

Author Member

Map<String, Integer> schemaVersions; is used by the test classes.

));
this.schemaVersions.put(kvp.getKey(), version);
} catch (IOException | RestClientException e) {
getLog().error(

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Hmm, should we just be logging the error? That won't cause the rest of the build to fail, will it? Should we capture the list of exceptions and throw a wrapper exception? Or even just the first one?

import java.util.Map;

/**
*

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Should either fill this in or remove it.

}

@Parameter(required = true)
List<String> schemaRegistryUrls;

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Normally fields and methods wouldn't be mixed like this -- could move this up above client(SchemaRegistryClient) definition.

}

if (errorCount > 0) {
throw new IllegalStateException("One or more schema was found to be incompatible with the current version.");

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Are these expected, in addition to MojoExecutionException and MojoFailureException?


@Before
public void before() throws IOException {
this.tempDirectory = File.createTempFile(this.getClass().getSimpleName(), "tmp");

This comment has been minimized.

Copy link
@ewencp
}

@Test
public void download() throws IOException, RestClientException {

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Not sure I understand what this is supposed to be testing? I thought the DownloadSchemaRegistryMojoTest would test functions of the DownloadSchemaRegistryMojo, but this just seems to invoke the client to registry schemas (which also doesn't seem to match the name of the method)?

Same holds for the rest of the tests in this class -- none ever seem to execute the mojo?

this.mojo.subjects = subjectToFile;
this.mojo.execute();

Assert.assertThat(this.mojo.schemaVersions, IsEqual.equalTo(expectedVersions));

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Presumably we never reach this since an exception is expected?

}

@Test(expected = IllegalStateException.class)
public void malformedSchema() throws IOException, MojoFailureException, MojoExecutionException, RestClientException {

This comment has been minimized.

Copy link
@ewencp

ewencp Jul 19, 2016

Member

Seems like the malformed and missing schema tests could probably just be handled by a test of SchemaRegistryMojo since it is shared functionality in loadSchemas.

jcustenborder added some commits Jul 20, 2016

Suggestions from @ewencp. Changed to require subjects for download in…
…stead of downloading everything. Collapsed error handling during download to a single try/catch. Changed register to track the number of exceptions and error out if exceptions were encountered. Fixed copyright date. Consolidated duplicated test code to a shared class. Moved documentation from README.md to docs/
Moved the docs over to RST from markdown. @ewencp can you give me any…
… pointers on adding it to the index?
@ewencp

This comment has been minimized.

Copy link
Member

ewencp commented Jul 26, 2016

@jcustenborder jcustenborder merged commit 83d97be into confluentinc:master Jul 28, 2016

1 check passed

default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.