Ouf of date: ConfigValue implementation for dynamic properties #91

struberg · 2017-03-09T23:20:37Z

This is almost the full content of ConfigValue which is up for discussion.
I completely leaves out CDI for now but I will ship this in a separate PR.

If you want to try this out then please feel free to check out https://github.com/struberg/javaConfig/tree/microprofile-impl
This contains a fully working implementation. This is basically the stuff from June 2016 but adopted to the new interface changes.

have fun!

Emily-Jiang

Please don't merge the dynamic changes handling in. I'll do a pr for my solution. We can then compare next week.

Emily-Jiang · 2017-03-09T23:42:32Z

tck/src/main/java/org/eclipse/microprofile/config/tck/ConfigValueTest.java

+
+        Assert.assertEquals(config.access("tck.config.test.javaconfig.configvalue.boolean.oui").as(Boolean.class).get(), Boolean.TRUE);
+        Assert.assertEquals(config.access("tck.config.test.javaconfig.configvalue.boolean.oui_uppercase").as(Boolean.class).get(), Boolean.TRUE);
+        Assert.assertEquals(config.access("tck.config.test.javaconfig.configvalue.boolean.oui_mixedcase").as(Boolean.class).get(), Boolean.TRUE);


I don't really like to force to convert y, ja, j n, oui to true. It is not well known. I will suggestion to convert yes, y, on to true, which should be sufficient.

You don't speak french? :)
Seriously, we have seen this as requests in DeltaSpike. And it doesn't hurt.

Please don't quote DeltaSpike. We are not defining specs around DeltaSpile.

It is fine for DeltaSpike to expand this. -1 on this.

I agree with Emily, the specification should be formal and pick only widely used options. Any implementation can support more options, and if it doesn't a custom converter should solve the problem. Otherwise, we would have to support all language variants - possibly Japanese, since Fujitsu have also joined MP.

Ok for me, we can move this to be an implementation detail.
Just let me know which valid list of 'true' values you like to support out of the box.

Emily-Jiang · 2017-03-09T23:45:05Z

api/src/main/java/org/eclipse/microprofile/config/ConfigValue.java

+    interface ConfigChanged {
+        <T> void onValueChange(String key, T oldValue, T newValue);
+    }
+}


This is not what we discussed ConfigValue. What we discussed is just a proxy. This forces to configure the pulling intervals per property.

No, this is exactly what we discussed originally. Without any optional caching you will trash the config system (as explained on the hangout yesterday).
This is not a feedback from 'working sample' but from years in production:
https://github.com/apache/deltaspike/blob/master/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/config/ConfigResolver.java#L681

But you can also simply not use cacheFor then it will go directly to the underlying Config.
And this features is of course independent per config value. Because for some config values it's fine to reload them every 15 minutes while others must be refreshed all few seconds. This is really highly depending on the business meaning behind each value.

The next task is to show how we can leverage this for CDI injection.

After we decided that we would injection Provider instead of ConfigValue proxy and closing #42, ConfigValue class can be used for a different purpose, as described in this PR. It is now not intended to be injected into CDI beans, but in JavaSE API to allow for programmatic configuration and access to metadata of a single config value. In CDI API, this should be done via annotations on an injection point.

I think that ConfigValue in this PR covers too many things at once. I'd like to split some methods into a separate PR. As a base, I think it is a no-brainer to leave as, get, getOptional, getKey and move other methods into PRs on their own. We can then discuss separately.

The get has been provided by Provider. I don't see any more value added by this class.

@struberg The title of this PR is "ConfigValue implementation for dynamic properties" and regarding this, it doesn't offer any more value than a Provider we already accepted to be used.

The other features are not related much to the title of this PR and I would separate them to other PRs, a PR per feature, because they are mostly orthogonal. Would you agree to split it to more PRs, so that we may merge the migration of TCK tests to arquillian and further discuss the new features added by ConfigValue?

@OndrejM it adds a huge additional value indeed.
Currently you can inject a Long or a Provider<Long>.
A native (or wrapper) type provides no refresh at all. Using Provider will always go to the underlying Config system.

the cacheForof ConfigValue otoh provides means to locally cache values and prevents going to the underlying Config system for a specified amount of time. So it effectively removes tons of load from the Config system which gives the Config itself more room to do other tasks. It really heavily reduces the overall load.

I'm not saying it doesn't add any value. But I'd like to get the arquillian migration in TCK merged before we come to any conclusion about ConfigValue...

@OndrejM and @Emily-Jiang, I separated the test setup into an own commit for #107

Thanks @struberg! Now we can continue in this lengthy discussion :)

Signed-off-by: Mark Struberg <[email protected]>

hacked together with @OndrejM Signed-off-by: Mark Struberg <[email protected]>

places resources at correct location in the ShrinkWrap archive We cannot keep them 1:1 in the same location as they would otherwise get picked up twice in locale Arquillian containers (getResources). Signed-off-by: Mark Struberg <[email protected]>

* moving away the resources from the standard classpath * moving from a JAR to a WebArchive * bundling all resources into the WebArchive Signed-off-by: Mark Struberg <[email protected]>

Signed-off-by: Mark Struberg <[email protected]>

OndroMih

I'd like to split the ideas demonstrated in ConfigValue class into separate PRs and leave only basic stuff and migration to arquillian test cases so that we can merge it. Or please a separate PR for migration to arquillian, so that we can discuss proposed changes further.

OndroMih · 2017-03-16T08:59:36Z

api/src/main/java/org/eclipse/microprofile/config/ConfigValue.java

+    interface ConfigChanged {
+        <T> void onValueChange(String key, T oldValue, T newValue);
+    }
+}


After we decided that we would injection Provider instead of ConfigValue proxy and closing #42, ConfigValue class can be used for a different purpose, as described in this PR. It is now not intended to be injected into CDI beans, but in JavaSE API to allow for programmatic configuration and access to metadata of a single config value. In CDI API, this should be done via annotations on an injection point.

OndroMih · 2017-03-16T09:03:12Z

api/src/main/java/org/eclipse/microprofile/config/ConfigValue.java

+     *
+     * @return This builder
+     */
+    ConfigValue<T> cacheFor(long value, TimeUnit timeUnit);


I understand that this method allows defining a business requirement to cache a value for at most value+timeUnit period. This is to prevent longer caching for sensitive data, like blacklists, which should be applied ASAP. However, I'm not sure this can be ensured if config sources implement internal caching. Is there a way to notify all config sources that this value should not be cached for more than configured interval?

I would not want to specify this on the property basis. It adds too much burden on the developers. Surely, the cache for has no effect if the underline config sources have no changes.

This is often a business requirement. Different config properties often have different requirements for being up to date. This is something we added to DeltaSpike as a result to business needs. So there IS a real world need to handle this on a per-attribute basis.

@OndrejM

However, I'm not sure this can be ensured if config sources implement internal caching.

The caching of the ConfigValue is totally independent from any possible caching of any ConfigSource. This is really a local cache for a single attribute evaluation inside each ConfigValue instance.

@OndrejM in addition to my last reply: One of the points of having the ability to cache on a per-property basis is that this takes a lot load away from the Config system itself. So the ConfigSources usually are able to implement updates much faster because they have generally less load.

OndroMih · 2017-03-16T09:07:51Z

api/src/main/java/org/eclipse/microprofile/config/ConfigValue.java

+     * @param evaluateVariables whether to evaluate variables in values or not
+     * @return This builder
+     */
+    ConfigValue<T> evaluateVariables(boolean evaluateVariables);


Please also clarify the default value if evaluateVariables is not set

you mean if the referenced variable is not found?
Yes, that's a good point which should get clarified!
In that case we better leave the original string untouched.

OndroMih · 2017-03-16T09:09:02Z

api/src/main/java/org/eclipse/microprofile/config/ConfigValue.java

+     *
+     * @return This builder
+     */
+    ConfigValue<T> onChange(ConfigChanged valueChange);


We have discussed this - it is not clear in which context (thread) the ConfigChanged lambda would be executed. I would drop this method until we clarify this.

What are we going to achieve here? A use case will help.

@OndrejM it will be a synchronous callback within the same thread. Of course we should better specify how it works and how it behaves.

@Emily-Jiang A use case would be

ConfigValue cfgPort = config.access("myprj.archive.port") .as(Integer.class) .cacheFor(2, TimeUnit.MINUTES) .onChange(::logConfigChange);

and logConfigChange will then log out that a value got changed.
Of course there might be other tasks which get triggered when the configured value flips.

OndroMih · 2017-03-16T09:19:47Z

api/src/main/java/org/eclipse/microprofile/config/ConfigValue.java

+    interface ConfigChanged {
+        <T> void onValueChange(String key, T oldValue, T newValue);
+    }
+}


I think that ConfigValue in this PR covers too many things at once. I'd like to split some methods into a separate PR. As a base, I think it is a no-brainer to leave as, get, getOptional, getKey and move other methods into PRs on their own. We can then discuss separately.

OndroMih · 2017-03-16T09:22:01Z

tck/src/main/java/org/eclipse/microprofile/config/tck/ConfigValueTest.java

+
+        Assert.assertEquals(config.access("tck.config.test.javaconfig.configvalue.boolean.oui").as(Boolean.class).get(), Boolean.TRUE);
+        Assert.assertEquals(config.access("tck.config.test.javaconfig.configvalue.boolean.oui_uppercase").as(Boolean.class).get(), Boolean.TRUE);
+        Assert.assertEquals(config.access("tck.config.test.javaconfig.configvalue.boolean.oui_mixedcase").as(Boolean.class).get(), Boolean.TRUE);


I agree with Emily, the specification should be formal and pick only widely used options. Any implementation can support more options, and if it doesn't a custom converter should solve the problem. Otherwise, we would have to support all language variants - possibly Japanese, since Fujitsu have also joined MP.

OndroMih · 2017-03-16T09:25:11Z

tck/src/main/java/org/eclipse/microprofile/config/tck/base/AbstractTest.java

+        addFile(testJar, "META-INF/microprofile-config.properties");
+        addFile(testJar, "sampleconfig.yaml");
+
+        WebArchive war = ShrinkWrap


Maybe we would need to change WebArchive to JavaArchive later to allow that TCK can be passed also by implementations that don't support WAR deployments. But I agree to leave WebArchive for now, especially since I tried to hack around arquillian extensions and it is very hard, if not impossible, to turn a JavaArchive into a WebArchive before it is deployed to a container.

It originally was a JavaArchive, but I moved it to a WAR for the following reason:
WebArchive is actually the only type which is widely supported on every container. Regardless of owb and weld standalone, servlet containers or fully blown JavaEE servers.

Emily-Jiang · 2017-03-16T10:28:17Z

Please don't push in ConfigValue. I don't wish to bring down this just because it is in DeltaSpike. What is the use case here?

Emily-Jiang · 2017-03-16T11:07:08Z

Each method needs to have a use case associated with. I prefer not to add this at all in the first release. I have not got any use cases for this one.

Signed-off-by: Mark Struberg <[email protected]>

Emily-Jiang · 2017-03-17T10:04:52Z

I would say the ConfigValue is not needed for Config 1.0. Please convince me otherwise.

Signed-off-by: Mark Struberg <[email protected]>

struberg · 2017-03-17T10:19:09Z

@Emily-Jiang

I would say the ConfigValue is not needed for Config 1.0. Please convince me otherwise.

As already written dozen times above. The following use cases get added in a clean way.

caching
list support
lookup chains
callback handling
variable replacement

And all the work is finished. So just saying "I don't like it" is simply not a valid argument!
Tell me which of the features you don't like and we can discuss it. But there was no single argument against any of those features yet!

OndroMih

@struberg: the cacheFor of ConfigValue otoh provides means to locally cache values

I was again caught in the same trap that ConfigValue was used for 2 different concepts :( We shouldn't compare Provider and ConfigValue, because ConfigValue now is meant to be used in a programmatic way, via config.access(), and not injected.

With CDI first approach, I would focus more on how to add this functionality to the injection point, e.g. by a @ConfigPropery(cacheFor=10, cacheForUnit=TimeUnit.SECOND) for caching.

However, caching should be quite doable even with Provider, if you wrap it into a class that caches the value for some time, so it's not necessary to have it for v1.0. And I'm still not convinced that it would be useful if config sources themselves wouldn't refresh after the cache interval. The business request of getting a new fresh value after that interval could not be met.

OndroMih · 2017-03-17T10:17:45Z

api/src/main/java/org/eclipse/microprofile/config/ConfigValue.java

+    interface ConfigChanged {
+        <T> void onValueChange(String key, T oldValue, T newValue);
+    }
+}


Thanks @struberg! Now we can continue in this lengthy discussion :)

struberg · 2017-03-17T10:39:41Z

Yes @OndrejM we also should have a cacheFor (or whatever we call it) also for the CDI way. It should effectively be in sync so you can do the same in a programmatic and in a DI way. E.g.

@Inject 
@ConfigProperty(name="myprj.archive.url", cacheFor=5000)
private Provider<String> archiveUrl;

OndroMih · 2017-03-17T10:50:03Z

Maybe having a separate @CacheProperty would be better:

@Inject 
@ConfigProperty(name="myprj.archive.url")
@CacheProperty(for=5000, unit=TimeUnit.SECOND)
private Provider<String> archiveUrl;

I can create a dual PR to have all functionality proposed both in SE and CDI API.

Emily-Jiang · 2017-03-17T11:32:13Z

Please don't merge before we reach final agreement.

Emily-Jiang · 2017-03-17T11:35:51Z

@struberg I don't think the features you listed are absolutely necessary for the config 1.0. We have a lot of cleanup to do and doc/tests to update and add. The list goes on. We cannot keep on going adding more and more stuff. When I first write the proposal, I listed the dynamic support. That is one of the features to be provided and demonstrated.

struberg · 2017-03-17T11:59:09Z

Please don't merge before we reach final agreement.

Like you did yesterday despite 2 -1 votes and open clarification requests? ;)

No worries...

struberg · 2017-03-17T12:03:44Z

When I first write the proposal, I listed the dynamic support.

The first config proposal from June 2016 did actually contain all those features. So this is not a valid argument. And it is all finished. It really works already.

Tell me which use case of the 5 you think is not useful and we can discuss it.
And again: "I don't like it" is not an argument!

Emily-Jiang · 2017-03-17T12:12:17Z

@struberg I'm not going to argue with you over the merging. You merged multiple commits without waiting for the final solution without any pre-warning. I messed up my environment and the commit contains multiple things which I could not lift it out. I prewarned with a comment saying it will be discussed in the hangout and it is only line and I said I would remove today if no agreement were reached.

Emily-Jiang · 2017-03-17T12:17:23Z

For the features you listed:

caching
list support
lookup chains
callback handling
variable replacement

I am not saying they are not valid or useful. Are they must for the Config 1.0 release? I am cautious about the deadline. We have a lot of open issues to be closed in a short timeframe.

struberg · 2017-03-17T12:19:17Z

Oki no worries, we will get it done :)
I'm also fine to postpone it post 1.0.
But the features are still valid.

Emily-Jiang · 2017-03-17T12:25:21Z

@struberg agreed!

OndroMih · 2017-03-17T12:50:09Z

+1 to leave the PR open and leave it to post 1.0

All those are nice features and I'd like to have them definitely in 1.1, but I'd like to have a CDI equivalent at the same time. For 1.0 it would be a lot of work to create the CDI equivalent, not even talking about updating the spec document and polishing TCK and javadoc (if necessary).

I'd like to finalize what we have, cleanup the PRs and issues related to 1.0, and align the spec documents with all the rest. Still a lot of work to do. If all goes well, the MP umbrella could even skip config v1.0 and include config v1.1 if it is finished before MP 1.1

Emily-Jiang · 2019-03-11T14:59:25Z

This PR was superseded by some later merged PRs. Close this one.

add dynamic value pull handling, callback and TCK fixes

19ba8f4

Emily-Jiang reviewed Mar 9, 2017

View reviewed changes

struberg added 7 commits March 10, 2017 10:24

Merge branch 'master' into configValue

6d3ac4d

Signed-off-by: Mark Struberg <[email protected]>

refs: eclipse#67 fix merge error

e9fb617

Signed-off-by: Mark Struberg <[email protected]>

refs: eclipse#56 treating empty as not existing.fix broken TCK tests

8ac2220

Signed-off-by: Mark Struberg <[email protected]>

refs: eclipse#42 add Arquillian integration for CDI support

c390f91

hacked together with @OndrejM Signed-off-by: Mark Struberg <[email protected]>

fix TCK setup for a wide range of containers

a55214d

* moving away the resources from the standard classpath * moving from a JAR to a WebArchive * bundling all resources into the WebArchive Signed-off-by: Mark Struberg <[email protected]>

Merge branch 'master' into configValue

eff40fe

Signed-off-by: Mark Struberg <[email protected]>

OndroMih suggested changes Mar 16, 2017

View reviewed changes

Merge branch 'master' into configValue

7943574

Signed-off-by: Mark Struberg <[email protected]>

struberg mentioned this pull request Mar 17, 2017

Move tests to use Arquillian #107

Closed

Merge branch 'master' into configValue

5358bfa

Signed-off-by: Mark Struberg <[email protected]>

OndroMih reviewed Mar 17, 2017

View reviewed changes

OndroMih added the mpconfig-later label Aug 26, 2017

OndroMih mentioned this pull request Apr 16, 2018

Access to config value metadata using a ConfigValue wrapper #43

Closed

Emily-Jiang changed the title ~~ConfigValue implementation for dynamic properties~~ Ouf of date: ConfigValue implementation for dynamic properties Mar 11, 2019

Emily-Jiang closed this Mar 11, 2019

dmlloyd added this to the MP Config future milestone Mar 4, 2020

Ouf of date: ConfigValue implementation for dynamic properties #91

Ouf of date: ConfigValue implementation for dynamic properties #91

Conversation

struberg commented Mar 9, 2017

Emily-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

struberg Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndroMih left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emily-Jiang commented Mar 16, 2017

Emily-Jiang commented Mar 16, 2017

Emily-Jiang commented Mar 17, 2017

struberg commented Mar 17, 2017

OndroMih left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

struberg commented Mar 17, 2017

OndroMih commented Mar 17, 2017

Emily-Jiang commented Mar 17, 2017

Emily-Jiang commented Mar 17, 2017

struberg commented Mar 17, 2017

struberg commented Mar 17, 2017

Emily-Jiang commented Mar 17, 2017

Emily-Jiang commented Mar 17, 2017

struberg commented Mar 17, 2017

Emily-Jiang commented Mar 17, 2017

OndroMih commented Mar 17, 2017

Emily-Jiang commented Mar 11, 2019

struberg Mar 10, 2017 •

edited

Loading