Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support WriteBatch operations #5113

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pfi79
Copy link
Contributor

@pfi79 pfi79 commented Jan 21, 2025

@pfi79 pfi79 requested a review from a team as a code owner January 21, 2025 19:52
@pfi79 pfi79 requested review from C0rWin and denyeart January 21, 2025 19:53
@pfi79
Copy link
Contributor Author

pfi79 commented Jan 21, 2025

In the e2e integration tests, benchmark test has been added and 2 speed tables appear with and without the use of batch.

image image

@pfi79 pfi79 force-pushed the batch-operation branch 2 times, most recently from 31c3dbf to 9776b45 Compare January 23, 2025 06:05
)
})

func RunInvokeOnlyPutState(n *nwo.Network, orderer *nwo.Orderer, peer *nwo.Peer, fn string, startBatch bool, initial int) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a more expressive variable name than initial? It is not obvious what this is...

And shouldn't startBatch really be called useWriteBatch to be consistent?

Copy link
Contributor Author

@pfi79 pfi79 Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. In this case it is passing a sign that the chaincode should call startBatch or not.

	startWB, _ := strconv.ParseBool(args[0])
	if startWB {
		stub.StartWriteBatch()
	}

Copy link
Contributor Author

@pfi79 pfi79 Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna run these tests

useWriteBatch (on peer) StartWriteBatch (on chaincode)
true false
false true
true true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


switch function {
case "invoke":
return t.put(stub, args[1:])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a single argument is passed right? e.g. args[1] , better to use a variable with an expressive name to make it clear what exactly is being passed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

},
Entry("without write batch", "without write batch", false),
Entry("with write batch", "with write batch", true),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to read at least one of the keys in a chaincode query after writing the transaction, to ensure end-to-end behavior is consistent regardless of useWriteBatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -659,6 +659,14 @@ chaincode:
# Format for the chaincode container logs
format: "%{color}%{time:2006-01-02 15:04:05.000 MST} [%{module}] %{shortfunc} -> %{level:.4s} %{id:03x}%{color:reset} %{message}"

# AdditionalParams section for parameters that are passed to chaincode
# to specify additional operation modes in which the peer operates.
additionalParams:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe runtimeParams would be a better name than additionalParams in the config.
I realize the proto is called ChaincodeAdditionalParams, but we can give it a more expressive name in the context of the config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@pfi79 pfi79 force-pushed the batch-operation branch 4 times, most recently from 52b3b8c to 4790b27 Compare January 25, 2025 05:48
Signed-off-by: Fedor Partanskiy <[email protected]>
@pfi79 pfi79 requested a review from denyeart January 25, 2025 06:24
Copy link
Contributor

@C0rWin C0rWin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, while I think we should take care to add tests to make sure behaviour remain consistent as @manish-sethi mentioned is his comment hyperledger/fabric-rfcs#58 (comment)

@pfi79
Copy link
Contributor Author

pfi79 commented Jan 28, 2025

Overall looks good to me, while I think we should take care to add tests to make sure behaviour remain consistent as @manish-sethi mentioned is his comment hyperledger/fabric-rfcs#58 (comment)

Thanks @C0rWin
I'm willing to finish any tests.
But I'm not quite sure what those tests should be.
If they give me scenarios, I'd be happy to implement them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants