Hi guys, we discussed about this a little bit in the past and this morning on IRC. Here are some proposed removals: - Remove the async transactional modes, as they are quite pointless - Remove batching: users should use transactions

- Remove the tree module: it doesn't work properly, and uses batching Please cast your votes Tristan

On 20-02-2017 16:12, Bela Ban wrote: > > > On 20/02/17 17:06, Tristan Tarrant wrote: >> Hi guys, we discussed about this a little bit in the past and this >> morning on IRC. Here are some proposed removals: >> >> - Remove the async transactional modes, as they are quite pointless >> - Remove batching: users should use transactions > > How do you make a bunch of modifications and send them asynchronously if > both batching *and* async TXs are getting removed? We are not removing features, we are removing broken code. Batching is using transactions and async transactions doesn't make sense since infinispan has to report to TransactionManager. Our current asyn-tx is broken in a way that is starts to commit and reports OK to the transaction manager. if you have a topology change or a conflict, you will end with inconsistent data. So, why do we keeping this code around? you can still move a transaction commit to another thread if you don't wanna wait for its commit: tm.begin() doWork() tx = tm.suspend() new_thread { tm.resume(tx) tm.commit() } The best thing I can think of is to keep the batching API and re-implement it to provide an endBatchAsync() that will do the above. > > So if someone wants to apply a unit of work *atomically* (either all > modifications within that unit of work are applied, or none), what > alternatives exist? > >> - Remove the tree module: it doesn't work properly, and uses batching >> >> Please cast your votes >> >> Tristan >> > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Hello. May I ask what's wrong with tree module? I work on a project which depends on this module heavily. I hope it is not a huge problem to reimplement tree module or at least some part of it if someone will tell me where to start :) -- Denis On 20.02.2017 23:02, Sanne Grinovero wrote: > -1 to batch removal > > Frankly I've always been extremely negative about the fact that > batches are built on top of transactions. It's easy to find several > iterations of rants of mine on this mailing list, especially fierce > debates with Mircea. So I'd welcome a separation of these features. > > Yet, removing batching seems very wrong. I disagree that people should > use Transactions instead; for many use cases it's overkill, and for > others - and this is the main pain point I always had with the current > design - it might make sense to have a batch (or more than one) within > a transaction. > I have had practical problems with needing to "flush" a single batch > within a transaction as the size of the combined elements was getting > too large. That doesn't imply at all that I'm ready to commit. > > @Pedro: the fact that some code is broken today is not relevant, when > there's need for such features. Like you suggest, it had bad premises > (build it on TX) so we should address that, but not throw it all out. > > @Bela is making spot-on objections based on use cases, which need to > be addressed in some way. The "atomical" operations still don't work > right[1] in Infinispan and that's a big usability problem. > > +1 to remove async TX > > I actually like the concept but the API should be different.. might as > well remove this for now. > > +1 to remove the Tree module > > I personally never used it as you all advised against, yet it's been > often requested by users; sometimes explicitly and others not so > explicit, yet people often have need which would be solved by a good > Tree module. > I understand the reasons you all want to remove it: it's buggy. But I > believe the real reason is that it should have been built on top of a > proper batch API, and using real MVCC. In that case it wouldn't have > been buggy, nor too hard to maintain, and might have attracted way > more interest as well. > I'd remove it as a temporary measure: delete the bad stuff, but > hopefully it could be reintroduced built on better principles in some > future? > > Thanks, > Sanne > > [1] - "right" as in user expectations and actual practical use, which > is currently different than in the twisted definition of "right" that > the team has been using as an excuse ;-) I'll also proof that it > doesn't work right according to your own twisted specs, when I find > the time to finish some tests.. > > > On 20 February 2017 at 16:48, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >> >> On 20-02-2017 16:12, Bela Ban wrote: >>> >>> On 20/02/17 17:06, Tristan Tarrant wrote: >>>> Hi guys, we discussed about this a little bit in the past and this >>>> morning on IRC. Here are some proposed removals: >>>> >>>> - Remove the async transactional modes, as they are quite pointless >>>> - Remove batching: users should use transactions >>> How do you make a bunch of modifications and send them asynchronously if >>> both batching *and* async TXs are getting removed? >> We are not removing features, we are removing broken code. >> >> Batching is using transactions and async transactions doesn't make sense >> since infinispan has to report to TransactionManager. >> >> Our current asyn-tx is broken in a way that is starts to commit and >> reports OK to the transaction manager. if you have a topology change or >> a conflict, you will end with inconsistent data. >> >> So, why do we keeping this code around? >> >> you can still move a transaction commit to another thread if you don't >> wanna wait for its commit: >> >> tm.begin() >> doWork() >> tx = tm.suspend() >> new_thread { >> tm.resume(tx) >> tm.commit() >> } >> >> The best thing I can think of is to keep the batching API and >> re-implement it to provide an endBatchAsync() that will do the above. >> >>> So if someone wants to apply a unit of work *atomically* (either all >>> modifications within that unit of work are applied, or none), what >>> alternatives exist? >>> >>>> - Remove the tree module: it doesn't work properly, and uses batching >>>> >>>> Please cast your votes >>>> >>>> Tristan >>>> >> _______________________________________________ >> 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Tristan, yes, I'd like to help. And the list of issues with this module would be something to start with. Thanks. -- Denis On 21.02.2017 01:13, Tristan Tarrant wrote: > It has always been regarded as buggy and unmaintained and more of a > convenience for users coming from the old JBossCache. > If you are willing to help out, we can list the most important issues. > > Tristan > > On 20/02/17 19:22, Denis V. Kirpichenkov wrote: >> Hello. >> >> May I ask what's wrong with tree module? >> >> I work on a project which depends on this module heavily. I hope it is >> not a huge problem to reimplement tree module or at least some part of >> it if someone will tell me where to start :) >> >> -- >> >> Denis >> >> >> On 20.02.2017 23:02, Sanne Grinovero wrote: >>> -1 to batch removal >>> >>> Frankly I've always been extremely negative about the fact that >>> batches are built on top of transactions. It's easy to find several >>> iterations of rants of mine on this mailing list, especially fierce >>> debates with Mircea. So I'd welcome a separation of these features. >>> >>> Yet, removing batching seems very wrong. I disagree that people should >>> use Transactions instead; for many use cases it's overkill, and for >>> others - and this is the main pain point I always had with the current >>> design - it might make sense to have a batch (or more than one) within >>> a transaction. >>> I have had practical problems with needing to "flush" a single batch >>> within a transaction as the size of the combined elements was getting >>> too large. That doesn't imply at all that I'm ready to commit. >>> >>> @Pedro: the fact that some code is broken today is not relevant, when >>> there's need for such features. Like you suggest, it had bad premises >>> (build it on TX) so we should address that, but not throw it all out. >>> >>> @Bela is making spot-on objections based on use cases, which need to >>> be addressed in some way. The "atomical" operations still don't work >>> right[1] in Infinispan and that's a big usability problem. >>> >>> +1 to remove async TX >>> >>> I actually like the concept but the API should be different.. might as >>> well remove this for now. >>> >>> +1 to remove the Tree module >>> >>> I personally never used it as you all advised against, yet it's been >>> often requested by users; sometimes explicitly and others not so >>> explicit, yet people often have need which would be solved by a good >>> Tree module. >>> I understand the reasons you all want to remove it: it's buggy. But I >>> believe the real reason is that it should have been built on top of a >>> proper batch API, and using real MVCC. In that case it wouldn't have >>> been buggy, nor too hard to maintain, and might have attracted way >>> more interest as well. >>> I'd remove it as a temporary measure: delete the bad stuff, but >>> hopefully it could be reintroduced built on better principles in some >>> future? >>> >>> Thanks, >>> Sanne >>> >>> [1] - "right" as in user expectations and actual practical use, which >>> is currently different than in the twisted definition of "right" that >>> the team has been using as an excuse ;-) I'll also proof that it >>> doesn't work right according to your own twisted specs, when I find >>> the time to finish some tests.. >>> >>> >>> On 20 February 2017 at 16:48, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >>>> On 20-02-2017 16:12, Bela Ban wrote: >>>>> On 20/02/17 17:06, Tristan Tarrant wrote: >>>>>> Hi guys, we discussed about this a little bit in the past and this >>>>>> morning on IRC. Here are some proposed removals: >>>>>> >>>>>> - Remove the async transactional modes, as they are quite pointless >>>>>> - Remove batching: users should use transactions >>>>> How do you make a bunch of modifications and send them asynchronously if >>>>> both batching *and* async TXs are getting removed? >>>> We are not removing features, we are removing broken code. >>>> >>>> Batching is using transactions and async transactions doesn't make sense >>>> since infinispan has to report to TransactionManager. >>>> >>>> Our current asyn-tx is broken in a way that is starts to commit and >>>> reports OK to the transaction manager. if you have a topology change or >>>> a conflict, you will end with inconsistent data. >>>> >>>> So, why do we keeping this code around? >>>> >>>> you can still move a transaction commit to another thread if you don't >>>> wanna wait for its commit: >>>> >>>> tm.begin() >>>> doWork() >>>> tx = tm.suspend() >>>> new_thread { >>>> tm.resume(tx) >>>> tm.commit() >>>> } >>>> >>>> The best thing I can think of is to keep the batching API and >>>> re-implement it to provide an endBatchAsync() that will do the above. >>>> >>>>> So if someone wants to apply a unit of work *atomically* (either all >>>>> modifications within that unit of work are applied, or none), what >>>>> alternatives exist? >>>>> >>>>>> - Remove the tree module: it doesn't work properly, and uses batching >>>>>> >>>>>> Please cast your votes >>>>>> >>>>>> Tristan >>>>>> >>>> _______________________________________________ >>>> 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 >> _______________________________________________ >> 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

On 20/02/17 19:02, Sanne Grinovero wrote: > -1 to batch removal > > Frankly I've always been extremely negative about the fact that > batches are built on top of transactions. I think the discussion is pointless without clearing up what the expected semantics of a batch should be and what the expected advantages over individual invocations should be. A batch is just a glorified putAll which also supports removes. All write ops are queued locally and are then sent in groups to the respective owners. What you get is deferred invocations and 1 remote invocation per unique owner. What you don't get is atomicity and isolation. You should use transactions for that.

Tristan -- Tristan Tarrant Infinispan Lead JBoss, a division of Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Mon, Feb 20, 2017 at 8:02 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: > -1 to batch removal > > Frankly I've always been extremely negative about the fact that > batches are built on top of transactions. It's easy to find several > iterations of rants of mine on this mailing list, especially fierce > debates with Mircea. So I'd welcome a separation of these features. > > Yet, removing batching seems very wrong. I disagree that people should > use Transactions instead; for many use cases it's overkill, and for > others - and this is the main pain point I always had with the current > design - it might make sense to have a batch (or more than one) within > a transaction. > I have had practical problems with needing to "flush" a single batch > within a transaction as the size of the combined elements was getting > too large. That doesn't imply at all that I'm ready to commit. > WDYM by "flush" here? I have a feeling this is nothing like our batching ever was...

> @Pedro: the fact that some code is broken today is not relevant, when > there's need for such features. Like you suggest, it had bad premises > (build it on TX) so we should address that, but not throw it all out. > Infinispan never created nested batches:

calling startBatch() when a batch was already associated with the current thread just incremented a refcount, and only the final endBatch() did any work. OTOH running a batch within a transaction always worked very much like suspending the current transaction, starting a new one, and committing it on endBatch(). So the only real difference between batching and using DummyTransactionManager is that batching is limited to one cache's operations, while DummyTransactionManager supports multiple resources. > @Bela is making spot-on objections based on use cases, which need to > be addressed in some way. The "atomical" operations still don't work > right[1] in Infinispan and that's a big usability problem. > Batching never was about sending updates asynchronously. We have putAllAsync() for that, which doesn't need transactions, and it's even slightly more efficient without transactions.

And atomical operations have bugs, yes, but I'm not sure how implementing a new kind of batching that isn't based on transactions would help with that. > +1 to remove async TX > > I actually like the concept but the API should be different.. might as > well remove this for now. > > +1 to remove the Tree module > > I personally never used it as you all advised against, yet it's been > often requested by users; sometimes explicitly and others not so > explicit, yet people often have need which would be solved by a good > Tree module. > I understand the reasons you all want to remove it: it's buggy. But I > believe the real reason is that it should have been built on top of a > proper batch API, and using real MVCC. In that case it wouldn't have > been buggy, nor too hard to maintain, and might have attracted way > more interest as well. I think the fact that we haven't been able to build a "proper" batch API using real MVCC yet is a proof to the contrary... > I'd remove it as a temporary measure: delete the bad stuff, but > hopefully it could be reintroduced built on better principles in some > future? > > Thanks, > Sanne > > [1] - "right" as in user expectations and actual practical use, which > is currently different than in the twisted definition of "right" that > the team has been using as an excuse ;-) I'll also proof that it > doesn't work right according to your own twisted specs, when I find > the time to finish some tests.. > > > On 20 February 2017 at 16:48, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >> >> >> On 20-02-2017 16:12, Bela Ban wrote: >>> >>> >>> On 20/02/17 17:06, Tristan Tarrant wrote: >>>> Hi guys, we discussed about this a little bit in the past and this >>>> morning on IRC. Here are some proposed removals: >>>> >>>> - Remove the async transactional modes, as they are quite pointless >>>> - Remove batching: users should use transactions >>> >>> How do you make a bunch of modifications and send them asynchronously if >>> both batching *and* async TXs are getting removed? >> >> We are not removing features, we are removing broken code. >> >> Batching is using transactions and async transactions doesn't make sense >> since infinispan has to report to TransactionManager. >> >> Our current asyn-tx is broken in a way that is starts to commit and >> reports OK to the transaction manager. if you have a topology change or >> a conflict, you will end with inconsistent data. >> >> So, why do we keeping this code around? >> >> you can still move a transaction commit to another thread if you don't >> wanna wait for its commit: >> >> tm.begin() >> doWork() >> tx = tm.suspend() >> new_thread { >> tm.resume(tx) >> tm.commit() >> } >> >> The best thing I can think of is to keep the batching API and >> re-implement it to provide an endBatchAsync() that will do the above. >> >>> >>> So if someone wants to apply a unit of work *atomically* (either all >>> modifications within that unit of work are applied, or none), what >>> alternatives exist? >>> >>>> - Remove the tree module: it doesn't work properly, and uses batching >>>> >>>> Please cast your votes >>>> >>>> Tristan >>>> >>> >> _______________________________________________ >> 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 02/21/2017 09:39 AM, Sanne Grinovero wrote: > On 21 February 2017 at 07:37, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> On Mon, Feb 20, 2017 at 8:02 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: >>> -1 to batch removal >>> >>> Frankly I've always been extremely negative about the fact that >>> batches are built on top of transactions. It's easy to find several >>> iterations of rants of mine on this mailing list, especially fierce >>> debates with Mircea. So I'd welcome a separation of these features. >>> >>> Yet, removing batching seems very wrong. I disagree that people should >>> use Transactions instead; for many use cases it's overkill, and for >>> others - and this is the main pain point I always had with the current >>> design - it might make sense to have a batch (or more than one) within >>> a transaction. >>> I have had practical problems with needing to "flush" a single batch >>> within a transaction as the size of the combined elements was getting >>> too large. That doesn't imply at all that I'm ready to commit. >>> >> WDYM by "flush" here? I have a feeling this is nothing like our >> batching ever was... > I'm just listing points about why incorporating the batch concept with > transactions is not practical. > > I mean that when one has to write very large amounts of data, you need to > break it up in smaller batches; *essentially* to flush the first batch before > starting the second one. > So that is unrelated with atomicity and consistency, as in practice one has > to split one business operation into multiple batches. You haven't explained what "flush" means. Since you separate that from atomicity/consistency, I assume that batches on non-tx cache are just ordered putOrRemoveAll operations, immediately visible on flush without any atomicity. If you want batches in transactions, you probably mean that the changes are not visible until tx commits. So you are worried that all data modified within this transaction won't fit into single node memory? Or is there more to that? What's a "nested batch", then? I could see some benefits in removing repeatable-reads cached values.

>>> @Pedro: the fact that some code is broken today is not relevant, when >>> there's need for such features. Like you suggest, it had bad premises >>> (build it on TX) so we should address that, but not throw it all out. >>> >> Infinispan never created nested batches: > I know. I'm not describing the current implementation, I'm describing use > cases which are not addressed, or in which Infinispan is clumsy to use, > to suggest improvements. > And I'm not asking to have nested batches. Again, just pointing > out practical problems with the current batch design. > > It should be possible to run an efficient batch of operations within a > transaction. > Or a sequence of batches, all within the same transaction. > N.B. you could see that as a "nested batch" in the current twisted idea that > a batch is a transaction - which is what I'm arguing against. > > >> calling startBatch() when a >> batch was already associated with the current thread just incremented >> a refcount, and only the final endBatch() did any work. OTOH running a >> batch within a transaction always worked very much like suspending the >> current transaction, starting a new one, and committing it on >> endBatch(). So the only real difference between batching and using >> DummyTransactionManager is that batching is limited to one cache's >> operations, while DummyTransactionManager supports multiple resources. >> >> >>> @Bela is making spot-on objections based on use cases, which need to >>> be addressed in some way. The "atomical" operations still don't work >>> right[1] in Infinispan and that's a big usability problem. >>> >> Batching never was about sending updates asynchronously. We have >> putAllAsync() for that, which doesn't need transactions, and it's even >> slightly more efficient without transactions. > If you think this way, it sounds like you give no value to the application > performance: people need more than a couple put operations. Removes? I am really asking out of curiosity, I hope these questions don't sound ironically. R. > >> And atomical operations have bugs, yes, but I'm not sure how >> implementing a new kind of batching that isn't based on transactions >> would help with that. >> >> >>> +1 to remove async TX >>> >>> I actually like the concept but the API should be different.. might as >>> well remove this for now. >>> >>> +1 to remove the Tree module >>> >>> I personally never used it as you all advised against, yet it's been >>> often requested by users; sometimes explicitly and others not so >>> explicit, yet people often have need which would be solved by a good >>> Tree module. >>> I understand the reasons you all want to remove it: it's buggy. But I >>> believe the real reason is that it should have been built on top of a >>> proper batch API, and using real MVCC. In that case it wouldn't have >>> been buggy, nor too hard to maintain, and might have attracted way >>> more interest as well. >> I think the fact that we haven't been able to build a "proper" batch >> API using real MVCC yet is a proof to the contrary... >> >> >>> I'd remove it as a temporary measure: delete the bad stuff, but >>> hopefully it could be reintroduced built on better principles in some >>> future? >>> >>> Thanks, >>> Sanne >>> >>> [1] - "right" as in user expectations and actual practical use, which >>> is currently different than in the twisted definition of "right" that >>> the team has been using as an excuse ;-) I'll also proof that it >>> doesn't work right according to your own twisted specs, when I find >>> the time to finish some tests.. >>> >>> >>> On 20 February 2017 at 16:48, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >>>> >>>> On 20-02-2017 16:12, Bela Ban wrote: >>>>> >>>>> On 20/02/17 17:06, Tristan Tarrant wrote: >>>>>> Hi guys, we discussed about this a little bit in the past and this >>>>>> morning on IRC. Here are some proposed removals: >>>>>> >>>>>> - Remove the async transactional modes, as they are quite pointless >>>>>> - Remove batching: users should use transactions >>>>> How do you make a bunch of modifications and send them asynchronously if >>>>> both batching *and* async TXs are getting removed? >>>> We are not removing features, we are removing broken code. >>>> >>>> Batching is using transactions and async transactions doesn't make sense >>>> since infinispan has to report to TransactionManager. >>>> >>>> Our current asyn-tx is broken in a way that is starts to commit and >>>> reports OK to the transaction manager. if you have a topology change or >>>> a conflict, you will end with inconsistent data. >>>> >>>> So, why do we keeping this code around? >>>> >>>> you can still move a transaction commit to another thread if you don't >>>> wanna wait for its commit: >>>> >>>> tm.begin() >>>> doWork() >>>> tx = tm.suspend() >>>> new_thread { >>>> tm.resume(tx) >>>> tm.commit() >>>> } >>>> >>>> The best thing I can think of is to keep the batching API and >>>> re-implement it to provide an endBatchAsync() that will do the above. >>>> >>>>> So if someone wants to apply a unit of work *atomically* (either all >>>>> modifications within that unit of work are applied, or none), what >>>>> alternatives exist? >>>>> >>>>>> - Remove the tree module: it doesn't work properly, and uses batching >>>>>> >>>>>> Please cast your votes >>>>>> >>>>>> Tristan >>>>>> >>>> _______________________________________________ >>>> 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 >> _______________________________________________ >> 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 -- Radim Vansa <rvansa(a)redhat.com&gt; JBoss Performance Team _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 21/02/17 16:29, Sanne Grinovero wrote: >> You haven't explained what "flush" means. Since you separate that from >> atomicity/consistency, I assume that batches on non-tx cache are just >> ordered putOrRemoveAll operations, immediately visible on flush without >> any atomicity. I assume that in Sanne's idea, ordering in a batch doesn't matter, aside from operations on the same key. Having ordering in there would for example not allow us to parallelize by segment. > So I want to write a first chunk, in our code that looks like: > > startBatch > put(chunk1/A, [some large value]) > put(chunk1/B, [some small metadata]) > put(chunk1/C, [some small metadata]) > endBatch > There is no reason to use a transaction, in fact we had to disable > transactions as some of these entries could be large. > There also is no reason for the batch, other than optimising the latency. Let me summarize to see if we have the requirements for a useful batching system (which is sort of patterned on the JDBC statement batching): - a batch is not an atomic operation, i.e. it is not backed by a transaction - it can be wrapped in a transaction if needed - batches cannot be nested - batches only involve unconditional write operations (put, putAll, remove) - ordering of operations within a batch is unimportant aside from modifications to the same key where we apply "last one wins" - when a batch is "flushed" (i.e. endBatch is invoked) the ops are grouped by segment and sent to the appropriate owner for processing, potentially in parallel As Radim has called it, this is essentially a putOrRemoveAll op (with an async counterpart). Is that summary correct ?

Tristan -- Tristan Tarrant Infinispan Lead JBoss, a division of Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 21/02/17 16:29, Sanne Grinovero wrote: >> You haven't explained what "flush" means. Since you separate that from >> atomicity/consistency, I assume that batches on non-tx cache are just >> ordered putOrRemoveAll operations, immediately visible on flush without >> any atomicity. I assume that in Sanne's idea, ordering in a batch doesn't matter, aside from operations on the same key. Having ordering in there would for example not allow us to parallelize by segment. > So I want to write a first chunk, in our code that looks like: > > startBatch > put(chunk1/A, [some large value]) > put(chunk1/B, [some small metadata]) > put(chunk1/C, [some small metadata]) > endBatch > There is no reason to use a transaction, in fact we had to disable > transactions as some of these entries could be large. > There also is no reason for the batch, other than optimising the latency. Let me summarize to see if we have the requirements for a useful batching system (which is sort of patterned on the JDBC statement batching): - a batch is not an atomic operation, i.e. it is not backed by a transaction - it can be wrapped in a transaction if needed - batches cannot be nested - batches only involve unconditional write operations (put, putAll, remove) - ordering of operations within a batch is unimportant aside from modifications to the same key where we apply "last one wins" - when a batch is "flushed" (i.e. endBatch is invoked) the ops are grouped by segment and sent to the appropriate owner for processing, potentially in parallel As Radim has called it, this is essentially a putOrRemoveAll op (with an async counterpart).

Is that summary correct ? Tristan

But doesn't the functional API's evalMany() provide something very close to the API you're suggesting? Dan On Tue, Feb 21, 2017 at 7:55 PM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > On 02/21/2017 05:16 PM, Tristan Tarrant wrote: >> On 21/02/17 16:29, Sanne Grinovero wrote: >>>> You haven't explained what "flush" means. Since you separate that from >>>> atomicity/consistency, I assume that batches on non-tx cache are just >>>> ordered putOrRemoveAll operations, immediately visible on flush without >>>> any atomicity. >> I assume that in Sanne's idea, ordering in a batch doesn't matter, aside >> from operations on the same key. Having ordering in there would for >> example not allow us to parallelize by segment. >> >>> So I want to write a first chunk, in our code that looks like: >>> >>> startBatch >>> put(chunk1/A, [some large value]) >>> put(chunk1/B, [some small metadata]) >>> put(chunk1/C, [some small metadata]) >>> endBatch >>> There is no reason to use a transaction, in fact we had to disable >>> transactions as some of these entries could be large. >>> There also is no reason for the batch, other than optimising the latency. >> Let me summarize to see if we have the requirements for a useful >> batching system (which is sort of patterned on the JDBC statement batching): >> >> - a batch is not an atomic operation, i.e. it is not backed by a transaction >> - it can be wrapped in a transaction if needed >> - batches cannot be nested >> - batches only involve unconditional write operations (put, putAll, remove) >> - ordering of operations within a batch is unimportant aside from >> modifications to the same key where we apply "last one wins" >> - when a batch is "flushed" (i.e. endBatch is invoked) the ops are >> grouped by segment and sent to the appropriate owner for processing, >> potentially in parallel >> >> As Radim has called it, this is essentially a putOrRemoveAll op (with an >> async counterpart). > It is putOrRemoveAll when applied on a non-tx cache, and actually > implementing that shouldn't be complex. However, when transactions come > into play, it is different, because Sanne wants us to remove the > modifications in completed batch from the local transactional invocation > context and 'cache' them on the owners. Since reads have to be > transactionally consistent, we need to inspect the transaction on the > remote nodes (remote repeatable read). > > Sanne's request makes sense to me. However as the current implementation > is providing a false assumption that it could work as JDBC batches while > it's nothing but crippled JTA, and as I don't see anyone shouting "I'll > implement that, next week it's done!", I second deprecating/removing the > API for the time being. > Exactly my thoughts, it's definitely not an unreasonable request, but it would require a lot of work to implement correctly. > I don't find the current API ideal either, as it depends on thread > locals (JTA does as well, but...) while it does not seem useful enough > to me. I would prefer > > interface BatchingCache { > Batch start(); > } > > @NotThreadSafe > interface Batch { > void put(K k, V value); > ... > void execute(); > void drop(); // maybe not needed > } > I was hoping the functional API's evalMany() would be good enough, but I see now that it replicates the function argument (which holds all the values) everywhere. So putAll() is still more efficient, unless using groups to make sure all keys in the batch have the same owners.

Dan _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Tue, Feb 21, 2017 at 8:28 PM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: > On 02/21/2017 07:14 PM, Dan Berindei wrote: >> But doesn't the functional API's evalMany() provide something very >> close to the API you're suggesting? >> >> Dan >> >> >> On Tue, Feb 21, 2017 at 7:55 PM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>> On 02/21/2017 05:16 PM, Tristan Tarrant wrote: >>>> On 21/02/17 16:29, Sanne Grinovero wrote: >>>>>> You haven't explained what "flush" means. Since you separate that from >>>>>> atomicity/consistency, I assume that batches on non-tx cache are just >>>>>> ordered putOrRemoveAll operations, immediately visible on flush without >>>>>> any atomicity. >>>> I assume that in Sanne's idea, ordering in a batch doesn't matter, aside >>>> from operations on the same key. Having ordering in there would for >>>> example not allow us to parallelize by segment. >>>> >>>>> So I want to write a first chunk, in our code that looks like: >>>>> >>>>> startBatch >>>>> put(chunk1/A, [some large value]) >>>>> put(chunk1/B, [some small metadata]) >>>>> put(chunk1/C, [some small metadata]) >>>>> endBatch >>>>> There is no reason to use a transaction, in fact we had to disable >>>>> transactions as some of these entries could be large. >>>>> There also is no reason for the batch, other than optimising the latency. >>>> Let me summarize to see if we have the requirements for a useful >>>> batching system (which is sort of patterned on the JDBC statement batching): >>>> >>>> - a batch is not an atomic operation, i.e. it is not backed by a transaction >>>> - it can be wrapped in a transaction if needed >>>> - batches cannot be nested >>>> - batches only involve unconditional write operations (put, putAll, remove) >>>> - ordering of operations within a batch is unimportant aside from >>>> modifications to the same key where we apply "last one wins" >>>> - when a batch is "flushed" (i.e. endBatch is invoked) the ops are >>>> grouped by segment and sent to the appropriate owner for processing, >>>> potentially in parallel >>>> >>>> As Radim has called it, this is essentially a putOrRemoveAll op (with an >>>> async counterpart). >>> It is putOrRemoveAll when applied on a non-tx cache, and actually >>> implementing that shouldn't be complex. However, when transactions come >>> into play, it is different, because Sanne wants us to remove the >>> modifications in completed batch from the local transactional invocation >>> context and 'cache' them on the owners. Since reads have to be >>> transactionally consistent, we need to inspect the transaction on the >>> remote nodes (remote repeatable read). >>> >>> Sanne's request makes sense to me. However as the current implementation >>> is providing a false assumption that it could work as JDBC batches while >>> it's nothing but crippled JTA, and as I don't see anyone shouting "I'll >>> implement that, next week it's done!", I second deprecating/removing the >>> API for the time being. >>> >> Exactly my thoughts, it's definitely not an unreasonable request, but >> it would require a lot of work to implement correctly. >> >> >>> I don't find the current API ideal either, as it depends on thread >>> locals (JTA does as well, but...) while it does not seem useful enough >>> to me. I would prefer >>> >>> interface BatchingCache { >>> Batch start(); >>> } >>> >>> @NotThreadSafe >>> interface Batch { >>> void put(K k, V value); >>> ... >>> void execute(); >>> void drop(); // maybe not needed >>> } >>> >> I was hoping the functional API's evalMany() would be good enough, but >> I see now that it replicates the function argument (which holds all >> the values) everywhere. So putAll() is still more efficient, unless >> using groups to make sure all keys in the batch have the same owners. > Uh, doesn't putAll do the same? Is the modifications list isolate per > target segments when replicating? > I missed the evalMany(Map, BiFunction) overload yesterday, and I assume WriteOnlyMap.evalMany(Map, MarshallableFunctions.setValueConsumer()) would behave exactly the same as putAll(Map). Of course, it's not better either, as you still have to create the map beforehand... OTOH I'm pretty sure you're the one who wrote the code to split the input keys/values by target segments both for putAll() and for evalMany() in NonTxDistributionInterceptor :)

Dan _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 02/22/2017 09:11 AM, Dan Berindei wrote: > On Tue, Feb 21, 2017 at 8:28 PM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >> On 02/21/2017 07:14 PM, Dan Berindei wrote: >>> But doesn't the functional API's evalMany() provide something very >>> close to the API you're suggesting? >>> >>> Dan >>> >>> >>> On Tue, Feb 21, 2017 at 7:55 PM, Radim Vansa <rvansa(a)redhat.com&gt; wrote: >>>> On 02/21/2017 05:16 PM, Tristan Tarrant wrote: >>>>> On 21/02/17 16:29, Sanne Grinovero wrote: >>>>>>> You haven't explained what "flush" means. Since you separate that from >>>>>>> atomicity/consistency, I assume that batches on non-tx cache are just >>>>>>> ordered putOrRemoveAll operations, immediately visible on flush without >>>>>>> any atomicity. >>>>> I assume that in Sanne's idea, ordering in a batch doesn't matter, aside >>>>> from operations on the same key. Having ordering in there would for >>>>> example not allow us to parallelize by segment. >>>>> >>>>>> So I want to write a first chunk, in our code that looks like: >>>>>> >>>>>> startBatch >>>>>> put(chunk1/A, [some large value]) >>>>>> put(chunk1/B, [some small metadata]) >>>>>> put(chunk1/C, [some small metadata]) >>>>>> endBatch >>>>>> There is no reason to use a transaction, in fact we had to disable >>>>>> transactions as some of these entries could be large. >>>>>> There also is no reason for the batch, other than optimising the latency. >>>>> Let me summarize to see if we have the requirements for a useful >>>>> batching system (which is sort of patterned on the JDBC statement batching): >>>>> >>>>> - a batch is not an atomic operation, i.e. it is not backed by a transaction >>>>> - it can be wrapped in a transaction if needed >>>>> - batches cannot be nested >>>>> - batches only involve unconditional write operations (put, putAll, remove) >>>>> - ordering of operations within a batch is unimportant aside from >>>>> modifications to the same key where we apply "last one wins" >>>>> - when a batch is "flushed" (i.e. endBatch is invoked) the ops are >>>>> grouped by segment and sent to the appropriate owner for processing, >>>>> potentially in parallel >>>>> >>>>> As Radim has called it, this is essentially a putOrRemoveAll op (with an >>>>> async counterpart). >>>> It is putOrRemoveAll when applied on a non-tx cache, and actually >>>> implementing that shouldn't be complex. However, when transactions come >>>> into play, it is different, because Sanne wants us to remove the >>>> modifications in completed batch from the local transactional invocation >>>> context and 'cache' them on the owners. Since reads have to be >>>> transactionally consistent, we need to inspect the transaction on the >>>> remote nodes (remote repeatable read). >>>> >>>> Sanne's request makes sense to me. However as the current implementation >>>> is providing a false assumption that it could work as JDBC batches while >>>> it's nothing but crippled JTA, and as I don't see anyone shouting "I'll >>>> implement that, next week it's done!", I second deprecating/removing the >>>> API for the time being. >>>> >>> Exactly my thoughts, it's definitely not an unreasonable request, but >>> it would require a lot of work to implement correctly. >>> >>> >>>> I don't find the current API ideal either, as it depends on thread >>>> locals (JTA does as well, but...) while it does not seem useful enough >>>> to me. I would prefer >>>> >>>> interface BatchingCache { >>>> Batch start(); >>>> } >>>> >>>> @NotThreadSafe >>>> interface Batch { >>>> void put(K k, V value); >>>> ... >>>> void execute(); >>>> void drop(); // maybe not needed >>>> } >>>> >>> I was hoping the functional API's evalMany() would be good enough, but >>> I see now that it replicates the function argument (which holds all >>> the values) everywhere. So putAll() is still more efficient, unless >>> using groups to make sure all keys in the batch have the same owners. >> Uh, doesn't putAll do the same? Is the modifications list isolate per >> target segments when replicating? >> > I missed the evalMany(Map, BiFunction) overload yesterday, and I > assume WriteOnlyMap.evalMany(Map, > MarshallableFunctions.setValueConsumer()) would behave exactly the > same as putAll(Map). Of course, it's not better either, as you still > have to create the map beforehand... > > OTOH I'm pretty sure you're the one who wrote the code to split the > input keys/values by target segments both for putAll() and for > evalMany() in NonTxDistributionInterceptor :) I was referring to "I see now that it replicates the function argument (which holds all the values) everywhere. So putAll() is still more efficient." And besides sending the setValue operation along, I don't see what you mean. * In non-tx mode, evalMany() wraps the entries using ReadOnlySegmentAwareMap which filters segments for given node during marshalling. putAll does exactly the same. * In tx mode, both commands are added to modifications list and this is sent (in PrepareCommand) to all participating nodes, including entries that are not owned by a given node. (that's a space for optimizations). Anyway, while evalMany would be sufficient for non-transactional mode (and batching API would be only a convenience), it is not enough in tx mode as it does not wipe out non-local modifications/reads on originator during the flush.

When implementing tx functional commands, I was considering keeping modifications on remote node during the transaction (before the prepare phase) for repeatable functional reads, but in the end I went with sending all modifications for a given entry along with the read.

On Tue, Feb 21, 2017 at 10:39 AM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: > On 21 February 2017 at 07:37, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> On Mon, Feb 20, 2017 at 8:02 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: >>> -1 to batch removal >>> >>> Frankly I've always been extremely negative about the fact that >>> batches are built on top of transactions. It's easy to find several >>> iterations of rants of mine on this mailing list, especially fierce >>> debates with Mircea. So I'd welcome a separation of these features. >>> >>> Yet, removing batching seems very wrong. I disagree that people should >>> use Transactions instead; for many use cases it's overkill, and for >>> others - and this is the main pain point I always had with the current >>> design - it might make sense to have a batch (or more than one) within >>> a transaction. >>> I have had practical problems with needing to "flush" a single batch >>> within a transaction as the size of the combined elements was getting >>> too large. That doesn't imply at all that I'm ready to commit. >>> >> >> WDYM by "flush" here? I have a feeling this is nothing like our >> batching ever was... > > I'm just listing points about why incorporating the batch concept with > transactions is not practical. > > I mean that when one has to write very large amounts of data, you need to > break it up in smaller batches; *essentially* to flush the first batch before > starting the second one. > So that is unrelated with atomicity and consistency, as in practice one has > to split one business operation into multiple batches. > Kind of repeating Radim's question, but how is this better than having multiple small transactions and committing each one separately?

>> >>> @Pedro: the fact that some code is broken today is not relevant, when >>> there's need for such features. Like you suggest, it had bad premises >>> (build it on TX) so we should address that, but not throw it all out. >>> >> >> Infinispan never created nested batches: > > I know. I'm not describing the current implementation, I'm describing use > cases which are not addressed, or in which Infinispan is clumsy to use, > to suggest improvements. > And I'm not asking to have nested batches. Again, just pointing > out practical problems with the current batch design. > > It should be possible to run an efficient batch of operations within a > transaction. > Or a sequence of batches, all within the same transaction. > N.B. you could see that as a "nested batch" in the current twisted idea that > a batch is a transaction - which is what I'm arguing against. > I'm sure there there are use cases that we don't address properly, but I don't know enough about those use cases or your proposed batching API improvements to really say anything about them. The only thing I'm confident about is that the current batching API is almost certainly not the answer. > >> calling startBatch() when a >> batch was already associated with the current thread just incremented >> a refcount, and only the final endBatch() did any work. OTOH running a >> batch within a transaction always worked very much like suspending the >> current transaction, starting a new one, and committing it on >> endBatch(). So the only real difference between batching and using >> DummyTransactionManager is that batching is limited to one cache's >> operations, while DummyTransactionManager supports multiple resources. >> >> >>> @Bela is making spot-on objections based on use cases, which need to >>> be addressed in some way. The "atomical" operations still don't work >>> right[1] in Infinispan and that's a big usability problem. >>> >> >> Batching never was about sending updates asynchronously. We have >> putAllAsync() for that, which doesn't need transactions, and it's even >> slightly more efficient without transactions. > > If you think this way, it sounds like you give no value to the application > performance: people need more than a couple put operations. > Unfortunately, going beyond simple put operations, e.g. conditional writes, is exactly where asynchronous replication fails. Even doing simple put operations and making sure that those values are written to the cache is an impossible task with asynchronous replication. Considering that the batch methods are called startAtomic() and endAtomic() in TreeCache, I really don't think they were created with asynchronous replication in mind. Off-topic: The BatchingCache#startBatch() javadoc was never true for distributed caches: writes are queued, but reads (or puts without IGNORE_RETURN_VALUES) always result in a remote call. Locks cause remote calls in a cache with pessimistic locking, too, although one could argue that back in version 4.0, locks were indeed acquired locally during the write and remotely at the end of the batch. >> >> And atomical operations have bugs, yes, but I'm not sure how >> implementing a new kind of batching that isn't based on transactions >> would help with that. >> >> >>> +1 to remove async TX >>> >>> I actually like the concept but the API should be different.. might as >>> well remove this for now. >>> >>> +1 to remove the Tree module >>> >>> I personally never used it as you all advised against, yet it's been >>> often requested by users; sometimes explicitly and others not so >>> explicit, yet people often have need which would be solved by a good >>> Tree module. >>> I understand the reasons you all want to remove it: it's buggy. But I >>> believe the real reason is that it should have been built on top of a >>> proper batch API, and using real MVCC. In that case it wouldn't have >>> been buggy, nor too hard to maintain, and might have attracted way >>> more interest as well. >> >> I think the fact that we haven't been able to build a "proper" batch >> API using real MVCC yet is a proof to the contrary... >> >> >>> I'd remove it as a temporary measure: delete the bad stuff, but >>> hopefully it could be reintroduced built on better principles in some >>> future? >>> >>> Thanks, >>> Sanne >>> >>> [1] - "right" as in user expectations and actual practical use, which >>> is currently different than in the twisted definition of "right" that >>> the team has been using as an excuse ;-) I'll also proof that it >>> doesn't work right according to your own twisted specs, when I find >>> the time to finish some tests.. >>> >>> >>> On 20 February 2017 at 16:48, Pedro Ruivo <pedro(a)infinispan.org&gt; wrote: >>>> >>>> >>>> On 20-02-2017 16:12, Bela Ban wrote: >>>>> >>>>> >>>>> On 20/02/17 17:06, Tristan Tarrant wrote: >>>>>> Hi guys, we discussed about this a little bit in the past and this >>>>>> morning on IRC. Here are some proposed removals: >>>>>> >>>>>> - Remove the async transactional modes, as they are quite pointless >>>>>> - Remove batching: users should use transactions >>>>> >>>>> How do you make a bunch of modifications and send them asynchronously if >>>>> both batching *and* async TXs are getting removed? >>>> >>>> We are not removing features, we are removing broken code. >>>> >>>> Batching is using transactions and async transactions doesn't make sense >>>> since infinispan has to report to TransactionManager. >>>> >>>> Our current asyn-tx is broken in a way that is starts to commit and >>>> reports OK to the transaction manager. if you have a topology change or >>>> a conflict, you will end with inconsistent data. >>>> >>>> So, why do we keeping this code around? >>>> >>>> you can still move a transaction commit to another thread if you don't >>>> wanna wait for its commit: >>>> >>>> tm.begin() >>>> doWork() >>>> tx = tm.suspend() >>>> new_thread { >>>> tm.resume(tx) >>>> tm.commit() >>>> } >>>> >>>> The best thing I can think of is to keep the batching API and >>>> re-implement it to provide an endBatchAsync() that will do the above. >>>> >>>>> >>>>> So if someone wants to apply a unit of work *atomically* (either all >>>>> modifications within that unit of work are applied, or none), what >>>>> alternatives exist? >>>>> >>>>>> - Remove the tree module: it doesn't work properly, and uses batching >>>>>> >>>>>> Please cast your votes >>>>>> >>>>>> Tristan >>>>>> >>>>> >>>> _______________________________________________ >>>> 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 >> _______________________________________________ >> 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Hi guys, we discussed about this a little bit in the past and this morning on IRC. Here are some proposed removals: - Remove the async transactional modes, as they are quite pointless - Remove batching: users should use transactions - Remove the tree module: it doesn't work properly, and uses batching Please cast your votes Tristan -- Tristan Tarrant Infinispan Lead JBoss, a division of Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev