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

remove ResetWriteBatch #141

Merged
merged 1 commit into from
Jan 17, 2025
Merged

remove ResetWriteBatch #141

merged 1 commit into from
Jan 17, 2025

Conversation

pfi79
Copy link
Contributor

@pfi79 pfi79 commented Jan 16, 2025

@pfi79 pfi79 requested a review from a team as a code owner January 16, 2025 19:46
@pfi79 pfi79 requested review from denyeart and C0rWin January 16, 2025 19:51
@C0rWin
Copy link

C0rWin commented Jan 16, 2025

I am fine with the PR code-wise. But given that @denyeart and @bestbeforetoday are opposed to having this method, it makes sense to wait for their review.

@pfi79 pfi79 mentioned this pull request Jan 16, 2025
Copy link
Contributor

@denyeart denyeart left a comment

Choose a reason for hiding this comment

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

This looks good to me, just one question to make sure we are on the same page...

Comment on lines +547 to +549
func (h *Handler) handleStartWriteBatch(channelID string, txID string) {
if !h.usePeerWriteBatch {
return errors.New("peer does not support write batch")
return
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the intent here is that chaincode developer can call StartWriteBatch(), and it will work regardless of whether peer supports it or not.
If supported, requests will be batched.
If not supported, requests will be sent individually (consistent with existing behavior).

That seems like the right approach to allow for hybrid environments where some peers may support it, while other peers do not yet support it.

Did I get that right?

Copy link
Member

Choose a reason for hiding this comment

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

It might be worth clarifying that behavior in the godoc for StartWriteBatch.

I notice that the existing godoc for StartWriteBatch reads:

StartWriteBatch enables a mode where all changes are not immediately forwarded to the feast

Perhaps feast was supposed to read peer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the intent here is that chaincode developer can call StartWriteBatch(), and it will work regardless of whether peer supports it or not. If supported, requests will be batched. If not supported, requests will be sent individually (consistent with existing behavior).

That seems like the right approach to allow for hybrid environments where some peers may support it, while other peers do not yet support it.

Did I get that right?

Yeah. That's right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be worth clarifying that behavior in the godoc for StartWriteBatch.

I notice that the existing godoc for StartWriteBatch reads:

StartWriteBatch enables a mode where all changes are not immediately forwarded to the feast

Perhaps feast was supposed to read peer?

That's right. Thank you for finding out. I fixed it.

Signed-off-by: Fedor Partanskiy <[email protected]>
Copy link
Contributor

@denyeart denyeart left a comment

Choose a reason for hiding this comment

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

Thanks @pfi79 , looks good, I'll go ahead and merge.

@denyeart denyeart merged commit 384513e into hyperledger:main Jan 17, 2025
6 checks passed
@pfi79 pfi79 deleted the batch-operations branch January 17, 2025 04:59
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.

4 participants