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

Allow the peer delivery client to select policy #4856

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

semil
Copy link
Contributor

@semil semil commented May 12, 2024

Type of change

  • Improvement (improvement to code, performance, etc)

Description

Issue: #4847

Related issues

#4847

Copy link
Contributor

@tock-ibm tock-ibm left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just a few minor comments.
We'll need to add an integration test that checks that this indeed works (another PR). We can use the same approach is in the orderer.ReplicationPolicy: copy a simple test from the smartbft test suite (that uses a peer) and simply change to Policy="simple".

@@ -52,6 +54,7 @@ type DeliverServiceConfig struct {
// OrdererEndpointOverrides is a map of orderer addresses which should be
// re-mapped to a different orderer endpoint.
OrdererEndpointOverrides map[string]*orderers.Endpoint
Policy string
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment with an explanation, same as in the yaml file.

case "BFT":
d.blockDeliverer, err = d.createBlockDelivererBFT(chainID, ledgerInfo)
switch d.conf.DeliverServiceConfig.Policy {
case "cluster", "":
Copy link
Contributor

Choose a reason for hiding this comment

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

at this stage the policy should not be "", as we det the default "cluster" much earlier.

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 is needed for the unit tests, they do not go through the code that "patches" this case, so we need the code here in the delivery client itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it unit test you should check that if the policy is not set you get an error... IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed :)

Comment on lines 147 to 149
if createCftDeliverer {
d.blockDeliverer, err = d.createBlockDelivererCFT(chainID, ledgerInfo)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why not simply call this function from 2 places? it will save 4 line of code, no? (and would be more readable in my opinion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct :)

@semil semil force-pushed the issue-4847 branch 2 times, most recently from fa34b0a to 2f799e2 Compare June 4, 2024 08:24
@tock-ibm tock-ibm force-pushed the issue-4847 branch 2 times, most recently from b77e65d to a3987ba Compare June 4, 2024 13:36
Emil Elizarov and others added 2 commits June 4, 2024 19:40
…Delivere

Signed-off-by: Emil Elizarov <emil.elizarov@ibm.com>
Signed-off-by: Yoav Tock <tock@il.ibm.com>
@denyeart denyeart merged commit 62e1af9 into hyperledger:main Jun 4, 2024
13 checks passed
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.

None yet

3 participants