spring-projects / spring-data-geode

Spring Data support for Apache Geode
Apache License 2.0
50 stars 38 forks source link

Add enumeration for eviction attributes [DATAGEODE-87] #134

Closed spring-projects-issues closed 6 years ago

spring-projects-issues commented 6 years ago

Srikanth Manvi opened DATAGEODE-87 and commented

Currently, in the gfe namespace, 2 attributes (action and type) of the gfe:eviction element don’t have a enumeration defined for the possible values that the attributes can take. This causes the possible values (_LOCALDESTROY, _OVERFLOW_TO_DISK _for action and _ENTRYCOUNT, _HEAPPERCENTAGE, _MEMORYSIZE for type) not be prompted by the auto-suggest feature in the IDEs making it less developer friendly and also making it possible for users to enter random strings. This is unlike other attributes in the namespace, Ex: IDE suggests the possible values for the shortcut attribute for any of the gfe:*-region elements which is developer friendly.

Note: Similar improvement can be made for expiration (gfe:region-ttl, gfe:region-tti) attributes as well. I will open a new JIRA for that


Referenced from: pull request https://github.com/spring-projects/spring-data-geode/pull/4

spring-projects-issues commented 6 years ago

Srikanth Manvi commented

Hi John, I have made the changes to the xsd, and that caused some tests to break. Looking into fixing them. I will submit a PR if you think this improvement request is valid.

spring-projects-issues commented 6 years ago

John Blum commented

Hi Srikanth Manvi-

Thank you for the JIRA and PR.

Unfortunately, I cannot accept this change since most of the SDG XML namespace element attributes were modified sometime ago to be less restrictive, i.e. not strongly-typed (e.g. by using enumerations, etc).

The primary reason for this is that users wanted to be able to "dynamically" configure these attributes using either Spring property placeholders or even SpEL expressions.

Therefore, it is possible to do something like the following...

<gfe:replicated-region id="Example">
    <gfe:eviction type="${eviction.type}" action="OVERFLOW_TO_DISK"/>
</gfe:replicated-region>

The value of the $\{eviction.type\} property placeholder may come from any external configuration/property source.

Unfortunately, this means we have to forgo type safety at the schema-level and provide the necessary enforcement in the Spring parsers. It also means we give up a degree of convenience provided by the IDE. However, it is simply because these values may need to be tweaked based on environment, etc.

In fact, this was evident in 1 of the tests you modified, the RegionEvictionAttributesNamespaceTest and the corresponding XML configuration file RegionEvictionAttributesNamespaceTest-context.xml, specifically here and here, by way of example.

Now, I realize there are still some attributes (very few in fact) that still are "strongly-typed" like Region data-policy and shortcut attributes.

That was deliberate and not arbitrary since I really wanted to discourage users from using these attributes and having to explicitly know the enumeration of GemFire/Geode o.a.g.cache.DataPolicy, o.a.g.cache.RegionShortcut and o.a.g.cache.client.ClientRegionShortcut values.

The are defeating actually since DataPolicy of REPLICATE is redundant on the <gfe:replicated-region> element since this element creates a REPLICATE Region, or say a RegionShortcut of PARTITION_PERSISTENT when the user should instead rely on <gfe:partitioned-region id="Example" persistent="true"/>, etc.

Hope this helps clarify my position on this.

If you have further questions or comments, please post here