search

quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.83k stars 2.7k forks source link

Ensure that all our handlers extend `ExtHandler` #44565

Open dmlloyd opened 3 days ago

dmlloyd commented 3 days ago

Fixes #44540 (in theory)

quarkus-bot[bot] commented 3 days ago

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

gsmet commented 3 days ago

@dmlloyd when you say in theory did you test this particular example?

dmlloyd commented 3 days ago

I tested it obliquely, but I couldn't test it exactly (yet?) due to local configuration problems.

gastaldi commented 3 days ago

@MaxFichtelmann can you build this PR locally and confirm that it fixes your issue before we merge it?

brunobat commented 3 days ago

It makes sense to me even if it doesn't fix the bug.

MaxFichtelmann commented 3 days ago

@MaxFichtelmann can you build this PR locally and confirm that it fixes your issue before we merge it?

Sure, I'll build and Test when I am at the PC again.

quarkus-bot[bot] commented 3 days ago

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 8fe3c761a6d77c96686f7e4ac5492876b3353664.

:white_check_mark: The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

MaxFichtelmann commented 3 days ago

hm, still seeing that behaviour in quarkus dev - actually when running that same project without dev, the issue does not appear, neither with 3.14.3 nor 3.16.3 - so seems to be a devmode-only issue.

dmlloyd commented 2 days ago

I also opened https://github.com/jboss-logging/jboss-logmanager/pull/493 which should address what we're seeing on the stack trace, once that is merged & the component is updated.

gastaldi commented 2 days ago

I'd recommend to wait until https://github.com/jboss-logging/jboss-logmanager/pull/493 is merged and released and updated in this PR before merging

dmlloyd commented 2 days ago

I don't think waiting is necessary, because the two changes are not interdependent.

gsmet commented 2 days ago

@dmlloyd sorry if it's just noise but how will the new patch in JBoss Logmanager interact with this specific piece:

https://github.com/quarkusio/quarkus/commit/7e6c529629ea31e8c193110c8abe31aaa101b597#diff-3027189c5073a902729f033232338719972cd18a3c64018f24b99dd78e1fea6aR441-R447

where we add parameters to something that used to be NO_FORMAT.

gastaldi commented 2 days ago

@dmlloyd I know, but the second change will fix the bug, right?

dmlloyd commented 2 days ago

It might :-)

gastaldi commented 2 days ago

Loving the confidence behind this patch :D

dmlloyd commented 2 days ago

@dmlloyd sorry if it's just noise but how will the new patch in JBoss Logmanager interact with this specific piece:

7e6c529#diff-3027189c5073a902729f033232338719972cd18a3c64018f24b99dd78e1fea6aR441-R447

where we add parameters to something that used to be NO_FORMAT.

The interaction should be fine. The handler for NO_FORMAT here will change it to use message format, with two arguments.

gsmet commented 2 days ago

The interaction should be fine. The handler for NO_FORMAT here will change it to use message format, with two arguments.

And it won't fail if we end up with My message { that used to work in NO_FORMAT but won't work with message format? Or maybe you don't resolve the parameters recursively?

dmlloyd commented 2 days ago

The interaction should be fine. The handler for NO_FORMAT here will change it to use message format, with two arguments.

And it won't fail if we end up with My message { that used to work in NO_FORMAT but won't work with message format? Or maybe you don't resolve the parameters recursively?

The original message is passed as a parameter to a new message, so no further interpolation can take place.

gsmet commented 2 days ago

OK, perfect, thanks for the clarification. I'm still a bit puzzled the OP says it's failing only in dev mode but I wonder if he actually used the newly compiled version in dev mode, I will have to check that.

MaxFichtelmann commented 2 days ago

OK, perfect, thanks for the clarification. I'm still a bit puzzled the OP says it's failing only in dev mode but I wonder if he actually used the newly compiled version in dev mode, I will have to check that.

I am optimistic that I used the updated version, but this is the first time I built quarkus myself, so...