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

Collect classes dirs from all the modules in RuntimeUpdatesProcessor #38939

Conversation

aloubyansky
Copy link
Member

This change makes sure all the classes directories from all the modules are processed by the Vert.X HTTP StaticResourcesHotReplacementSetup.

@ia3andy
Copy link
Contributor

ia3andy commented Feb 21, 2024

@aloubyansky could you also add a new getOutputResourceDir (and replace getResourceDir by it). Currently it returns the src/main/resources instead of the output one. I've checked it's available in the data.

@ia3andy
Copy link
Contributor

ia3andy commented Feb 21, 2024

Thanks!

@aloubyansky
Copy link
Member Author

It does that on purpose though.

@quarkus-bot

This comment has been minimized.

@ia3andy
Copy link
Contributor

ia3andy commented Feb 22, 2024

It does that on purpose though.

does it? the target seems to be more logical no?

@aloubyansky
Copy link
Member Author

In this case we'll be checking for changes in the resources sources dir, since they don't automatically get copied to their output target dirs, which in case of Maven would typically be the classes directory.

@ia3andy
Copy link
Contributor

ia3andy commented Feb 22, 2024

@aloubyansky so maybe we should add both, with the output resource in priority. Also I think we should make sure this list has not duplicates, because we create one StaticHandler per item and that could have a cost.

@aloubyansky
Copy link
Member Author

Is there an actual issue it would fix?

@ia3andy
Copy link
Contributor

ia3andy commented Feb 22, 2024

Is there an actual issue it would fix?

Yes if you have generated resources they are not using the right static handler currently:
quarkiverse/quarkus-web-bundler#171

It would solve only part of the issue, because there is also an issue due to the fact that if there is no META-INF/resources it's will not add it, but I didn't find yet a solution to that problem.

@ia3andy
Copy link
Contributor

ia3andy commented Feb 22, 2024

@ia3andy
Copy link
Contributor

ia3andy commented Feb 22, 2024

@aloubyansky just got an idea, why don't we:

  • add all potential distinct directories in the HotRe..Setup (even without META-INF/resources)
  • in the recorder, we don't add the handle if the META-INF/resources is not a directory

This would allow generated content to be here and this directory won't have to be created before. We still need to add the output resource dir (because in gradle it's different from the target/classes)

Back to PTOs :)

@aloubyansky
Copy link
Member Author

@ia3andy let's discuss it when you are back

@ia3andy
Copy link
Contributor

ia3andy commented Mar 27, 2024

I've created #39735 which has a better solution IMO

@gsmet
Copy link
Member

gsmet commented Jun 7, 2024

Can this be closed or not?

@aloubyansky aloubyansky closed this Jun 7, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Jun 7, 2024
@ia3andy
Copy link
Contributor

ia3andy commented Jun 8, 2024

@aloubyansky I think it still make sense to merge that one, it's better than before

@aloubyansky aloubyansky reopened this Jun 8, 2024
@quarkus-bot quarkus-bot bot removed the triage/invalid This doesn't seem right label Jun 8, 2024
@aloubyansky aloubyansky force-pushed the all-classes-dirs-in-updates-processor branch from df782f1 to 02b1740 Compare June 8, 2024 09:03
@aloubyansky
Copy link
Member Author

aloubyansky commented Jun 8, 2024

Re-opened and rebased @ia3andy

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

LGTM

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 8, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 02b1740.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✔️ MicroProfile TCKs Tests Failures Logs Raw logs 🚧

Full information is available in the Build summary check run.

Failures

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/microprofile-opentelemetry 

📦 tcks/microprofile-opentelemetry

org.eclipse.microprofile.telemetry.tracing.tck.async.MpRestClientAsyncTest.testIntegrationWithMpRestClient - History - More details - Source on GitHub

java.util.concurrent.RejectedExecutionException: event executor terminated
	at io.netty.util.concurrent.SingleThreadEventExecutor.reject(SingleThreadEventExecutor.java:934)
	at io.netty.util.concurrent.SingleThreadEventExecutor.offerTask(SingleThreadEventExecutor.java:351)
	at io.netty.util.concurrent.SingleThreadEventExecutor.addTask(SingleThreadEventExecutor.java:344)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:836)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute0(SingleThreadEventExecutor.java:827)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:817)
	at io.vertx.core.impl.EventLoopExecutor.execute(EventLoopExecutor.java:35)

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/grpc/deployment

io.quarkus.grpc.server.devmode.GrpcDevModeTest.testEchoStreamReload - History

  • Expecting <CompletableFuture[Failed with the following stack trace: io.grpc.StatusRuntimeException: UNKNOWN: io.smallrye.mutiny.subscription.BackPressureFailure - Could not emit tick 12 due to lack of requests at io.grpc.Status.asRuntimeException(Status.java:533) - java.lang.AssertionError
java.lang.AssertionError: 

Expecting
  <CompletableFuture[Failed with the following stack trace:
io.grpc.StatusRuntimeException: UNKNOWN: io.smallrye.mutiny.subscription.BackPressureFailure - Could not emit tick 12 due to lack of requests
	at io.grpc.Status.asRuntimeException(Status.java:533)
	at io.grpc.stub.ClientCalls$StreamObserverToCallListenerAdapter.onClose(ClientCalls.java:481)
	at io.grpc.internal.DelayedClientCall$DelayedListener$3.run(DelayedClientCall.java:489)

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@aloubyansky aloubyansky merged commit 9222a8a into quarkusio:main Jun 8, 2024
53 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants