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

Update to Apicurio Registry 2.1.1.Final and use Vert.x HTTP client #20495

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

carlesarnal
Copy link
Contributor

We (the Apicurio Team) released a new version of Registry recently. This PR updates the usage of Registry to the new version.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/documentation labels Oct 1, 2021
@carlesarnal
Copy link
Contributor Author

cc @Ladicek

@geoand geoand requested a review from Ladicek October 1, 2021 11:48
@Ladicek
Copy link
Contributor

Ladicek commented Oct 1, 2021

Ah does this have the new HTTP client SPI? I will dig out the commit I have to use that. That can be a separate PR I believe.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 1, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building bc29317

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build Build Failures Logs Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/apicurio-registry-avro/runtime 
! Skipped: devtools/bom-descriptor-json docs extensions/apicurio-registry-avro/deployment and 7 more

📦 extensions/apicurio-registry-avro/runtime

Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M3:enforce (enforce) on project quarkus-apicurio-registry-avro: Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed.

@carlesarnal
Copy link
Contributor Author

Ah does this have the new HTTP client SPI? I will dig out the commit I have to use that. That can be a separate PR I believe.

Yes. This also removes the need for Keycloak libraries. I can do the changes if you want, just give me the pointers to the code :).

@carlesarnal
Copy link
Contributor Author

Ok, I've figured out what's going on with the failed build and I'm working to get it fixed. I'll ping here once is resolved.

@carlesarnal
Copy link
Contributor Author

@Ladicek do you mind approving the workflow run? :)

@carlesarnal
Copy link
Contributor Author

@Ladicek there's one failing test. I think it has something to do with the SPI not working in native mode. Anyway, we need to force it to use the Vert.x client, so I can leave it to you or I can do it if you give me some hints/pointers about where to do that and I can do it, whatever you prefer, thanks.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 7, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 431de09

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
JVM Tests - JDK 17 Build Failures Logs Raw logs
Native Tests - Messaging1 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/oidc 

📦 integration-tests/oidc

io.quarkus.it.keycloak.WebsocketOidcTestCase.websocketTest line 49 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <hello [email protected]> but was: <null>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/oidc 

📦 integration-tests/oidc

io.quarkus.it.keycloak.WebsocketOidcTestCase.websocketTest line 49 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <hello [email protected]> but was: <null>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)

⚙️ Native Tests - Messaging1 #

- Failing: integration-tests/kafka-avro-apicurio2 

📦 integration-tests/kafka-avro-apicurio2

io.quarkus.it.kafka.KafkaAvroIT.testApicurioAvroProducer - More details - Source on GitHub

java.util.NoSuchElementException
	at org.apache.kafka.common.utils.AbstractIterator.next(AbstractIterator.java:52)
	at io.quarkus.it.kafka.KafkaAvroTest.testAvroProducer(KafkaAvroTest.java:68)

io.quarkus.it.kafka.KafkaAvroIT.testApicurioAvroConsumer - More details - Source on GitHub

com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: 
Unrecognized field "details" (class io.quarkus.it.kafka.avro.Pet), not marked as ignorable (2 known properties: "name", "color"])
 at [Source: (String)"{"details":"Error id 56c047ef-dc6b-4ef3-9a9a-e07d84387919-2","stack":""}"; line: 1, column: 13] (through reference chain: io.quarkus.it.kafka.avro.Pet["details"])

@@ -22,7 +22,7 @@
-->

<properties>
<apicurio.version>2.0.1.Final</apicurio.version>
<apicurio.version>2.1.1.Final</apicurio.version>
Copy link
Member

Choose a reason for hiding this comment

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

Can't apicurio-registry.version be used in place of this property?

@@ -29,7 +29,7 @@ public static String getApicurioSchemaRegistryUrl() {
@Override
public Map<String, String> start() {
kafka.start();
registry = new GenericContainer<>("apicurio/apicurio-registry-mem:2.0.1.Final")
registry = new GenericContainer<>("apicurio/apicurio-registry-mem:2.1.1.Final")
Copy link
Member

Choose a reason for hiding this comment

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

This version should also be read from a property (added to the surefire plugin config).

@Ladicek
Copy link
Contributor

Ladicek commented Oct 11, 2021

@carlesarnal I see you tried to integrate the Vert.x client. That is a bit more complex, I have a branch somewhere, but the most important question is: what version of Vert.x is Apicurio Registry 2.1 compiled against? In 2.0, it used to be Vert.x 3.9, so I had to copy the Vert.x client to Quarkus (that's sitting on that branch I have), but if 2.1 is compiled against Vert.x 4, then I could remove that.

In any case, I think I'd just bump Apicurio Registry to 2.1 in this PR, and moved to using the Vert.x client in another PR. WDYT?

EDIT: if that is not possible, maybe I can add a commit to your branch?

@carlesarnal
Copy link
Contributor Author

carlesarnal commented Oct 13, 2021

Hi @Ladicek, sorry for the delay, I was on PTO. I only added the required dependencies, I was expecting this to be more complex than just that :). Apicurio Registry 2.1.1.Final is working with Vert.x 4.x and Quarkus 2.2.3.Final, so you should be able to remove the client there. I think it's better if you add the commit to this branch directly.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 13, 2021

OK, I'll try to take a look, but I'm rather busy with CDI Lite these days. I should get to it next week. For the record, here's the commit I did back when we discussed this: Ladicek@29c4624 The most interesting change is the one in ApicurioRegistryAvroProcessor. The copy of the Apicurio SPI implementation should be removed.

@carlesarnal
Copy link
Contributor Author

I've added a commit with something similar to what you did in yours but using the client on our side. Just check it when you have some time.

@Ladicek Ladicek force-pushed the apicurio-registry-2.1.0 branch from 2884de3 to 49e56f1 Compare October 25, 2021 11:13
@Ladicek Ladicek changed the title Update to Apicurio Registry 2.1.0.Final Update to Apicurio Registry 2.1.1.Final and use Vert.x HTTP client Oct 25, 2021
@Ladicek
Copy link
Contributor

Ladicek commented Oct 25, 2021

@carlesarnal I've rebased and squashed your branch, plus made a few tiny changes (excluded the JDK client, removed a dependency on the Vert.x client library from the -deployment artifact, because it's a duplicate, the deployment artifact has a dependency on the runtime artifact). Can you please check if current state of this PR makes sense to you? It does to me :-)

@@ -201,7 +201,8 @@
<log4j2-jboss-logmanager.version>1.0.0.Final</log4j2-jboss-logmanager.version>
<log4j-jboss-logmanager.version>1.2.2.Final</log4j-jboss-logmanager.version>
<avro.version>1.10.2</avro.version>
<apicurio-registry.version>2.0.1.Final</apicurio-registry.version>
<apicurio-registry.version>2.1.1.Final</apicurio-registry.version>
<apicurio-common-rest-client.version>0.0.9.Final</apicurio-common-rest-client.version> <!-- must be the version Apicurio Registry uses -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, nice comment.

@@ -17,6 +17,16 @@
<dependency>
<groupId>io.apicurio</groupId>
<artifactId>apicurio-registry-serdes-avro-serde</artifactId>
<exclusions>
<exclusion>
<groupId>io.apicurio</groupId>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@carlesarnal
Copy link
Contributor Author

@carlesarnal I've rebased and squashed your branch, plus made a few tiny changes (excluded the JDK client, removed a dependency on the Vert.x client library from the -deployment artifact, because it's a duplicate, the deployment artifact has a dependency on the runtime artifact). Can you please check if current state of this PR makes sense to you? It does to me :-)

It does, yes. Let's see what CI says. Thanks a lot, @Ladicek.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 25, 2021

Also FYI @cescoffier, it finally happens :-)

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 25, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 49e56f1

Status Name Step Failures Logs Raw logs
MicroProfile TCKs Tests Verify Failures Logs Raw logs
Native Tests - Messaging1 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/microprofile-fault-tolerance 

📦 tcks/microprofile-fault-tolerance

org.eclipse.microprofile.fault.tolerance.tck.TimeoutUninterruptableTest.testTimeoutAsyncBulkhead line 190 - More details - Source on GitHub

java.lang.AssertionError: Unexpected exception thrown from Future
	at org.testng.Assert.fail(Assert.java:85)
	at org.eclipse.microprofile.fault.tolerance.tck.util.Exceptions.expect(Exceptions.java:98)

⚙️ Native Tests - Messaging1 #

- Failing: integration-tests/kafka-avro-apicurio2 

📦 integration-tests/kafka-avro-apicurio2

io.quarkus.it.kafka.KafkaAvroIT.testApicurioAvroProducer - More details - Source on GitHub

org.apache.kafka.common.KafkaException: Failed to construct kafka consumer
	at org.apache.kafka.clients.consumer.KafkaConsumer.<init>(KafkaConsumer.java:823)
	at org.apache.kafka.clients.consumer.KafkaConsumer.<init>(KafkaConsumer.java:665)

io.quarkus.it.kafka.KafkaAvroIT.testApicurioAvroConsumer - More details - Source on GitHub

org.apache.kafka.common.KafkaException: Failed to construct kafka producer
	at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:440)
	at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:291)

@Ladicek
Copy link
Contributor

Ladicek commented Oct 25, 2021

TimeoutUninterruptableTest is flaky (though rare), but KafkaAvroIT is most likely relevant.

@carlesarnal
Copy link
Contributor Author

TimeoutUninterruptableTest is flaky (though rare), but KafkaAvroIT is most likely relevant.

Yes, it looks like the AtomicReference has not been properly set. Maybe due to the native nature of the test? I'm not sure tbh. We're using the factory and the reference in our testing with no issues but they're not native.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 26, 2021

Found the problem. The thing is that @QuarkusTest runs in the very Quarkus application it is supposed to test (the test class is even considered a bean and instantiated via CDI!), while @QuarkusIntegrationTest runs in a separate JVM. Unfortunately, the KafkaAvroTest (from which KafkaAvroIT inherits) also uses the Kafka client (and hence also Apicurio Registry client) -- so we have to setup the Apicurio Registry client factory explicitly for the test JVM (because we exclude the JDK client, which would otherwise be set up automatically).

@Ladicek Ladicek force-pushed the apicurio-registry-2.1.0 branch from 49e56f1 to 3d850f5 Compare October 26, 2021 15:22
@Ladicek
Copy link
Contributor

Ladicek commented Oct 26, 2021

Pushed a fix.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 26, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 3d850f5

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build ⚠️ Check → Logs Raw logs
Native Tests - Messaging2 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

⚠️ Errors occurred while downloading the build reports. This report is incomplete.

Failures

⚙️ Native Tests - Messaging2 #

- Failing: integration-tests/reactive-messaging-rabbitmq 

📦 integration-tests/reactive-messaging-rabbitmq

io.quarkus.it.rabbitmq.RabbitMQConnectorIT.test - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a io.quarkus.it.rabbitmq.RabbitMQConnectorTest expected: <6> but was: <0> within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)

@Ladicek
Copy link
Contributor

Ladicek commented Oct 27, 2021

I can't see how this test failure could be relevant. @cescoffier @ozangunalp do you perhaps know if RabbitMQConnectorIT is flaky or not?

@ozangunalp
Copy link
Contributor

@Ladicek it is a recently added extension. I see in #20659 that it was flaky to begin with. I'll take a look to fix it but I don't think that it is blocker for this PR.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 27, 2021

Thanks a lot!

@carlesarnal
Copy link
Contributor Author

@Ladicek any update on this? I would like to get this into the next release, if possible.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 3, 2021

Ouch, I thought it's already merged. Whoops! Let me see if I can resolve the conflict real quick.

@Ladicek Ladicek force-pushed the apicurio-registry-2.1.0 branch from 3d850f5 to a23c7b9 Compare November 3, 2021 14:58
@Ladicek
Copy link
Contributor

Ladicek commented Nov 3, 2021

OK, done. Hopefully CI will end up green :-)

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 3, 2021

Failing Jobs - Building a23c7b9

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
Native Tests - Windows - hibernate-validator Build Failures Logs Raw logs

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/hibernate-orm/deployment 
! Skipped: docs extensions/hibernate-envers/deployment extensions/hibernate-reactive/deployment and 85 more

📦 extensions/hibernate-orm/deployment

Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test (default-test) on project quarkus-hibernate-orm-deployment: There was a timeout in the fork


⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator 

📦 integration-tests/hibernate-validator

Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

@Ladicek
Copy link
Contributor

Ladicek commented Nov 4, 2021

I have no idea why these tests fail or if they're flaky :-/

@Ladicek
Copy link
Contributor

Ladicek commented Nov 4, 2021

Apparently these tests are flaky, so this is good to go!

@Ladicek Ladicek merged commit 72c8d42 into quarkusio:main Nov 4, 2021
@quarkus-bot quarkus-bot bot added this to the 2.5 - main milestone Nov 4, 2021
@carlesarnal carlesarnal deleted the apicurio-registry-2.1.0 branch November 4, 2021 11:04
@carlesarnal
Copy link
Contributor Author

Nice, thanks for merging @Ladicek!

@Ladicek
Copy link
Contributor

Ladicek commented Nov 4, 2021

Thanks for your patience! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants