Refactor #384

[jmesnil]

Update the onAttributeChange method to return a ChangeSupport class that
can be used to handle release of resources (in the close method).

When a Config is released, it should call ChangeSupport.close to ensure
that the ConfigSource has a chance to properly release any resources
held by the callbacks.

• Add TCK test ConfigAccessorTest#testOnAttributeChange

Signed-off-by: Jeff Mesnil [email protected]

[jmesnil]

@Emily-Jiang this is the same PR that #398 but for master .

[Emily-Jiang]

LGTM!

[struberg]

This is a duplicated mechanism now, isn't?
If the ConfigSource is AutoCloseable then it will already be automatically closed().

[struberg]

This test seems weird, what should it actually do?

[jmesnil]

the test checks that when the config source change the property value, the config is properly notified and the value from the ConfigAccessor is invalidated and fetch from the config source again.

I should have put a longer cache duration to make it clearer.

[jmesnil]

ChangeSupport is Closeable so that when a Config is closed, it can notify the ConfigSource to release any resources corresponding to the Config.

Of course, when the ConfigSource itself is closed, it will close any ChangeSupport it holds.

[struberg]

But why all that? what is the reason?
What would you like to close in which case?

The Config can already close all the ConfigSources, so what was missing? I still don't get that!

We have a ticket text which describes which code changes you do. But that's completely redundant to the code itself. What is missing is the WHY! WHY did you do that change. This is not apparent to me. Could you help me to understand the reason? I'd rather stick with KISS...

[jmesnil]

A ConfigSource can belong to many Config and can outlive them.

When a Config is closed, it must have a way to inform the ConfigSource that it no longer needs to receive attribute change notification (and the ConfigSource can release any resource associated to this specific Config). That's the reason behind making ChangeSupport be Closeable.

[jmesnil]

The Config can already close all the ConfigSources, so what was missing? I still don't get that!

That's not correct. A Config can only close ConfigSource that implements AutoCloseable, not all of them.
Besides, even when a ConfigSource implements AutoCloseable, it has no way to know which Config is calling its close() method.

If a ConfigSource could belong to a single ConfigSource, all I said would be moot. But the ConfigBuilder API allows to share ConfigSource among Config.

[struberg]

No Jeff, A ConfigSource instance must ALWAYS ONLY be part of a single Config. Tomas, Sebastian, Romain and I already explained this already a dozen times.
Of course one can internally share the config. But the registered ConfigSource instance is always 1:1 to the Config.

Having a shared Configuration is the exceptional case. So we should not make it more complicated for the standard use case.
What you need to do in your more advanced case is to have a wrapper ConfigSource (one per Config) which keeps a reference counter on your internally shared resource.

[jmesnil]

Maybe we should write that down in the spec instead of repeating it a 13 times.

By construction, the ConfigBuilder API allows to share ConfigSource among Config. That's not an exceptional case, that's the API design.
"A ConfigSource instance must ALWAYS ONLY be part of a single Config" is in contradiction to the API.

[struberg]

It is technical possible, but it is not intended.
You can also technically create a CriteriaQuery via OpenJPA and pass it to a Hibernate EntityManager. The API allows it. But it will never work.
But I agree, we should probably write it into the spec. Gonna ship a PR.

[rmannibucau]

I think Mark is right and this change should be reverted since it doesnt bring anything not already doable before. Also the test does not sound accurate too.

Also agree to enforce spec wording if you think it is not already the case, any natuve or well speaking person able to help?

[jmesnil]

I'm ok with reverting the change provided it is clearly written in the spec (or the ConfigBuilder API) that a ConfigSource must be owned by a single Config.

As for the test, there was 0 tests for this feature in the original PR. Any contribution to add or improve tests that verifies this feature is welcome.

[rmannibucau]

A config source with update feature only but the wording update makes sense. Probably with something like "otherwise the behavior is up to the impl".