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.

Sign up for GitHub

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

Jump to bottom

Disable peer.deliveryclient.blockGossipEnabled in sample core.yaml #4911

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Disable peer.deliveryclient.blockGossipEnabled in sample core.yaml #4911

merged 1 commit into from
Jun 26, 2024

Conversation

denyeart
Copy link
Contributor

@denyeart denyeart commented Jun 25, 2024 •
edited
Loading

Since v2.2 it has been recommended to configure all peers to be orgLeaders so that they pull blocks from ordering service.

In v2.4 peer.deliveryclient.blockGossipEnabled was added and recommended to be set to false so that peers don't gossip pulled blocks.

These settings simplify peer behavior and reduce communication between peers (at the expense of more connections to ordering service).

Finally in v3.0 peer.deliveryclient.blockGossipEnabled is set to false by default.

This change also updates core.yaml and documentation to make these recommendations clear and consistent.

Closes #3961

@denyeart denyeart requested review from a team as code owners June 25, 2024 19:39
@denyeart
Copy link
Contributor Author

@yacovm Let me know if I got the gossip doc correct...

@denyeart
Copy link
Contributor Author

@pfi79 I've seen this SmartBFT integration test fail multiple times, I think you have worked on it in the past, any ideas?

2024-06-25T20:18:37.1493918Z   �[38;5;9m[FAILED] Timed out after 120.001s.
2024-06-25T20:18:37.1494015Z   Expected
2024-06-25T20:18:37.1494280Z       <<-chan struct {} | len:0, cap:0>: 0xc000db4360
2024-06-25T20:18:37.1494419Z   to be closed�[0m
2024-06-25T20:18:37.1495252Z   �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/home/runner/work/fabric/fabric/integration/smartbft/smartbft_test.go:1941�[0m �[38;5;243m@ 06/25/24 20:18:36.848�[0m
2024-06-25T20:18:37.1495431Z �[38;5;243m------------------------------�[0m

2024-06-25T20:18:37.1514659Z   �[38;5;9m[FAIL]�[0m �[0mEndToEnd Smart BFT configuration test �[38;5;243msmartbft network �[38;5;9m�[1m[It] send an update transaction to each orderer and deleting failed requests�[0m
2024-06-25T20:18:37.1515131Z   �[38;5;243m/home/runner/work/fabric/fabric/integration/smartbft/smartbft_test.go:1941�[0m

@pfi79
Copy link
Contributor

pfi79 commented Jun 26, 2024

@pfi79 I've seen this SmartBFT integration test fail multiple times, I think you have worked on it in the past, any ideas?

2024-06-25T20:18:37.1493918Z   �[38;5;9m[FAILED] Timed out after 120.001s.
2024-06-25T20:18:37.1494015Z   Expected
2024-06-25T20:18:37.1494280Z       <<-chan struct {} | len:0, cap:0>: 0xc000db4360
2024-06-25T20:18:37.1494419Z   to be closed�[0m
2024-06-25T20:18:37.1495252Z   �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/home/runner/work/fabric/fabric/integration/smartbft/smartbft_test.go:1941�[0m �[38;5;243m@ 06/25/24 20:18:36.848�[0m
2024-06-25T20:18:37.1495431Z �[38;5;243m------------------------------�[0m

2024-06-25T20:18:37.1514659Z   �[38;5;9m[FAIL]�[0m �[0mEndToEnd Smart BFT configuration test �[38;5;243msmartbft network �[38;5;9m�[1m[It] send an update transaction to each orderer and deleting failed requests�[0m
2024-06-25T20:18:37.1515131Z   �[38;5;243m/home/runner/work/fabric/fabric/integration/smartbft/smartbft_test.go:1941�[0m

Yeah, I'm already looking for errors

yacovm
yacovm reviewed Jun 26, 2024
View reviewed changes
sampleconfig/core.yaml
# Note that 'gossip.state.enabled' controls point to point block replication
# of blocks committed in the past.
blockGossipEnabled: true
blockGossipEnabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you even need to put it false here? maybe comment it out?

Copy link
Contributor Author

@denyeart denyeart Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why do you even need to put it false here? maybe comment it out?

If it is not set it defaults to true, see https://github.com/hyperledger/fabric/blob/main/core/deliverservice/config.go#L133-L138

We want it to default to false. It will be more consistent for users and maintainers if the logic stays the same across release branches, with just a simple default value change that we can mention in the v3.0 release notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe like for bft just ban it here https://github.com/hyperledger/fabric/blob/main/core/deliverservice/deliveryclient.go#L253-L255

Copy link
Contributor

Choose a reason for hiding this comment

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

We want it to default to false. It will be more consistent for users and maintainers if the logic stays the same across release branches, with just a simple default value change that we can mention in the v3.0 release notes.

Agreed

@yacovm
Copy link
Contributor

yacovm commented Jun 26, 2024

@yacovm Let me know if I got the gossip doc correct...

Seems right. Wow, this is so legacy...

@pfi79
Copy link
Contributor

pfi79 commented Jun 26, 2024 •
edited
Loading

@pfi79 I've seen this SmartBFT integration test fail multiple times, I think you have worked on it in the past, any ideas?

2024-06-25T20:18:37.1493918Z   �[38;5;9m[FAILED] Timed out after 120.001s.
2024-06-25T20:18:37.1494015Z   Expected
2024-06-25T20:18:37.1494280Z       <<-chan struct {} | len:0, cap:0>: 0xc000db4360
2024-06-25T20:18:37.1494419Z   to be closed�[0m
2024-06-25T20:18:37.1495252Z   �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/home/runner/work/fabric/fabric/integration/smartbft/smartbft_test.go:1941�[0m �[38;5;243m@ 06/25/24 20:18:36.848�[0m
2024-06-25T20:18:37.1495431Z �[38;5;243m------------------------------�[0m

2024-06-25T20:18:37.1514659Z   �[38;5;9m[FAIL]�[0m �[0mEndToEnd Smart BFT configuration test �[38;5;243msmartbft network �[38;5;9m�[1m[It] send an update transaction to each orderer and deleting failed requests�[0m
2024-06-25T20:18:37.1515131Z   �[38;5;243m/home/runner/work/fabric/fabric/integration/smartbft/smartbft_test.go:1941�[0m

@denyeart Done. Please apply the changes from main. Or restart actions.

@denyeart
Copy link
Contributor Author

@Mergifyio rebase

@denyeart
Disable peer.deliveryclient.blockGossipEnabled in sample core.yaml
f4618e6 
Since v2.2 it has been recommended to configure all peers to be orgLeaders
so that they pull blocks from ordering service.

In v2.4 peer.deliveryclient.blockGossipEnabled was added and recommended
to be set to false so that peers don't gossip pulled blocks.

These settings simplify peer behavior and reduce communication between peers
(at the expense of more connections to ordering service).

Finally in v3.0 peer.deliveryclient.blockGossipEnabled is set to false by default.

This change also updates core.yaml and documentation to make these
recommendations clear and consistent.

Closes hyperledger#3961

Signed-off-by: David Enyeart <enyeart@us.ibm.com>
@mergify Mergify
Copy link

mergify bot commented Jun 26, 2024

rebase

✅ Branch has been successfully rebased

@yacovm yacovm merged commit 08aefb4 into hyperledger:main Jun 26, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfi79 pfi79 pfi79 left review comments

@yacovm yacovm yacovm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable peer.deliveryclient.blockGossipEnabled in sample core.yaml
3 participants
@denyeart @pfi79 @yacovm