#457 add a section to make dynamic config source more visible

[Emily-Jiang]

Signed-off-by: Emily Jiang emijiang6@googlemail.com

[kenfinnigan]

Maybe also mention that whether a ConfigSource supports this dynamic behavior or not is dependent on how it's implemented.

Need to make clear there's no guarantee as to whether any ConfigSource does, or does not, support this dynamic behavior and that it's a case by case basis

[dmlloyd]

I think that calling out dynamic config sources is exactly the opposite of what we agreed on last week's call. Since there's nothing we can do about the dynamicity of a given config source, we agreed not to specify whether or not it is dynamic, but instead only impose thread-safety requirements on the property names collection(s) and iterator(s).

[Emily-Jiang]

@dmlloyd Since the spec already supported dynamic behaviour and having tck to verify this. I think it is a good idea to clarify this. I did add a few sentences as per @kenfinnigan 's comments. As for the thread-safety, I think we were more in the context of whether all properties appear in getPropertyNames as you rightly pointed out some sensitive properties might be filtered out on purpose.

[ljnelson]

"MicroProfile Config Implementation [sic] can decide whether a ConfigSource can be dynamic or not": I find this hard to understand. ConfigSources can be packaged with applications. How can a given MicroProfile Config implementation "decide" whether a ConfigSource packaged with an application is dynamic or not?

[kenfinnigan]

@ljnelson it somewhat depends on whether a ConfigSource implementation using a backing source that could be updated.

@Emily-Jiang I'm not sure we can say "System Properties are dynamic by nature". Is there a requirement in the spec that the System Property ConfigSource an implementation includes doesn't copy the entire set of properties into an internal map on creation? If it does a copy, then System Property isn't dynamic as adding a new one won't appear in that ConfigSource

[ljnelson]

@kenfinnigan Not trying to be difficult, but again: how is, say, an OpenLiberty MicroProfile Config implementation shipped with an application server supposed to "know" that my ConfigSource implementation—the one in my application deployed to that server—is dynamic? It cannot.

[kenfinnigan]

Ah, see your point now.

So yes, an MP Config implementation can only "know" whether the ConfigSources it provides are dynamic or not. There is no way for it to "know" about any ConfigSource that is included within the application code directly

[ljnelson]

Correct. In other words: what is the contract of ConfigSource#getPropertyNames()? It is so underspecified as to be almost useless, viz. is the returned Set an immutable snapshot? A mutating snapshot? Is it thread safe (i.e. what must I do to iterate over it)? If I call the method two times must I get back equal Sets? Identical Sets? And so on. (And that's before even considering the ConfigSource#getProperties() method, which suffers from the same kind of lack of specificity.)

[ljnelson]

Right, and not just TCK tests: this is a public API exposed to the end user. She cannot use it because she does not know how to use the returned Set. Must she synchronize on it? Or not? Can she cache it? And so on. We are in agreement here at a high level that there is much more work to be done here, I think.

[kenfinnigan]

I agree that there is still lots to be done.

In terms of this PR, I'm wondering if we need to go back to discussing improving the definitions of the API first.

[Emily-Jiang]

@ljnelson I was going to add a PR to say the getPropertyNames() can be iterated in a thread safe manner based on the suggestion from @dmlloyd
@kenfinnigan there is a TCK to verify the dynamic aspect of system property.

[ljnelson]

Yes, that's a good start. An even better start would be to make sure that the iteration is fully specified. See the Concurrent Collections section in the package description of the java.util.concurrent package documentation for a particularly good example.

[Emily-Jiang]

Follow up on what I mentioned the following tcks to assert dynamic config lookup.

ConfigProviderTest.testDynamicValueInPropertyConfigSource()
ConfigProviderTest.testGetPropertyNames()
ConfigProviderTest.testPropertyConfigSource() 
EmptyValuesTest.testEmptyStringProgrammaticLookup()

[dmlloyd]

Is this still current, given the recent spec updates?

[Emily-Jiang]

I think this is still relevant as the spec still supports dynamic configsources. It worths the clarification.

[Emily-Jiang]

I would like to merge this PR by 15th July as I have answered all questions and have got 2 approvals. Please log your comments asap.

[radcortez]

Some typos need to be fixed.

[Emily-Jiang]

@radcortez I have done all of the tidy up including the format. Please double check and approve if no more comments.

[radcortez]

We probably need for @ljnelson to check the latest update to see if it addresses his comments.

[Emily-Jiang]

@radcortez I've addressed all of your comments! Please check again.

[Emily-Jiang]

@ljnelson can you double check to see whether this PR has addressed your issue?

[ljnelson]

It is written:

A dynamic ConfigSource is a ConfigSource where its content can be updated, such as updating a property name or value or adding or deleting config properties from it.

With respect, that is not grammatically correct. Nor does it indicate who is doing the updating: a naïve user might very well see "its content can be updated" and start looking for an update() method on org.eclipse.microprofile.config.spi.ConfigSource, or might look for a class named DynamicConfigSource or something.

I think instead you're trying to say something about the dynamism of the data that underlies any given ConfigSource, not the dynamism of the ConfigSource itself, right?

In other words, I think you might be trying to say something as simple as this (and indeed maybe the whole PR comes down to a couple of sentences):

"A ConfigSource is a view of configuration data. That data may be changing, or unchanging. If the data is changing, and a ConfigSource can represent its changes, we call that ConfigSource a dynamic ConfigSource, since at any two moments two operations on it may reflect two different sets of underlying configuration data. If instead the data is unchanging, we call the ConfigSource {something else that I don't think anyone has named yet; maybe static}, since at any two moments two operations on it will reflect only one set of underlying (unchanging) configuration data. A caller cannot know whether a ConfigSource is dynamic or static."

That is just a suggestion. (I sense some sudden urgency to this PR for some reason so I won't stand in the way of whatever you want to do.)

[radcortez]

@ljnelson the urgency is because there is planned RC release of MP platform by 1st week of August, so all individuals specs should have their RC's ready as well.

I like your text better, so I think we can modify it to better express the view. Cheers!

[Emily-Jiang]

@ljnelson I have updated the sentences in question with your suggested text with slight modification. Please double check to see whether you have any further comments.

[Emily-Jiang]

@ljnelson I'll merge this PR by COB today if I don't hear more feedback from you today.