I9 config snapshot #62

struberg · 2018-05-03T12:20:53Z

This adresses #9, #8 and #57

We will probably introduce another way to get notified later on.

This code got originally developed on 2018-04-04 in Apache DeltaSpike by Mark Struberg, Manfred Huber, Alex Falb, Gerhard Petracek and Romain Manni-Bucau based on the problem analysis discussions between Mark Struberg and Tomas Langer (tomas-langer) who also did reviews and gave important feedback.

…eparate discussion As agreed in the last EG meeting I gonna split out this feature into a separate ticket (eclipse#63).

Using the same attributes in different TCK tests but with different values leads to random behaviour of the whole TCK suite.

struberg · 2018-05-08T16:48:17Z

Hi!

I've removed the onValueChange callback as discussed in our last EG meeting.

tomas-langer

There are a few things that should be fixed before merging.

tomas-langer · 2018-05-10T14:13:10Z

api/src/main/java/javax/config/ConfigValue.java

-     * Returns the actual key which led to successful resolution and corresponds to the resolved value. This applies
-     * only when {@link #withLookupChain(String...)} is used.
+     * Returns the actual key which led to successful resolution and corresponds to the resolved value.
+     * This is mostly useful when {@link #withLookupChain(String...)} or {@link #evaluateVariables(boolean)} is used.
     * Otherwise the resolved key should always be equal to the original key.
     * This method is provided for cases, when arameterized resolution is


Typo: probably should be "parametrized".

Actually evaluateVariables does not have any impact at all the more I think about it.
evaluateVariable works on the value. It is a post processing of the value. But it doesn't affect the key at all. Gonna remove it.

Just wondered whether we should remove getResolvedKey at all?
It only gives you the value from the last 'successful' resolve. And if this happens in parallel rather quickly you might end up with glitches from another thread.
Of course this will only happen if the underlying config changes, otherwise it will be idempotent anyway.

Yes, the usability is questionable - why would I need this information in my business logic?
Though there may be some troubleshooting use cases (e.g. debugging this information).
I don't have a strong opinion on this one, so choose ;)

Indeed, would be clever to log it out in case of onChange - If only we would not just have ditched that ;)

tomas-langer · 2018-05-10T14:14:04Z

api/src/main/java/javax/config/ConfigValue.java

-     * Returns the actual key which led to successful resolution and corresponds to the resolved value. This applies
-     * only when {@link #withLookupChain(String...)} is used.
+     * Returns the actual key which led to successful resolution and corresponds to the resolved value.
+     * This is mostly useful when {@link #withLookupChain(String...)} or {@link #evaluateVariables(boolean)} is used.


What if this key is based on multiple variables? Which one is the "resolved one"?

The last effective String.
So if you have cfg.access("myconfigkey").withLookupChain("${app.projectstage}")
and app.projectstage resolves to 'Prodution' you might end up with 'myconfigkey.Production'

tomas-langer · 2018-05-10T14:18:49Z

tck/src/main/java/org/eclipse/configjsr/DynamicConfigSourceTest.java

-        int i2 = vals.get(DynamicChangeConfigSource.TEST_ATTRIBUTE);
-        assertTrue(i2 > i1);
+    public void testValueInvalidationOnConfigChange() throws Exception {
+        ConfigValue<Integer> valCfg = config.access(DynamicChangeConfigSource.TEST_ATTRIBUTE)


I considered this to be optional approach by the implementor. This behavior should not be tested - the implementation may decide not to refresh the value until the cache for is expired.

Well, the intention is to define that it must expire the cache if the Config gets notified about a change by the ConfigSource. Do you really think we should not mandate this?

Yes I think so. This does not stand in the way of portability. We can mention that the implementation should refresh the value, though I still don't see this as a must.

But not defining this as mandatory feature renders ConfigSource#setOnAttributeChange useless on implementations not supporting it.
So I'd rather tighten it instead of removing.

We need to find a way to test this. Otherwise, how to mandate implementor implement this?

There should be a test for it already. It sets a cacheFor to 15 minutes, then loads the values. Then the ConfigSource changes a value via the TCK test and fires the attributeChanged to the Config. And then the test reads the value again and verifies that the new value is picked up immediately.

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

struberg · 2018-05-11T10:56:37Z

Oki so in summary? Looks fine @tomas-langer? In which case I suggest we apply it now and refine it all together.

tomas-langer · 2018-05-13T22:05:06Z

LGTM, I would go through the must/should decision during our meeting next week (as this is to be decided in the spec - and TCK can be changed accordingly if we decide against your approach).

Emily-Jiang

TCKs to be added for

useConverter
getKey
getResolvedKey
getValue(ConfigSnapShot)

Emily-Jiang · 2018-05-13T22:18:52Z

api/src/main/java/javax/config/Config.java

     */
-    ConfigValue<String> access(String key);
+    ConfigValue<String> access(String propertyName);


I would change the method name to getConfigValue instead

I'd rather not do that. Then we'd have getValue and getConfigValue. That would confuse users I fear.

Emily-Jiang · 2018-05-13T22:21:45Z

api/src/main/java/javax/config/ConfigSnapshot.java

@@ -0,0 +1,42 @@
+/*
+ *******************************************************************************
+ * Copyright (c) 2011-2017 Contributors to the Eclipse Foundation and others


should be 2011-2018

Emily-Jiang · 2018-05-13T22:22:56Z

api/src/main/java/javax/config/ConfigSnapshot.java

+package javax.config;
+
+/**
+ * A value holder for TypedResolver values which all got resolved in a guaranteed atomic way.


Where was TypedResolver defined?

Emily-Jiang · 2018-05-13T22:23:24Z

api/src/main/java/javax/config/ConfigSnapshot.java

+/**
+ * A value holder for TypedResolver values which all got resolved in a guaranteed atomic way.
+ *
+ * @see Config#snapshotFor(ConfigValue[])


It is varargs

Emily-Jiang · 2018-05-13T22:26:00Z

api/src/main/java/javax/config/ConfigValue.java

     * @return This builder as a typed ConfigValue
     */
-    <N> ConfigValue<N> useConverter(Converter<N> converter);
+    ConfigValue<T> useConverter(Converter<T> converter);


Why should this converter here? Is this converter not loaded via service loader pattern? What is the use case here?

No this is intended for when you do not want to use the default converter but a special handling.

Emily-Jiang · 2018-05-13T22:35:33Z

api/src/main/java/javax/config/ConfigValue.java

@@ -192,29 +209,24 @@
    String getKey();


Why does getKey() return a value?

This is the name of the oridingal config property name. We might probably rename it.

Emily-Jiang · 2018-05-13T22:37:57Z

tck/src/main/java/org/eclipse/configjsr/DynamicConfigSourceTest.java

-        int i2 = vals.get(DynamicChangeConfigSource.TEST_ATTRIBUTE);
-        assertTrue(i2 > i1);
+    public void testValueInvalidationOnConfigChange() throws Exception {
+        ConfigValue<Integer> valCfg = config.access(DynamicChangeConfigSource.TEST_ATTRIBUTE)


We need to find a way to test this. Otherwise, how to mandate implementor implement this?

Emily-Jiang · 2018-05-13T22:38:25Z

tck/src/main/java/org/eclipse/configjsr/configsources/ConfigurableConfigSource.java

@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2016-2017 Contributors to the Eclipse Foundation


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

as discussed in the EG meeting Signed-off-by: Mark Struberg <[email protected]>

Emily and Tomas will come up with a better way to test this behaviour

struberg added 11 commits April 18, 2018 13:09

refs: eclipse#9 remove callback method from Config API.

f6f965d

We will probably introduce another way to get notified later on.

refs: eclipse#57 eclipse#9 fix JavaDoc

88dbafb

refs: eclipse#9 cleanup JavaDoc

c89b9cd

refs: eclipse#9 improve lookupChain description

bb16b7d

refs: eclipse#9 improve JavaDoc

c15d75f

refs: eclipse#63 eclipse#57 temporarily remove onChange feature for s…

8f946ab

…eparate discussion As agreed in the last EG meeting I gonna split out this feature into a separate ticket (eclipse#63).

refs: eclipse#24 fix TCK setup bug

867068f

refs: eclipse#24 fix broken TCK setup

a63b09f

Using the same attributes in different TCK tests but with different values leads to random behaviour of the whole TCK suite.

refs: eclipse#9 eclipse#57 refine TCK for dynamic values

db9d9c8

refs: eclipse#57 eclipse#9 fix ConfigValue TCK tests

93ba47e

tomas-langer reviewed May 10, 2018

View reviewed changes

refs: eclipse#9 improve docs for getResolvedKey

cca4502

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

struberg added 3 commits May 11, 2018 14:21

refs: eclipse#9 improve doc&TCK for getResolvedValue()

e29ba9d

refs: eclipse#9 add TCK for withDefault and getDefaultValue

e11b8b1

refs: eclipse#9 improve default values TCK

c266a4d

Emily-Jiang requested changes May 13, 2018

View reviewed changes

struberg added 6 commits May 17, 2018 16:07

refs: eclipse#9 start with Converter and ConfigValue spec wording

1487607

updating the license year to 2018

7c8c2ee

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

refs: eclipse#9 improve useConverter javadoc wording

81e4255

refs: eclipse#9 rename ConfigValue to ConfigAccessor

2021eab

as discussed in the EG meeting Signed-off-by: Mark Struberg <[email protected]>

refs: eclipse#9 rename getKey to getPropertyName

3f0e78c

refa: eclipse#9 disable onChange tck test for now

3fdf4fe

Emily and Tomas will come up with a better way to test this behaviour

struberg mentioned this pull request May 17, 2018

add missing TCK tests for ConfigAccessor #65

Open

refs: eclipse#9 fix JavaDoc varargs, txs to ejiang

987ece9

struberg merged commit 987ece9 into eclipse:master May 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I9 config snapshot #62

I9 config snapshot #62

struberg commented May 3, 2018

struberg commented May 8, 2018

tomas-langer left a comment

tomas-langer May 10, 2018

struberg May 11, 2018

struberg May 11, 2018

tomas-langer May 11, 2018

struberg May 11, 2018

tomas-langer May 10, 2018

struberg May 11, 2018

tomas-langer May 10, 2018

struberg May 11, 2018

tomas-langer May 11, 2018

struberg May 11, 2018

Emily-Jiang May 13, 2018

struberg May 14, 2018

struberg commented May 11, 2018

tomas-langer commented May 13, 2018

Emily-Jiang left a comment

Emily-Jiang May 13, 2018

struberg May 14, 2018

Emily-Jiang May 13, 2018

Emily-Jiang May 13, 2018

Emily-Jiang May 13, 2018

Emily-Jiang May 13, 2018

struberg May 17, 2018

Emily-Jiang May 13, 2018

struberg May 17, 2018

Emily-Jiang May 13, 2018

Emily-Jiang May 13, 2018

struberg May 17, 2018

		@@ -0,0 +1,79 @@
		/*
		* Copyright (c) 2016-2017 Contributors to the Eclipse Foundation

I9 config snapshot #62

I9 config snapshot #62

Conversation

struberg commented May 3, 2018

struberg commented May 8, 2018

tomas-langer 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

struberg commented May 11, 2018

tomas-langer commented May 13, 2018

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

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