search

eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

Fix MP Config usage in GlobalTagsTest #489

Closed jmartisk closed 4 years ago

jmartisk commented 4 years ago

Fixes #488

jmartisk commented 4 years ago

@dmlloyd please have a look if this makes sense, especially I'd like to know if you expect this to work in Quarkus after the config update :) SmallRye Metrics itself passes the TCK with it.

dmlloyd commented 4 years ago

It should be OK I think. If not we can revisit.

jmartisk commented 4 years ago

@donbourne could you guys give it a go to make sure it doesn't break the TCK for you?

tevans78 commented 4 years ago

Sorry that test is wrong, you can't re-register a config once it has been released. The MP Config spec says this;

A factory method ConfigProviderResolver#releaseConfig(Config config) to release the Config instance. This will unbind the current Config from the application. The ConfigSources that implement the java.io.Closeable interface will be properly destroyed. The Converters that implement the java.io.Closeable interface will be properly destroyed. Any subsequent call to ConfigProvider#getConfig() or ConfigProvider#getConfig(ClassLoader forClassLoader) will result in a new Config instance.

i.e. all the resources associated with the Config will have been closed and in essence the Config itself is closed.

dmlloyd commented 4 years ago

Yeah true, we've been working in the MP config spec to get that fixed (after all, what happens when the same config is registered twice?) but for now you should probably compare that the registered config is not identical to the config that is to be released.

jmartisk commented 4 years ago

I added a commit so that the original config is not re-registered after unregistering the overriding one; instead we expect the MP Config impl will provide a new config for subsequent tests. Does this work better now?

Channyboy commented 4 years ago

This works fine with the Liberty implementation now.

jmartisk commented 4 years ago

Thanks @Channyboy for checking, @donbourne does this look good for merging?

donbourne commented 4 years ago

discussed with @Channyboy ... we're good for merging.

jmartisk commented 4 years ago

Thanks, I will forward-port this to master now