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

BFT Block Puller: updatable connection source #4571

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

tock-ibm
Copy link
Contributor

Type of change

  • New feature

Description

Make the connection source update when a config block is processed by the block puller, not when it is committed.

Related issues

#4347

@tock-ibm tock-ibm requested a review from a team as a code owner December 11, 2023 07:44
@tock-ibm tock-ibm marked this pull request as draft December 11, 2023 07:50
@tock-ibm tock-ibm force-pushed the bft-bp-block-conn-source branch 10 times, most recently from 754fb6c to d0f795d Compare December 12, 2023 15:58
@tock-ibm tock-ibm force-pushed the bft-bp-block-conn-source branch 4 times, most recently from 6612036 to 4682dae Compare January 3, 2024 13:42
@tock-ibm tock-ibm marked this pull request as ready for review January 3, 2024 13:45
Signed-off-by: Yoav Tock <tock@il.ibm.com>
Change-Id: Id0b80df892595cf2b9d7b7b50bfa9069f0eb6905
@@ -0,0 +1,55 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we have any util package in our entire codebase we can move this function to?

Copy link
Contributor Author

@tock-ibm tock-ibm Jan 7, 2024

Choose a reason for hiding this comment

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

It got promoted from orderer/common/cluster/util.go to common/deliverclient/util.go, where I plan to put all the stuff that is common to both orderer and peer in the context of the block puller. Can't put it in package common/util nor protoutil because of import cycles. I am open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the import cycle is because of github.com/hyperledger/fabric/common/configtx? Then can't you move it there?

Copy link
Contributor

Choose a reason for hiding this comment

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

However if you think there are additional things to be placed here, I guess we can leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of import cycles it can be placed in common/configtx, but I think it is not a good place to put code that parses blocks, as that package deals only with individual txs. I prefer to leave it where it is for the time being. I think that during the refactoring that is about to happen when we overhaul the orderer, several additional things will get moved into that util.go file.

@yacovm yacovm merged commit 6d6986a into hyperledger:main Jan 8, 2024
14 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.

2 participants