I9 config snapshot by struberg · Pull Request #62 · eclipse-archived/ConfigJSR

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

This adresses #9, #8 and #57

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

…to multiple values 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.

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

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

…alues

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

+     * 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

-     * 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

-        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 <struberg@apache.org>

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

+     * @return a {@code ConfigValue} to access the given propertyName
     */
-    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

@@ -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

+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

+/**
+ * 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

     * @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

@@ -192,29 +209,24 @@ public interface ConfigValue<T> {
    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

-        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

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


bulk replaced/fixed all Copyright years to 2018

…rding

Signed-off-by: Mark Struberg <struberg@apache.org>

as discussed in the EG meeting Signed-off-by: Mark Struberg <struberg@apache.org>

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-archived#9 remove callback method from Config API.

f6f965d

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

refs: eclipse-archived#57 eclipse-archived#9 fix JavaDoc

88dbafb

refs: eclipse-archived#9 cleanup JavaDoc

c89b9cd

refs: eclipse-archived#9 improve lookupChain description

bb16b7d

refs: eclipse-archived#9 improve JavaDoc

c15d75f

refs: eclipse-archived#63 eclipse-archived#57 temporarily remove onCh…

8f946ab

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

refs: eclipse-archived#24 fix TCK setup bug

867068f

refs: eclipse-archived#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-archived#9 eclipse-archived#57 refine TCK for dynamic v…

db9d9c8

…alues

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

93ba47e

tomas-langer reviewed May 10, 2018

View reviewed changes

refs: eclipse-archived#9 improve docs for getResolvedKey

cca4502

Signed-off-by: Mark Struberg <struberg@apache.org>

struberg added 3 commits May 11, 2018 14:21

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

e29ba9d

refs: eclipse-archived#9 add TCK for withDefault and getDefaultValue

e11b8b1

refs: eclipse-archived#9 improve default values TCK

c266a4d

Emily-Jiang suggested changes May 13, 2018

View reviewed changes

struberg added 6 commits May 17, 2018 16:07

refs: eclipse-archived#9 start with Converter and ConfigValue spec wo…

1487607

…rding

updating the license year to 2018

7c8c2ee

Signed-off-by: Mark Struberg <struberg@apache.org>

refs: eclipse-archived#9 improve useConverter javadoc wording

81e4255

refs: eclipse-archived#9 rename ConfigValue to ConfigAccessor

2021eab

as discussed in the EG meeting Signed-off-by: Mark Struberg <struberg@apache.org>

refs: eclipse-archived#9 rename getKey to getPropertyName

3f0e78c

refa: eclipse-archived#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-archived#9 fix JavaDoc varargs, txs to ejiang

987ece9

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

		@@ -192,29 +209,24 @@ public interface ConfigValue<T> {
		String getKey();

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

Conversation

struberg commented May 3, 2018

Uh oh!

struberg commented May 8, 2018

Uh oh!

tomas-langer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

struberg commented May 11, 2018

Uh oh!

tomas-langer commented May 13, 2018

Uh oh!

Emily-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants