Hi, comments inline Pedro On 05/13/2015 05:42 PM, Tristan Tarrant wrote: > Hi all, > > this is a list of proposed changes for 8.x that affect configuration and > default behaviour. We built this list last year, but never acted upon > it, but now I believe it's time to do these changes. Please > comment/add/remove as you think is appropriate. > > - Make IGNORE_RETURN_VALUES the default hmmm, not sure about it. it only makes sense for put(k,v) and remove(k,v) since all other write operations are conditional (i.e. need the previous value and no optimization can be made)

> - Drop asymmetric marshalling [1]

> - Remove/change some transaction options [2] > - ReplicationQueue - one should not be allowed to specify it as an instance > - ReplicationQueue - enabled should be implicit ⇔ interval > 0 || > queueSize > 0. Also flushing when queueSize has been reached should > happen async. Can we remove the ReplicationQueue? First, it does not have any benefit (JGroups already bundles the message and the Network can do it too) and second, it is not more efficient (when the message is delivered, we process the commands sequentially. So, if the first command blocks, the other commands are not processed until it finished). Third, it is complex if you have commands with multiple delivers orders (no order, FIFO, total)

> - AsyncStoreConfiguration.shutdownTimeout - > force-immediate=”false[default]” (destructive) > .flushLockTimeout - remove as it is no longer used > - BackupConfigurationBuilder - remove getters and implicitly set > BackupFailurePolicy > - allow setting failurePolicy by class not by string > - Reconsider usefulness of ClusterLoader didn't get this one ^

> - DataContainer.properties - to be removed so, we will not allow any custom implementation to be configured? or I miss something? > - ExpirationConfiguration.reaperEnabled should be removed and the > interval value used instead > - HashConfiguration - remove deprecated elements

> - L1Configuration to fail if the reaper frequency interval is less or > equal than 0 > - LockingConfigurationBuilder.useLockStriping - remove it & > writeSkewCheck enabled by default and removed

write skew check should be moved to Transactions. It is the only place in which it makes sense

> - SingletonStore - pushStateWhenCoordinator implicit by pushStateTimeout > -> flushTimeout > - StateTransfer.fetchInMemoryState -> renamed to enabled

> - StoreAsBinary - remove “enabled” and reply on storeKey/storeBinary + > remove deprecation > - remove deprecations from TransactionConfiguration and remove > use1PcForAutCommitTx

> - VersioningConfiguration.enabled should be computed based on the scheme > and removed

> - Version - remove the version fields and populate them through a mvn script > - SerializationConfigurationBuilder - consider removing the version field

> - ShutdownConfiguration - to be removed, confirm with an email > - TransportConfiguration.distributedSyncTimeout -> global RPC timeout

> - strictPeerToPeer should go as it is deprecated

> another suggestion: - Transaction, remove the transaction_protocol and add TOTAL as an option for locking_mode *or* - Transaction, remove the transaction_protocol - Locking, add locking mode with LOCK and TOTAL_ORDER options (since I'll probably implement total order based non transactional caches)

> > [1] https://issues.jboss.org/browse/ISPN-2939 > [2] https://issues.jboss.org/browse/ISPN-3927 >

> On 14 May 2015, at 16:40, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > On Wed, May 13, 2015 at 8:07 PM, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >> Hi, >> >> comments inline >> >> Pedro >> >> On 05/13/2015 05:42 PM, Tristan Tarrant wrote: >>> Hi all, >>> >>> this is a list of proposed changes for 8.x that affect configuration and >>> default behaviour. We built this list last year, but never acted upon >>> it, but now I believe it's time to do these changes. Please >>> comment/add/remove as you think is appropriate. >>> >>> - Make IGNORE_RETURN_VALUES the default >> >> hmmm, not sure about it. it only makes sense for put(k,v) and >> remove(k,v) since all other write operations are conditional (i.e. need >> the previous value and no optimization can be made) > > For putIfAbsent(k, v) it also helps if you don't have to return the > previous value, and I would expect most write operations to be put(k, > v) anyway. AFAIK, the main objective of IGNORE_RETURN_VALUES has been to avoid having to check what the previous value was. Even if putIfAbsent() would not return the previous value, it needs to check if put happens for the first time, so there's an attempt to retrieve the previous value, if there's any, from the cluster.

> On the other hand, why not adopt the JCache interface and stop implementing Map? ^ The Java 8 API I've been working on tries to make this easier to achieve (and update will be available in the next week or so!), but I don't think we can abandon ConcurrentMap just yet...

> >> >>> - Drop asymmetric marshalling [1] > > +1, but it should be *asynchronous* marshalling. > >>> - Remove/change some transaction options [2] >>> - ReplicationQueue - one should not be allowed to specify it as an instance >>> - ReplicationQueue - enabled should be implicit ⇔ interval > 0 || >>> queueSize > 0. Also flushing when queueSize has been reached should >>> happen async. >> >> Can we remove the ReplicationQueue? First, it does not have any benefit >> (JGroups already bundles the message and the Network can do it too) and >> second, it is not more efficient (when the message is delivered, we >> process the commands sequentially. So, if the first command blocks, the >> other commands are not processed until it finished). Third, it is >> complex if you have commands with multiple delivers orders (no order, >> FIFO, total) > > +1, the replication queue has the same purpose as the bundler in > JGroups. And while the JGroups bundler has multiple options that keep > evolving, our replication queue still has the same algorithm it had in > 4.0. > >> >>> - AsyncStoreConfiguration.shutdownTimeout - >>> force-immediate=”false[default]” (destructive) >>> .flushLockTimeout - remove as it is no longer used >>> - BackupConfigurationBuilder - remove getters and implicitly set >>> BackupFailurePolicy >>> - allow setting failurePolicy by class not by string >>> - Reconsider usefulness of ClusterLoader >> >> didn't get this one ^ > > -1 from me to remove the ClusterLoader, unless we replace it with a > better integrated alternative for invalidation mode. > >> >>> - DataContainer.properties - to be removed >> >> so, we will not allow any custom implementation to be configured? or I >> miss something? >> >>> - ExpirationConfiguration.reaperEnabled should be removed and the >>> interval value used instead >>> - HashConfiguration - remove deprecated elements > > Here I would suggest also replacing the Hash with another interface: > https://issues.jboss.org/browse/ISPN-5465 > >>> - L1Configuration to fail if the reaper frequency interval is less or >>> equal than 0 >>> - LockingConfigurationBuilder.useLockStriping - remove it & >>> writeSkewCheck enabled by default and removed >> > > +1 to remove lock striping > >> write skew check should be moved to Transactions. It is the only place >> in which it makes sense > > +1 to enable writeSkewCheck by default > +1 to remove it from the locking configuration. But I think "write > skew disabled" should be another locking mode, perhaps called > LAST_WRITE_WINS. > >> >>> - SingletonStore - pushStateWhenCoordinator implicit by pushStateTimeout >>> -> flushTimeout >>> - StateTransfer.fetchInMemoryState -> renamed to enabled > > +1 > >>> - StoreAsBinary - remove “enabled” and reply on storeKey/storeBinary + >>> remove deprecation >>> - remove deprecations from TransactionConfiguration and remove >>> use1PcForAutCommitTx > > +1 > >>> - VersioningConfiguration.enabled should be computed based on the scheme >>> and removed > > +1 > >>> - Version - remove the version fields and populate them through a mvn script >>> - SerializationConfigurationBuilder - consider removing the version field > > +1 to remove it, the javadoc there suggests we support > interoperability between different Infinispan versions and that's > still too onerous for us at this point. > >>> - ShutdownConfiguration - to be removed, confirm with an email >>> - TransportConfiguration.distributedSyncTimeout -> global RPC timeout > > +1 > >>> - strictPeerToPeer should go as it is deprecated > > +1 > >>> >> >> another suggestion: >> >> - Transaction, remove the transaction_protocol and add TOTAL as an >> option for locking_mode >> >> *or* >> >> - Transaction, remove the transaction_protocol >> - Locking, add locking mode with LOCK and TOTAL_ORDER options (since >> I'll probably implement total order based non transactional caches) > > I don't feel like TOTAL belongs to the locking element. Since total > order is only relevant in clustered caches, how about making it an > alternative clustering mode? > > <clustering mode="SYNC|ASYNC|TOTAL"/> > > >> >>> >>> [1] https://issues.jboss.org/browse/ISPN-2939 >>> [2] https://issues.jboss.org/browse/ISPN-3927 >>> > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño galder(a)redhat.com _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

> On 22 May 2015, at 09:54, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > On Fri, May 22, 2015 at 9:41 AM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >> >>> On 14 May 2015, at 16:40, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>> >>> On Wed, May 13, 2015 at 8:07 PM, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >>>> Hi, >>>> >>>> comments inline >>>> >>>> Pedro >>>> >>>> On 05/13/2015 05:42 PM, Tristan Tarrant wrote: >>>>> Hi all, >>>>> >>>>> this is a list of proposed changes for 8.x that affect configuration and >>>>> default behaviour. We built this list last year, but never acted upon >>>>> it, but now I believe it's time to do these changes. Please >>>>> comment/add/remove as you think is appropriate. >>>>> >>>>> - Make IGNORE_RETURN_VALUES the default >>>> >>>> hmmm, not sure about it. it only makes sense for put(k,v) and >>>> remove(k,v) since all other write operations are conditional (i.e. need >>>> the previous value and no optimization can be made) >>> >>> For putIfAbsent(k, v) it also helps if you don't have to return the >>> previous value, and I would expect most write operations to be put(k, >>> v) anyway. >> >> AFAIK, the main objective of IGNORE_RETURN_VALUES has been to avoid having to check what the previous value was. Even if putIfAbsent() would not return the previous value, it needs to check if put happens for the first time, so there's an attempt to retrieve the previous value, if there's any, from the cluster. > > In transactions, yes, you still have to retrieve the value from the > owners, because the originator makes the decision. But in a > non-transactional cache, the primary owner wouldn't have to ship the > previous value to the originator. Well, but the primary owner would need to ship information to the originator on whether the cache entry exists or not. Sure, there's a difference between sending back a Boolean vs a V if the V is large, but you still pay the penalty of network. > >> >>> On the other hand, why not adopt the JCache interface and stop implementing Map? >> >> ^ The Java 8 API I've been working on tries to make this easier to achieve (and update will be available in the next week or so!), but I don't think we can abandon ConcurrentMap just yet... >> > > Ok, I might have exaggerated with "stop implementing Map", but > IGNORE_RETURN_VALUES won't matter if we make JCache the > default/recommended way to access the cache in 8.0. > >>> >>>> >>>>> - Drop asymmetric marshalling [1] >>> >>> +1, but it should be *asynchronous* marshalling. >>> >>>>> - Remove/change some transaction options [2] >>>>> - ReplicationQueue - one should not be allowed to specify it as an instance >>>>> - ReplicationQueue - enabled should be implicit ⇔ interval > 0 || >>>>> queueSize > 0. Also flushing when queueSize has been reached should >>>>> happen async. >>>> >>>> Can we remove the ReplicationQueue? First, it does not have any benefit >>>> (JGroups already bundles the message and the Network can do it too) and >>>> second, it is not more efficient (when the message is delivered, we >>>> process the commands sequentially. So, if the first command blocks, the >>>> other commands are not processed until it finished). Third, it is >>>> complex if you have commands with multiple delivers orders (no order, >>>> FIFO, total) >>> >>> +1, the replication queue has the same purpose as the bundler in >>> JGroups. And while the JGroups bundler has multiple options that keep >>> evolving, our replication queue still has the same algorithm it had in >>> 4.0. >>> >>>> >>>>> - AsyncStoreConfiguration.shutdownTimeout - >>>>> force-immediate=”false[default]” (destructive) >>>>> .flushLockTimeout - remove as it is no longer used >>>>> - BackupConfigurationBuilder - remove getters and implicitly set >>>>> BackupFailurePolicy >>>>> - allow setting failurePolicy by class not by string >>>>> - Reconsider usefulness of ClusterLoader >>>> >>>> didn't get this one ^ >>> >>> -1 from me to remove the ClusterLoader, unless we replace it with a >>> better integrated alternative for invalidation mode. >>> >>>> >>>>> - DataContainer.properties - to be removed >>>> >>>> so, we will not allow any custom implementation to be configured? or I >>>> miss something? >>>> >>>>> - ExpirationConfiguration.reaperEnabled should be removed and the >>>>> interval value used instead >>>>> - HashConfiguration - remove deprecated elements >>> >>> Here I would suggest also replacing the Hash with another interface: >>> https://issues.jboss.org/browse/ISPN-5465 >>> >>>>> - L1Configuration to fail if the reaper frequency interval is less or >>>>> equal than 0 >>>>> - LockingConfigurationBuilder.useLockStriping - remove it & >>>>> writeSkewCheck enabled by default and removed >>>> >>> >>> +1 to remove lock striping >>> >>>> write skew check should be moved to Transactions. It is the only place >>>> in which it makes sense >>> >>> +1 to enable writeSkewCheck by default >>> +1 to remove it from the locking configuration. But I think "write >>> skew disabled" should be another locking mode, perhaps called >>> LAST_WRITE_WINS. >>> >>>> >>>>> - SingletonStore - pushStateWhenCoordinator implicit by pushStateTimeout >>>>> -> flushTimeout >>>>> - StateTransfer.fetchInMemoryState -> renamed to enabled >>> >>> +1 >>> >>>>> - StoreAsBinary - remove “enabled” and reply on storeKey/storeBinary + >>>>> remove deprecation >>>>> - remove deprecations from TransactionConfiguration and remove >>>>> use1PcForAutCommitTx >>> >>> +1 >>> >>>>> - VersioningConfiguration.enabled should be computed based on the scheme >>>>> and removed >>> >>> +1 >>> >>>>> - Version - remove the version fields and populate them through a mvn script >>>>> - SerializationConfigurationBuilder - consider removing the version field >>> >>> +1 to remove it, the javadoc there suggests we support >>> interoperability between different Infinispan versions and that's >>> still too onerous for us at this point. >>> >>>>> - ShutdownConfiguration - to be removed, confirm with an email >>>>> - TransportConfiguration.distributedSyncTimeout -> global RPC timeout >>> >>> +1 >>> >>>>> - strictPeerToPeer should go as it is deprecated >>> >>> +1 >>> >>>>> >>>> >>>> another suggestion: >>>> >>>> - Transaction, remove the transaction_protocol and add TOTAL as an >>>> option for locking_mode >>>> >>>> *or* >>>> >>>> - Transaction, remove the transaction_protocol >>>> - Locking, add locking mode with LOCK and TOTAL_ORDER options (since >>>> I'll probably implement total order based non transactional caches) >>> >>> I don't feel like TOTAL belongs to the locking element. Since total >>> order is only relevant in clustered caches, how about making it an >>> alternative clustering mode? >>> >>> <clustering mode="SYNC|ASYNC|TOTAL"/> >>> >>> >>>> >>>>> >>>>> [1] https://issues.jboss.org/browse/ISPN-2939 >>>>> [2] https://issues.jboss.org/browse/ISPN-3927 >>>>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> >> -- >> Galder Zamarreño >> galder(a)redhat.com >> >> >> >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño galder(a)redhat.com _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev