quarkiverse / quarkus-artemis

Quarkus Artemis extensions
Apache License 2.0
12 stars 13 forks source link

Question: Release notes #90

Closed timonzi closed 1 year ago

timonzi commented 1 year ago

Hello,

I don't find any documentation about changes in the releases. Are there any release notes? For us it is important to know for example when the Artemis version was changed (which of course can be found in the pom.xml) and of course when there are breaking changes. For example as far as I understood the release version 2.0.0 was created, because of changes in the config. But what are those changes? After I switched to the latest version, our tests no longer work. Now I wonder if there is a change I could/should have noticed via release notes.

turing85 commented 1 year ago

Hi! we have currently no release notes.

To you first question: The version of artemis was bumped from 2.25.0 to 2.26.0 on 2022-09-27 and released with release 1.3.0.

As for the config-changes: we aligned the configuration options with the ones of datasources. This was done during the ingetration of named configurations, and released with version 2.0.0. Those are the changes:

So the main breaking change here is that ...xa.enabled is no longer valid, and should now be written as ...xa-enabled

turing85 commented 1 year ago

Update: I have created some documentation improvements (#92), they are up for review. As soon as they are merged, we'll write notes for release 2.0.0. Sorry for the inconvenience. We will take care of doing things "in the right order" in future releases.

turing85 commented 1 year ago

Update 2: In the meantime, here are the release notes for 1.3.0.

middagj commented 1 year ago

After I switched to the latest version, our tests no longer work. Now I wonder if there is a change I could/should have noticed via release notes.

@timonzi On a separate topic did you find out why your tests were no longer working? Perhaps we overlooked a use case, interested to know about it.

turing85 commented 1 year ago

Release notes for 2.0.0. The documentation site should update within the next 6 hours.

zhfeng commented 1 year ago

@timonzi if your tests depend on the default settings. Please try to add quarkus.artemis.enabled=true.

timonzi commented 1 year ago

Sorry for the late reply, I was on vacation.

First of all, thank you very much for the release notes! I think they can/will be a great help, especially in case of breaking changes.

Regarding my failed tests: I didn't have time to investigate the issues. I will catch up today and give you feedback.

Edit: One hint regarding the release notes for 2.0.0:

Changed quarkus version from 2.13.0.FINAL to 2.13.2.FINAL

As far as I can see the used Quarkus version is 2.13.3.Final.

timonzi commented 1 year ago

Not sure at this moment, but I think the problem is that the property quarkus.artemis.url seems to be fixed at build time in the new version (2.0.0). But the documentation does not show it as such.

At least I get the following warning:

2022-11-02 10:10:40.251 WARN  [io.quarkus.runtime.configuration.ConfigRecorder] (main) Build time property cannot be changed at runtime:
 - quarkus.artemis.url is set to 'tcp://localhost:49193' but it is build time fixed to 'tcp://127.0.0.1:61616'. Did you change the property quarkus.artemis.url after building the application?

We use an own Artemis test resource (container), where we map the automatically chosen port to the property (including the host of course). When I start a container with the port defined in the application.properties (61616), the tests work again (at least some of them, as I probably cannot use the same broker instance for all tests).

zhfeng commented 1 year ago

It's weird. I think quarkus.artemis.url can be override at runtime.

timonzi commented 1 year ago

It's weird. I think quarkus.artemis.url can be override at runtime.

This was the case until we switched to release 2.0.0 (for testing purpose). Maybe it is somehow related to https://github.com/quarkiverse/quarkus-artemis/issues/36 ?

zhfeng commented 1 year ago

@timonzi is this happening in JVM or Native mode? @turing85 can you check it?

timonzi commented 1 year ago

It happens in JVM mode

turing85 commented 1 year ago

This could be a side-effect due to the fact how we determine the connections at compile-time (i.e. the ShadowRunTimeConfig). If it is, then the warning can be ignored. I'll double-check and report back.

turing85 commented 1 year ago

I cannot reproduce the behaviour. My setup: - in application.properties: quarkus.artemis.url=${ARTEMIS_URL:tcp://dummy:61616} - start artemis throuhg docker-compose, expose it on localhost:61616 - export ARTEMIS_URL=tcp://localhost:61616, java -jar quarkus-run.jar - access localhost:8080/q/health, all connections are up

@timonzi Could you provide a reproducer?

timonzi commented 1 year ago

@timonzi Could you provide a reproducer?

I will try to provide an example tomorrow.

turing85 commented 1 year ago

On a related note: @zhfeng do you remember what the initial decidion was to not start devservices if a url is present?

turing85 commented 1 year ago

@timonzi correction: I can reproduce the problem. And yes, it is related to the ShadowRunTimeConfig. Baseline is: If we set a value, it is fixed at compile-time. If we just set a variable (like quarkus.artemis.url=${ARTEMIS_URL} and override it at compile-time, it works. We are working on a solution.

zhfeng commented 1 year ago

@turing85 did you find a way to sort it out? I just add two comments on your above commit.

turing85 commented 1 year ago

@zhfeng Yes, I think so. I also answered your comments. Those are the shadows of the ArtemisRuntimeConfigs, therefore Shadow*Runitme*Config

turing85 commented 1 year ago

@timonzi I released version 2.0.1.CR1. This version will still produce the warning, however the property should work as expected now. We will have a section in the documetation about that.

zhfeng commented 1 year ago

Did you check it with native mode?

turing85 commented 1 year ago

@zhfeng Yes. And we have new dedicated tests for this case (here and here). The tests produce the warnings reported by @timonzi, but the semanitc is correct. The tests also run in native mode.

zhfeng commented 1 year ago

Hi @turing85 I just take a deep look at your changes. So

I wonder if this property quarkus.artemis.url is confilict?

zhfeng commented 1 year ago

It confuse me.

turing85 commented 1 year ago

@zhfeng Yes, they are "in conflict", but they have to be. I use the ShadowRuntimeConfigs to figure out what configurations are present (in order to iterate them and create the ConnectionFactorys). I had a discussion with David Llody about that. The error we had was that some classes were shared before (i.e. ShadowRuntimeConfigs extended ArtemisRuntimeConfigs). This lead - as far as I can see - to the issue.

So the gist of it is: we do not have any known, fix value at compile-time to "detect" configurations. Thus, I created the ShadowRuntimeConfigsand ShadowRuntimeConfig to allow me access to the configuration (I read only keys, no value, and I need to know whether "something is present"). This allows us to determine the connection names at compile-time in order to generate the beans. Keep in mind that we have to call the corresponding recorder once per bean we want to create.

turing85 commented 1 year ago

@zhfeng If you are interested: I opened a feature request to read configuration keys at compile-time, but it was closed as "not planned".

timonzi commented 1 year ago

@timonzi I released version 2.0.1.CR1. this one will still produce the warning, however the property should work as expected now. We will have a section in the documetation about that.

Using the new version the tests run again successfully. As you wrote, the warning is still present.

turing85 commented 1 year ago

@timonzi good to hear! I will prepare the 2.0.1 release.

zhfeng commented 1 year ago

Thanks a lot @turing85 and now it is much clearer. I can understand the motivation to have ShadowRuntimeConfigs at build time. Good job! Is there any chance to improve it on Quarkus side as David mentioned in the discussion? I think it is worthy to reopen https://github.com/quarkusio/quarkus/issues/28415

middagj commented 1 year ago

If we fix it in Quarkus we still have the use the old way if we want to support older versions of Quarkus.

What if we make enabled for all named configurations mandatory and use the same approach as DataSource? I don't like that we now instruct users to ignore warnings.

turing85 commented 1 year ago

Thanks a lot @turing85 and now it is much clearer. I can understand the motivation to have ShadowRuntimeConfigs at build time. Good job! Is there any chance to improve it on Quarkus side as David mentioned in the discussion? I think it is worthy to reopen quarkusio/quarkus#28415

I wrote a mail to quarkus-dev about this. For now, I think, we are stuck with the workaround.

turing85 commented 1 year ago

If we fix it in Quarkus we still have the use the old way if we want to support older versions of Quarkus.

What if we make enabled for all named configurations mandatory and use the same approach as DataSource? I don't like that we now instruct users to ignore warnings.

I think that this behaviour is not very user-friendly. Users would have to explicitly set enabled on all configurations, even if other properties are already set.

middagj commented 1 year ago

True, let's keep it for now since it is working anyways. Hopefully we can fix it in Quarkus.

turing85 commented 1 year ago

FTR: a discussion on enabling access to runtime-configs at build time in zulip

turing85 commented 1 year ago

Release 2.0.1 is on its way. Expect it to be available within the next 4-6 hours.