add write batch

[pfi79]

hyperledger/fabric#5086

[pfi79]

@denyeart @C0rWin @satota2 @yacovm

[C0rWin]

While reading the PR, I thought we might miss an API that can abort or cancel the collecting batch. For instance, there might be logic where the user reads something from the state and decides not to send anything or perform a different batch of operations.

Wdyt @pfi79? @denyeart, if you think it might make sense, do we need to update the RFC to reflect it, or can we implement a suggested API, say, CancelWriteBatch()?

[pfi79]

While reading the PR, I thought we might miss an API that can abort or cancel the collecting batch. For instance, there might be logic where the user reads something from the state and decides not to send anything or perform a different batch of operations.

Wdyt @pfi79? @denyeart, if you think it might make sense, do we need to update the RFC to reflect it, or can we implement a suggested API, say, CancelWriteBatch()?

Let's talk about it. I wrote this change so that I don't have to change the chaincode for different peers. That is, those that support WriteBatch and those that do not (flag usePeerWriteBatch true or false). And I think that behaviour should not change, maximum in processing speed.

What will happen to your proposal if the old peer is next to the new chaincode?

In my variant, the call to StartWriteBatch will not fail, but it will be ignored and the sending of changes will follow the old one. What should CancelWriteBatch() do in this case?

[pfi79]

The thing is, at first there was the concept of just reducing transaction processing time. And now everything is ready for this requirement.

If we accept the proposal of the esteemed @C0rWin, then here, in addition to reducing the processing time, we are also changing the logic of transaction processing.
It will be necessary to make small changes. Make the StartWriteBatch function return an error if sending by batches is not supported

[pfi79]

While reading the PR, I thought we might miss an API that can abort or cancel the collecting batch. For instance, there might be logic where the user reads something from the state and decides not to send anything or perform a different batch of operations.

Wdyt @pfi79? @denyeart, if you think it might make sense, do we need to update the RFC to reflect it, or can we implement a suggested API, say, CancelWriteBatch()?

Changed StartWriteBatch
Added ResetWriteBatch

[C0rWin]

LGTM

[bestbeforetoday]

I don’t really understand the CancelWriteBatch requirement. Why would a smart contract perform some writes, then change its mind, discard the writes and do something different, all within a single transaction invocation? Why would a transaction function not just gather the required information and make a decision on how it wants to behave before performing any writes?

I liked the initial implementation where the same smart contract code could run regardless of whether the peer supported the write batching optimisation.

The documentation should state that the batch operations will return an error if batching is not supported. Is it then OK to ignore that error and continue, or does the transaction function need to fail? The documentation should be clear on the required behaviour.

[pfi79]

I don’t really understand the CancelWriteBatch requirement. Why would a smart contract perform some writes, then change its mind, discard the writes and do something different, all within a single transaction invocation? Why would a transaction function not just gather the required information and make a decision on how it wants to behave before performing any writes?

I liked the initial implementation where the same smart contract code could run regardless of whether the peer supported the write batching optimisation.

The documentation should state that the batch operations will return an error if batching is not supported. Is it then OK to ignore that error and continue, or does the transaction function need to fail? The documentation should be clear on the required behaviour.

Great. I'm willing to discuss some more and, if necessary, rescind my changes or add to them.

What you are proposing, if I understand you correctly:

1. Remove the functionality of ResetWriteBatch() - CancelWriteBatch

or

2. change StartWriteBatch() error, for example to such StartWriteBatch() (isSupportWriteBatch bool)

These two suggestions are mutually exclusive. Which one do you insist on?

[bestbeforetoday]

I don't insist on any changes. I would like to understand the value of ResetWriteBatch(). Also the semantics around failed batching.

[pfi79]

I liked the initial implementation where the same smart contract code could run regardless of whether the peer supported the write batching optimisation.

That's still a possibility right now.
In my chaincodes, I will insert the StartWriteBatch call at the beginning and that's it.
But in the future maybe more flexible management of the batch will be more relevant than it seems now.

The documentation should state that the batch operations will return an error if batching is not supported. Is it then OK to ignore that error and continue, or does the transaction function need to fail? The documentation should be clear on the required behaviour.

I think your remark about the error returned in the StartWriteBatch method is completely fair. It seems to me that StartWriteBatch should return a bool sign of support for writing a patch.

[C0rWin]

I don’t really understand the CancelWriteBatch requirement. Why would a smart contract perform some writes, then change its mind, discard the writes and do something different, all within a single transaction invocation? Why would a transaction function not just gather the required information and make a decision on how it wants to behave before performing any writes?

OK, this is not a cancel per se, but more like a reset or clear... When you start doing a batching, it collects internally all update operations to be transferred in batch, so what if, in some cases, you might want to put a different set of instructions based on the get/read operation? How would you do it? So, in order to provide developers with the ability to change the set of put operations, I suggested adding a reset.

I liked the initial implementation where the same smart contract code could run regardless of whether the peer supported the write batching optimisation.

Agree.

The documentation should state that the batch operations will return an error if batching is not supported. Is it then OK to ignore that error and continue, or does the transaction function need to fail? The documentation should be clear on the required behaviour.

Well, it's a developer's choice what to do in error, right? If you understand the semantics of the operation, you should be able to decide whether to break and exit with error or to continue. For instance, changing it from returning an error to having a boolean return results, IMO won't change too much as the developer will still have to make a decision whether to continue or to fail the entire execution. But if you think it will make it more consistent with current APIs, I am also fine with it.

[C0rWin]

I don't insist on any changes. I would like to understand the value of ResetWriteBatch(). Also the semantics around failed batching.

The key purpose is to enable the developer to change the content of the batched operations based on some conditional logic. For instance, you string doing a batch then performing a read operation and realize you might need a different set of put operations to be provided, w/p reset or cancel; you have no option to do it.

@bestbeforetoday, at any rate, I saw @pfi79 provided an alternative approach moving away from returning an error to return a boolean value to indicate whenever batching support is enabled.

[bestbeforetoday]

I still don't understand the value of being able to discard previously batched writes within a single transaction invocation. If you're expecting to change what information will be written based on read information, why not just read the information before doing the writes and then make the correct set of writes?

There is certainly a cost in expanding the API complexity so there should be real end-user value to make that worthwhile.

[C0rWin]

I still don't understand the value of being able to discard previously batched writes within a single transaction invocation. If you're expecting to change what information will be written based on read information, why not just read the information before doing the writes and then make the correct set of writes?

There is certainly a cost in expanding the API complexity so there should be real end-user value to make that worthwhile.

That approach significantly narrows down the way you do your program... That covers if you would like to batch, read all keys you might need, and then do your logic with updates. Having this reset functionality, you can have a more generalized approach without restricting developers on the way they should use batching.

[denyeart]

Sorry for the delayed response, but I am also skeptical of ResetWriteBatch(). I've never heard an actual requirement for it, just a theoretical argument that somebody may want it. I can't think of an actual smart contract use case for it. Usually smart contracts check some set of conditions first, and if they meet expectations, then perform some writes. Performing a set of writes and then deciding to perform a different set of writes seems unlikely. I'd prefer to keep the API as simple as possible (remove ResetWriteBatch), it can always be added later if there is a concrete need for it.

[C0rWin]

Sorry for the delayed response, but I am also skeptical of ResetWriteBatch(). I've never heard an actual requirement for it, just a theoretical argument that somebody may want it. I can't think of an actual smart contract use case for it. Usually smart contracts check some set of conditions first, and if they meet expectations, then perform some writes. Performing a set of writes and then deciding to perform a different set of writes seems unlikely. I'd prefer to keep the API as simple as possible (remove ResetWriteBatch), it can always be added later if there is a concrete need for it.

That's a fair comment, though I do think we better have it rather than not or add it later on. But I am also fine with another approach.

[denyeart]

I'm pretty confident we'll never add it, just saying that we have the option to add later if I'm wrong :-)

[C0rWin]

I'm pretty confident we'll never add it, just saying that we have the option to add later if I'm wrong :-)

I think that having the option to control the way you fill the batch and being able to change based on conditional logic and execution context is something I'd like to see as a chaincode developer. Currently, it is very limited, once the batch is started, the only options you have are to either send it to the peer or abort chaincode execution. But I can understand your conservative approach.

[pfi79]

Sorry for the delayed response, but I am also skeptical of ResetWriteBatch(). I've never heard an actual requirement for it, just a theoretical argument that somebody may want it. I can't think of an actual smart contract use case for it. Usually smart contracts check some set of conditions first, and if they meet expectations, then perform some writes. Performing a set of writes and then deciding to perform a different set of writes seems unlikely. I'd prefer to keep the API as simple as possible (remove ResetWriteBatch), it can always be added later if there is a concrete need for it.

ok. I removed ResetWriteBatch
#141