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

AVRO-1524: avro-maven-plugin be able to generate schema from IDL #2568

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

databius
Copy link

@databius databius commented Oct 25, 2023

What is the purpose of the change

Implement AVRO-1524: update avro-maven-plugin to generate schema from IDL

Verifying this change

This change consists of several parts:

  • The new avro-maven-plugin configurations, plus tests
  • Documentation (1 file)

All changes have tests.

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@github-actions github-actions bot added the Java Pull Requests for Java binding label Oct 25, 2023
@databius databius changed the title AVRO-1524: avro-maven-plugin be able to generate schema from IDL draft: AVRO-1524: avro-maven-plugin be able to generate schema from IDL Oct 25, 2023
@databius databius marked this pull request as draft October 25, 2023 06:19
@databius databius marked this pull request as ready for review October 25, 2023 07:08
@databius databius changed the title draft: AVRO-1524: avro-maven-plugin be able to generate schema from IDL AVRO-1524: avro-maven-plugin be able to generate schema from IDL Oct 25, 2023
@opwvhk
Copy link
Contributor

opwvhk commented Oct 25, 2023

Thank you for implementing this!

The idea is good, but plugin mojos should have one task.

Can you please create separate mojos for each of the two new actions?

@databius
Copy link
Author

Thank you for implementing this!

The idea is good, but plugin mojos should have one task.

Can you please create separate mojos for each of the two new actions?

Agree that plugin mojos should have single responsibility. But the mojo names confused me.

Since #1589, we have:

  • IDLMojo - @goal idl - Generate Java classes and interfaces from AvroIDL files (.avdl)
  • IDLProtocolMojo - @goal idl-protocol - The old version of IDLMojo

IDLMojo looks like a generic IDL tool so I added the new actions to it. I think it would be clearer if it had a more meaningful name likes IDL2JavaMojo

And the new Mojos might be:

  • IDL2ProtocolMojo - @goal idl-2-protocol - Generate Json protocol from Avro IDL files (.avdl)
  • IDL2SchemaMojo - @goal idl-2-schema - Generate Json Schema from Avro IDL files (.avdl)
    Are they OK?

@opwvhk
Copy link
Contributor

opwvhk commented Oct 30, 2023

Since #1589, we have:

  • IDLMojo - @goal idl - Generate Java classes and interfaces from AvroIDL files (.avdl)
  • IDLProtocolMojo - @goal idl-protocol - The old version of IDLMojo

IDLMojo looks like a generic IDL tool so I added the new actions to it. I think it would be clearer if it had a more meaningful name likes IDL2JavaMojo

And the new Mojos might be:

  • IDL2ProtocolMojo - @goal idl-2-protocol - Generate Json protocol from Avro IDL files (.avdl)
  • IDL2SchemaMojo - @goal idl-2-schema - Generate Json Schema from Avro IDL files (.avdl)
    Are they OK?

Looks good, but I'm not a fan of the -2- infix; it looks like a separate number. I prefer this without the dashes, but -to- as infix works as well.

Given that #1589 hasn't been released yet, what do you think of the following:

  • IDL2JavaMojo - @goal idl2java - Generate Java classes and interfaces from AvroIDL files (.avdl)
  • IDLProtocolMojo - @goal idl-protocol - The old version of IDLMojo
  • IDL2ProtocolMojo - @goal idl2protocol - Generate Json protocol from Avro IDL files (.avdl)
  • IDL2SchemaMojo - @goal idl2schema - Generate Json Schema from Avro IDL files (.avdl)

@databius
Copy link
Author

databius commented Nov 2, 2023

Thanks for your comment. Just pushed some updates. Please help to take a look if you have time @opwvhk

@databius
Copy link
Author

Thank @opwvhk for your approval. May I ask how to merge it to main?

@westsail42
Copy link

Any chance of getting this feature into a release soon?

@@ -66,14 +66,18 @@ A Maven plugin is also provided to compile .avdl files. To use it, add something
<executions>
<execution>
<goals>
<goal>idl</goal>
<goal>idl2java</goal>
Copy link
Member

Choose a reason for hiding this comment

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

I am not a user of the Java SDK but this looks like a breaking change.
Old applications that use <goal>idl</goal> will break ?! I see no goal idl in the list of available goals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point.. I should have noticed that.

That said, there's also work underway (for the Java SDK) to parse IDL schemas using the same code as the standard JSON schemas.

Going forward with this PR, we should thus recreate the old goal. Preferably as near-empty subclass of the new mojo (logging a warning directing users to the new name). As the parameters have not changed, this will ensure backwards compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Pull Requests for Java binding website
Projects
None yet
4 participants