ISPN-11424 Deprecate StorageType Configuration #8240

gustavocoding · 2020-04-24T09:54:44Z

https://issues.redhat.com/browse/ISPN-11424

wburns

We also need to update the user guide as all the old examples are completely out of date. And we need to explain when you can and can't use sized based eviction and make sure we have ample examples.

wburns · 2020-04-29T13:08:53Z

core/src/main/java/org/infinispan/configuration/cache/EncodingConfigurationBuilder.java

@@ -65,6 +82,11 @@ public EncodingConfiguration create() {

   @Override
   public void validate(GlobalConfiguration globalConfig) {
+      if(mediaType != null) {


Nitpick: space

wburns · 2020-04-29T13:58:05Z

core/src/main/java/org/infinispan/configuration/cache/MemoryConfiguration.java

+      return ELEMENT_DEFINITION;
+   }
+
+   public boolean offHeap() {


Suggested change

public boolean offHeap() {

public boolean isStoredOffHeap() {

The builders in general seem to avoid the is for boolean properties, was just following the trend

I can see either way. To me the name offHeap doesn't really convey what the setting is personally. Some Configuration objects have is or uses etc for prefixes of such config options. It is just a suggestion.

I also prefer the way you suggested, I will change it

wburns · 2020-04-29T14:01:41Z

core/src/main/java/org/infinispan/configuration/cache/MemoryConfiguration.java

+    */
+   public long maxSizeBytes() {
+      String str = attributes.attribute(MAX_SIZE).get();
+      if(str == null) return -1;


Nitpick: space

wburns · 2020-04-29T14:02:27Z

core/src/main/java/org/infinispan/configuration/cache/MemoryConfiguration.java

+   public long maxSizeBytes() {
+      String str = attributes.attribute(MAX_SIZE).get();
+      if(str == null) return -1;
+      return ByteQuantity.parse(str);


I wonder if instead we should store the parsed value in the Configuration object. Otherwise everytime we invoke this we have to parse the String.

wburns · 2020-04-29T14:15:17Z

core/src/main/java/org/infinispan/configuration/cache/MemoryConfigurationBuilder.java

+   public void validate() {
+      if (hasLegacyConfiguration()) {
+         if (isSizeBounded() || isCountBounded()) {
+            CONFIG.ignoredMemoryDeprecatedSettings("size", memoryStorageConfigurationBuilder.size());


I would just throw an exception if using both, to remove ambiguity.

wburns · 2020-04-29T15:00:23Z

core/src/main/resources/schema/infinispan-config-11.0.xsd

+        <xs:documentation>
+           Defines the size of the data container in a byte quantity plus an optional multiplier.
+           Supported multipliers are kB (kilobytes), MB (megabytes), GB (gigabytes) and TB (terabytes).
+           Eviction occurs either the amount of memory exceeds the maximum size.


Suggested change

Eviction occurs either the amount of memory exceeds the maximum size.

Eviction occurs after the approximate memory usage exceeds the maximum size.

Guessing we may also want to mention that binary or off heap media type is specified.

wburns · 2020-04-29T15:22:54Z

core/src/main/resources/schema/infinispan-config-11.0.xsd

+    <xs:attribute name="max-count" type="xs:long" default="-1">
+      <xs:annotation>
+        <xs:documentation>
+          Defines the max number of entries in the data container, before eviction occurs.


To keep consistent with max-size

Suggested change

Defines the max number of entries in the data container, before eviction occurs.

Defines the size of the data container by a count of entries. Eviction occurs after the container size exceeds the maximum count.

wburns · 2020-04-29T15:23:27Z

core/src/main/resources/schema/infinispan-config-11.0.xsd

+        <xs:documentation>
+          Specifies a strategy for evicting cache entries. Eviction always
+          takes place when you define either the max-size or the max-count
+          (but not both) for the data container.


Should probably mention that REMOVE is used if no strategy is defined but size or count are.

wburns · 2020-04-29T15:29:18Z

commons/all/src/main/java/org/infinispan/commons/dataconversion/MediaType.java

+   /**
+    * @return true if the MediaType's java type is a byte array.
+    */
+   public boolean isBinary() {


So to clarify we assume everything is binary if it has ByteArray as a type in the media type (ie. application/object;type=ByteArray) or it doesn't match application object?

Media types like text/* wouldn't be binary, although they could support size based eviction.

Also I am not sure for application/unknown etc.

To be honest thinking more we may need pluggable size calculators based on media type moving forward.

The idea is that everything is binary by default. text/plain is UTF-8 byte[], same as application/json.
Regarding size calculators, it's enough to know the java type from the MediaType, right?
Most MediaType are byte[], apart from that, application/x-java-object represents random Java objects, a type can make it more specific, like application/x-java-object; type=java.lang.String

The idea is that everything is binary by default.

Okay that was the part I was unaware.

I guess in that case the size calculators are probably fine then how they are.

wburns · 2020-04-29T15:34:38Z

commons/all/src/main/java/org/infinispan/commons/util/ByteQuantity.java

+ *
+ * @since 11.0
+ */
+public final class ByteQuantity {


We have the MemoryUnit class that already does a lot of this. Maybe we can consolidate?

I have completely missed that 👍

gustavocoding · 2020-04-29T15:41:00Z

We also need to update the user guide as all the old examples are completely out of date. And we need to explain when you can and can't use sized based eviction and make sure we have ample examples.

To avoid sending a massive PR at once, I limited the changes to only "production" code here. Next PR will remove all the deprecated stuff from tests, and yes, the doc needs updating as well

tristantarrant · 2020-05-03T15:41:14Z

Conflict

gustavocoding · 2020-05-06T09:47:13Z

Updated and rebased @wburns

wburns

I haven't dug deeper, but it seems we should be able to simplify the conversion still. But maybe there is something I am not seeing that would block that.

wburns · 2020-05-06T13:40:39Z

commons/all/src/main/java/org/infinispan/commons/util/MemoryUnit.java

+/**
+ * @deprecated since 11.0, use {@link ByteQuantity} instead.
+ */
+@Deprecated


Hrmm, is there any reason we couldn't have added enhanced the parser of this class instead?

Also we don't have all the enum instances with the new one or conversions between.

The public interface is based on long for the quantity, I think the new one is more flexible.
MemoryUnit can convert from one type to another, but it is not really used apart from a unit test?

wburns · 2020-05-06T13:48:14Z

core/src/main/java/org/infinispan/configuration/cache/AttributeChangeTracker.java

+ * @since 11.0
+ */
+class AttributeChangeTracker {
+   private boolean isTracking = true;


I wonder if this needs to be volatile or not.

I had it volatile before but I removed: I am almost sure builders and attributes are inherently thread-confined

wburns · 2020-05-06T13:49:47Z

core/src/main/java/org/infinispan/configuration/cache/AttributeChangeTracker.java

+         if (this.isTracking) {
+            String name = attr.name();
+            Object value = attr.get();
+            changedAttributes.add(name.concat("=").concat(value == null ? "" : value.toString()));


Why calling concat here? I would worry this would prevent StringBuilder optimization for +'s.

This is hardly a hotspot, but I will remove the concats :)

wburns · 2020-05-06T13:54:08Z

core/src/main/java/org/infinispan/configuration/cache/EncodingConfigurationBuilder.java

@@ -65,6 +82,11 @@ public EncodingConfiguration create() {

   @Override
   public void validate(GlobalConfiguration globalConfig) {
+      if (mediaType != null) {
+         CONFIG.ignoringSpecificMediaTypes();


This seems a bit odd to me. We already set the key and value metadatatype in mediaType. And also shouldn't we only log this if the key or value media types were explicitly defined?

~~Why? This mediatype is just a cascade switch to set both key and value at the same time (less typing), and it takes precedence over anything that is set individually.~~

Ok, I realized it was setting twice, removing...

And also shouldn't we only log this if the key or value media types were explicitly defined?

Yes, I will change it, otherwise the yellow lines will keep popping up in the console...

wburns · 2020-05-06T14:16:29Z

core/src/main/java/org/infinispan/configuration/cache/MemoryConfiguration.java

+   /**
+    * @return The configured {@link EvictionStrategy}.
+    */
+   public EvictionStrategy whenFull() {


This isn't needed. We should be able to just maintain the eviction strategy in the MemoryConfiguration(Builder) directly and the MemoryStorageConfiguration(Builder) just invokes the parent when setting or retrieving the value.

I have a feeling we can do the same with size.

That is unless there was a reason we had to have separate config values for all of theses? But I would think that should reduce the complexity of the validate and other methods. This is what I did originally when changing eviction to memory element. https://github.com/infinispan/infinispan/blob/9.4.x/core/src/main/java/org/infinispan/configuration/cache/EvictionConfigurationBuilder.java#L82

You can also maintain the value in the child to see if it was set or not.

This specific name when-full was a suggestion from @danberindei in the JIRA I had forgot to add. I think it was quite a expressive attribute name.

I separated on purpose the attributes to be able to detect when using legacy and new configs together, and throw an exception when they are mixed.
The problem I found with syncing attributes one by one in the "setter" is that the JSON parser sets the Attribute values directly, and there's a read method that also sets attributes directly.
Some other places recycle builders, meaning they create a Configuration from the builder, then re-feed the Configuration into the builder's read and then build again (one or more times). So yes, it's a bit complex but the config system is full of back doors...

wburns · 2020-05-06T14:20:51Z

core/src/main/java/org/infinispan/configuration/serializing/Serializer.java

@@ -250,6 +248,7 @@ private void writeCacheContainer(XMLExtendedStreamWriter writer, ConfigurationHo
            default:
               break;
         }
+         writeExtraConfiguration(writer, config.modules());


We may want to include this in a separate PR. I just worry if this is tested, since xml schema validation can be ordered.

Curious that I did not change it. Intellij did this for some bizarre reason...

It was probably a stale merge from adff9c9

gustavocoding · 2020-05-06T17:42:40Z

Updated

wburns · 2020-05-08T16:23:39Z

I must admit I am not a fan of the whole legacy config stuff, it seems overly complex for very little benefit, but I don't want that to hold this PR up.

wburns · 2020-05-08T16:23:56Z

Integrated into master, thanks @gustavonalle !

gustavocoding · 2020-05-08T17:41:54Z

Thanks @wburns, I am not a fan either. I did tried the simpler approach, but the fact the some new [and sometimes overlapping] configs were introduced, and the way the JSON parsing currently works (attributes only, bypassing builder methods) made it messy.
So when can BINARY be finally removed? Infinispan 14 minimum?

wburns requested changes Apr 29, 2020

View reviewed changes

tristantarrant added the Needs Rebase label May 3, 2020

gustavocoding force-pushed the ISPN-11424_deprecate_storagetype branch from 1e6493b to 4b0caa5 Compare May 6, 2020 09:45

gustavocoding removed the Needs Rebase label May 6, 2020

wburns reviewed May 6, 2020

View reviewed changes

ISPN-11424 Deprecate StorageType Configuration

e8786e6

gustavocoding force-pushed the ISPN-11424_deprecate_storagetype branch from 4b0caa5 to e8786e6 Compare May 6, 2020 17:42

wburns merged commit 8150e1d into infinispan:master May 8, 2020

	public boolean offHeap() {
	public boolean isStoredOffHeap() {

	Eviction occurs either the amount of memory exceeds the maximum size.
	Eviction occurs after the approximate memory usage exceeds the maximum size.

	Defines the max number of entries in the data container, before eviction occurs.
	Defines the size of the data container by a count of entries. Eviction occurs after the container size exceeds the maximum count.

ISPN-11424 Deprecate StorageType Configuration #8240

ISPN-11424 Deprecate StorageType Configuration #8240

Conversation

gustavocoding commented Apr 24, 2020

wburns 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

gustavocoding May 5, 2020 • edited

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

wburns Apr 29, 2020 • edited

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

gustavocoding Apr 29, 2020 • edited

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

gustavocoding commented Apr 29, 2020 • edited

tristantarrant commented May 3, 2020

gustavocoding commented May 6, 2020 • edited

wburns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gustavocoding May 6, 2020 • edited

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

gustavocoding May 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gustavocoding May 6, 2020 • edited

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

gustavocoding commented May 6, 2020

wburns commented May 8, 2020

wburns commented May 8, 2020

gustavocoding commented May 8, 2020

gustavocoding May 5, 2020 •

edited

wburns Apr 29, 2020 •

edited

gustavocoding Apr 29, 2020 •

edited

gustavocoding commented Apr 29, 2020 •

edited

gustavocoding commented May 6, 2020 •

edited

gustavocoding May 6, 2020 •

edited

gustavocoding May 6, 2020 •

edited

gustavocoding May 6, 2020 •

edited