Add support for Confluent Schema Registry + restructure schema registry extensions #24526

alesj · 2022-03-24T11:58:17Z

No description provided.

cescoffier

Documentation needs to be updated too:

Avro doc
Apicurio dev service doc

cescoffier · 2022-03-28T12:31:22Z

...eployment/src/main/java/io/quarkus/apicurio/registry/avro/ApicurioRegistryAvroProcessor.java

+
+        // Confluent API compatibility support -- TODO
+
+        reflectiveClass.produce(new ReflectiveClassBuildItem(true, true, false,


wondering if we need to add a flag to enable/disable this.

Typically, guarded with isCcompat

Is this actually even needed?
Since the IT (native) test worked before I added this ...

I just added this not to forget, and was hoping IT (native) test would expose all the missing stuff ...

Actually, that's a good question. What happens if we include in the native image classes that are not available in the classpath. Would that fail? I don't remember... (that's what I'm trying to prevent).

Looking at KafkaProcessor, all these registrations are guarded by try/catch on the CNFE, with a previous try to load a class from the lib it's trying to support.
So maybe this try/catch is missing / needed here as well?

cescoffier · 2022-03-28T12:32:26Z

...nt/src/main/java/io/quarkus/apicurio/registry/avro/DevServicesApicurioRegistryProcessor.java

@@ -118,7 +120,21 @@ public void run() {
    }

    private String getRegistryUrlConfig(String baseUrl) {
-        return baseUrl + "/apis/registry/v2";
+        if (isCcompat()) {


I know that ccompat is what is used by apicurio, but maybe let's say: isUsingConfluentCompatibility() (I've found ccompat to be confusing).

Yeah, I remember we used "Confluent" before in the Apicurio, but then rather went with ccompat.
I'm OK with your isUsingConfluentCompatibility.

Ladicek · 2022-03-28T12:55:52Z

IMHO, adding support for Confluent Avro serde to the Apicurio Registry Avro serde extension is a wrong way to do this. That way, you will always have both serdes on the classpath (unless the user explicitly excludes the Apicurio Registry Avro serde from the extension dependency, which is hardly idiomatic, because support of that serde library is the main purpose of the extension).

What we could/should do is:

Separate the Apicurio Registry dev service from the Avro extension into something like apicurio-registry-base. That's what we always intended to do, except we thought that would be when we add support for Apicurio Registry Protobuf serde or something like that :-)
Add an extension for Confluent. That would be somewhat confusing, because that extension can't really bring in the serde library, as it's not in Maven Central, but at least it makes the intent very clear and can even validate that the serde is present. This extension could then use the Apicurio Registry dev service (and configure the ccompat endpoint), or it could use Karapace perhaps, or do something completely different. I guess using the Apicurio Registry makes most sense.

alesj · 2022-03-28T13:07:34Z

That way, you will always have both serdes on the classpath (unless the user explicitly excludes the Apicurio Registry Avro serde from the extension dependency, which is hardly idiomatic, because support of that serde library is the main purpose of the extension).

Ah, true ... yeah, then we need a proper split ...

That would be somewhat confusing, because that extension can't really bring in the serde library, as it's not in Maven Central

Why is this - not in Maven Central - a problem?

Ladicek · 2022-03-28T13:15:37Z

That would be somewhat confusing, because that extension can't really bring in the serde library, as it's not in Maven Central

Why is this - not in Maven Central - a problem?

I'm not entirely sure if it is possible for extensions to bring dependencies from another Maven repo, perhaps it is. I'm pretty sure we don't really want them to, because of effects on supply chain security and build stability (and downstreams who rebuild Quarkus would also not be particularly pleased I'm sure). But if @gsmet would be fine with it, so would I :-)

alesj · 2022-03-28T14:24:02Z

unless the user explicitly excludes the Apicurio Registry Avro serde from the extension dependency, which is hardly idiomatic, because support of that serde library is the main purpose of the extension

@Ladicek you mean that reflection support in ApicurioRegistryAvroProcessor?
As otherwise I fail to see where some extra value wrt serdes is really added ...

Otoh, wrt reflection support in ApicurioRegistryAvroProcessor ... I can see KafkaAvroIT passing for Confluent serde classes,
yet they are not registered in ApicurioRegistryAvroProcessor ... or am I missing something obvious?

Ladicek · 2022-03-28T14:30:49Z

Ah ah, now I remember -- we actually register Confluent classes for reflection elsewhere, see KafkaProcessor in the kafka-client extension.

ozangunalp · 2022-03-28T14:50:23Z

@Ladicek was there a specific reason for the apicurio-registry-avro extension to not to have kafka-client dependency ?

Ladicek · 2022-03-28T14:53:46Z

I can't remember one, you just don't use apicurio-registry-avro without also using another extension that transitively brings kafka-client.

ozangunalp · 2022-03-28T15:01:57Z

Separate the Apicurio Registry dev service from the Avro extension into something like apicurio-registry-base. That's what we always intended to do, except we thought that would be when we add support for Apicurio Registry Protobuf serde or something like that :-)

BTW this change should also work for confluent kafka protobuf serializer when we add support for protobuf serde. So I think this would be included in apicurio-registry-base.

alesj · 2022-04-07T18:20:09Z

OK, tests passed ... feedback on the restructure (before I dig into protobuf / json) ?

ozangunalp · 2022-04-08T09:09:45Z

Yes the restructuring looks good to me.

Ladicek · 2022-04-11T15:43:04Z

Sorry it took me so long to get to this. The PR is rather complex, so I had to look at it in my IDE.

The general idea of the restructuring sounds good to me. As in, to have something like this:

schema registry parent POM
- dev service
  - deployment
  - runtime
- Apicurio parent POM
  - common
    - deployment
    - runtime
  - avro
    - deployment
    - runtime
- Confluent parent POM
  - common
    - deployment
    - runtime
  - avro
    - deployment
    - runtime

The dev service would use Apicurio Registry, but that's an implementation detail. Then, apicurio / common would provide basic support for Apicurio serdes, while apicurio / avro would depend on apicurio / common and add support for Apicurio Avro serde. Same thing for confluent.

However, the present PR uses very confusing naming (apicurio-registry-confluent-what?!). Also, some classes were moved incorrectly in my opinion (the service binding support is present in the dev service? That can't be correct).

I took the liberty to restructure everything once again. The structure I outlined above looks physically like this:

extensions/schema-registry: quarkus-schema-registry-parent
- devservice: quarkus-schema-registry-devservice-parent
  - deployment: quarkus-schema-registry-devservice-deployment
  - runtime: quarkus-schema-registry-devservice
- apicurio: quarkus-apicurio-registry-parent
  - common: quarkus-apicurio-registry-common-parent
    - deployment: quarkus-apicurio-registry-common-deployment
    - runtime: quarkus-apicurio-registry-common
  - avro: quarkus-apicurio-registry-avro-parent
    - deployment: quarkus-apicurio-registry-avro-deployment
    - runtime: quarkus-apicurio-registry-avro
- confluent: quarkus-confluent-registry-parent
  - common: quarkus-confluent-registry-common-parent
    - deployment: quarkus-confluent-registry-common-deployment
    - runtime: quarkus-confluent-registry-common
  - avro: quarkus-confluent-registry-avro-parent
    - deployment: quarkus-confluent-registry-avro-deployment
    - runtime: quarkus-confluent-registry-avro

Here's a branch with 1 more commit that restructures everything once more: https://github.com/Ladicek/quarkus/commits/schema-registry-restructured

And here's a branch where the 2 commits from previous branch are squashed: https://github.com/Ladicek/quarkus/commits/schema-registry-restructured-squashed

One question: the changes in integration-tests/kafka-avro-apicurio2 that move from QuarkusTestResourceLifecycleManager to dev services are intentional, to test the dev service stuff, or the QuarkusTestResourceLifecycleManager approach didn't work anymore?

alesj · 2022-04-14T08:30:23Z

@Ladicek yeah, good call and restructuring on the names

the service binding support is present in the dev service?

Yeah, that one I didn't know where it fits best.
Did you move it appropriately now?

move from QuarkusTestResourceLifecycleManager

There was an issue with enabled/disabled dev service, which I wasn't able to override from test code.
While fixing that, I didn't see a reason why not to use and test dev service at the same time.
So I simply removed it, and handled that AvroCreator accordingly.

I guess if there are no other issues, we're good to merge?

ozangunalp · 2022-04-14T08:57:51Z

Maybe we should consider changing this PRs title and description?

Ladicek · 2022-04-14T09:26:07Z

I moved the service binding thing into apicurio-common, which I think is the right place, but I don't know anything about it. @ozangunalp or @cescoffier, is there a way (I don't dare saying "reasonably simple", though that's what I wish for...) to test the service binding stuff?

Also, I didn't push my changes to this PR. @alesj if you agree with my changes, feel free to update this PR, or I can do that too.

ozangunalp · 2022-04-14T09:55:29Z

@Ladicek I think also that apicurio-common is the right place for the service binding config converter. I honestly don't know how to test it. The whole thing is dependent on the service type you want to bind. The service type "serviceregistry" provides those mapped keys (I don't know by which spec or convention) and it works until someone changes something.

Overall I am thinking that we should not need to do this conversion and service binding resource should also contain the mapping for the application. But I am not that deep in service binding to understand if it is possible.

alesj · 2022-04-14T12:40:02Z

Maybe we should consider changing this PRs title and description?

Sure.
But since @Ladicek is gonna create a new PR anyway, we can just fix it there.

alesj · 2022-04-14T12:40:30Z

or I can do that too.

Yeah, go ahead.
No need to complicate things.

Ladicek · 2022-04-14T12:48:33Z

Pushed and changed PR title.

Ladicek · 2022-04-21T09:13:19Z

Hi @ozangunalp @cescoffier @geoand @iocanel, could you please advise on how to test (manually I guess?) the Apicurio Registry service binding stuff? I don't know who authored that and I have exactly zero experience with service binding, but I figured some of you might know :-)

geoand · 2022-04-21T09:30:13Z

No idea about apicurio, but an integration test for service binding for JDBC can be found at: https://github.com/quarkusio/quarkus/tree/2.8.1.Final/integration-tests/kubernetes-service-binding-jdbc

I assume something similar can be done for apicurio

ozangunalp · 2022-04-21T17:01:11Z

@Ladicek @geoand I don't think that kind of IT will actually test the Apicurio config mapping. It effectively tests that a service binding root directory is transformed to config, but that's it.

It seems like those magical keys in ServiceRegistryBindingConverter are provided by the RHOAS Service Registry Operator. I could find a short documentation here: https://github.com/redhat-developer/app-services-operator/blob/main/docs/service_binding.adoc

I recently did a (manual) test and can confirm that the converter for Apicurio works as intended.
On another note, the ServiceBinding resources we generate in Quarkus did not work OOTB with the RHOAS SBO.
I'll create a PR to give the possibility to generate ServiceBindings that play well with RHOAS SBO.

Ladicek · 2022-04-22T06:47:35Z

In that case, I guess I'll just rebase this PR and we can merge it...

…ry extensions

Ladicek · 2022-04-22T13:40:16Z

@ozangunalp could you please take a look too? Thanks!

ozangunalp · 2022-04-22T13:44:25Z

@Ladicek I am on it!

ozangunalp

LGTM! @Ladicek I also checked that the service binding converter for apicurio still works :)

I believe this was addressed

Ladicek · 2022-04-22T16:10:50Z

Thank you!

I'm a little scared, but the big green button is so tempting...

This comment was marked as resolved.

Sign in to view