Catchup: NetworkFetcher service to export fetchBlock #4388

algoganesh · 2022-08-10T18:24:06Z

Summary

NetworkFetcher is the new struct used to export fetchBlock function from universalFetcher.
createPeerSelector has been partially exported.

codecov · 2022-08-10T18:55:59Z

Codecov Report

Merging #4388 (0a11973) into master (fd488f8) will decrease coverage by 0.01%.
The diff coverage is 74.60%.

@@            Coverage Diff             @@
##           master    #4388      +/-   ##
==========================================
- Coverage   55.34%   55.33%   -0.02%     
==========================================
  Files         400      401       +1     
  Lines       50797    50852      +55     
==========================================
+ Hits        28112    28137      +25     
- Misses      20311    20335      +24     
- Partials     2374     2380       +6

Impacted Files	Coverage Δ
catchup/networkFetcher.go	`70.90% <70.90%> (ø)`
catchup/service.go	`69.38% <100.00%> (+0.74%)`	⬆️
ledger/roundlru.go	`90.56% <0.00%> (-5.67%)`	⬇️
network/wsPeer.go	`65.47% <0.00%> (-2.20%)`	⬇️
crypto/merkletrie/trie.go	`66.42% <0.00%> (-2.19%)`	⬇️
crypto/merkletrie/node.go	`91.62% <0.00%> (-1.87%)`	⬇️
ledger/blockqueue.go	`85.63% <0.00%> (-1.73%)`	⬇️
data/abi/abi_type.go	`87.67% <0.00%> (-0.95%)`	⬇️
ledger/acctonline.go	`79.00% <0.00%> (+0.52%)`	⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

… catchup-networkFetch-interface merge master

algonautshant

Looks very good.
Some comments. The major 2 things are:

The fetcher should have a retry loop
There are 3 criteria for ranking the peer

catchup/networkFetcher.go

algonautshant · 2022-08-11T19:51:32Z

catchup/networkFetcher.go

+)
+
+// NetworkFetcher is the interface used to export fetchBlock function from universalFetcher
+type NetworkFetcher interface {


I don't think you need it as an interface and an implementation.
Just use the implementation. You are not using the interface with a different impl.

catchup/networkFetcher.go

… catchup-networkFetch-interface merge master

algonautshant

Thanks for addressing the comments.
Getting close. A few more comments.

catchup/networkFetcher.go

… catchup-networkFetch-interface

catchup/networkFetcher.go

… catchup-networkFetch-interface

algonautshant

Looks great!
Will it be possible to test all or some of the error branches in the code?

algoganesh · 2022-08-16T20:00:46Z

Looks great! Will it be possible to test all or some of the error branches in the code?

I've added tests to test most of the error branches and increase codecov

algonautshant · 2022-08-17T03:36:04Z

catchup/networkFetcher.go

@@ -52,7 +52,7 @@ func MakeNetworkFetcher(log logging.Logger, net network.GossipNode, cfg config.L
 }

 func (netFetcher *NetworkFetcher) getHTTPPeer() (network.HTTPPeer, *peerSelectorPeer, error) {
-	for retryCount := 0; retryCount < netFetcher.cfg.CatchupBlockDownloadRetryAttempts; retryCount++ {
+	for retryCount := 1; retryCount <= netFetcher.cfg.CatchupBlockDownloadRetryAttempts; retryCount++ {


Why is this change?

algonautshant · 2022-08-17T03:37:28Z

catchup/networkFetcher.go

@@ -79,7 +79,7 @@ func (netFetcher *NetworkFetcher) getHTTPPeer() (network.HTTPPeer, *peerSelector
 func (netFetcher *NetworkFetcher) FetchBlock(ctx context.Context, round basics.Round) (*bookkeeping.Block,
 	*agreement.Certificate, time.Duration, error) {
 	// internal retry attempt to fetch the block
-	for retryCount := 0; retryCount < netFetcher.cfg.CatchupBlockDownloadRetryAttempts; retryCount++ {
+	for retryCount := 1; retryCount <= netFetcher.cfg.CatchupBlockDownloadRetryAttempts; retryCount++ {


to say attempt 1 out of 100 instead of 0 out of 100 while logging

It is a confusing notation. All the loops start from 0 (usually). You can add +1 in the logging statement.

algonautshant

More testing might be needed, but I am fine the the changes to the existing code and the implementation of the new interfaces.
Great job!

algoganesh · 2022-08-18T04:49:00Z

More testing might be needed, but I am fine the the changes to the existing code and the implementation of the new interfaces. Great job!

Thank you!
I've fixed the notation in the loop

initial commit

1f9d312

algoganesh added the Enhancement label Aug 10, 2022

algoganesh requested a review from algonautshant August 10, 2022 19:03

algoganesh added 3 commits August 11, 2022 13:02

fix logging

aa9200a

Merge branch 'master' of https://github.com/algorand/go-algorand into…

cfd8ab6

… catchup-networkFetch-interface merge master

fix failed test and logging

70dab28

algonautshant reviewed Aug 12, 2022

View reviewed changes

algoganesh added 3 commits August 12, 2022 12:30

Merge branch 'master' of https://github.com/algorand/go-algorand into…

910894e

… catchup-networkFetch-interface merge master

PR comments

6561ecb

fix lint error

4f58ac4

algoganesh changed the title ~~Catchup: Add new interface - NetworkFetcher~~ Catchup: NetworkFetcher service to export fetchBlock Aug 12, 2022

algoganesh requested a review from winder August 12, 2022 17:30

algoganesh mentioned this pull request Aug 12, 2022

Catchup: exporting functions to increase use case of universalFetcher and peerSelector #4283

Closed

algoganesh requested a review from algonautshant August 12, 2022 19:15

algonautshant reviewed Aug 13, 2022

View reviewed changes

PR comments

6c1379a

algonautshant reviewed Aug 14, 2022

View reviewed changes

catchup/networkFetcher.go Outdated Show resolved Hide resolved

catchup/networkFetcher.go Outdated Show resolved Hide resolved

algoganesh added 2 commits August 14, 2022 14:19

Merge branch 'master' of https://github.com/algorand/go-algorand into…

c065ec3

… catchup-networkFetch-interface

PR comments | added block authentication

129dfc1

algonautshant reviewed Aug 15, 2022

View reviewed changes

catchup/networkFetcher.go Outdated Show resolved Hide resolved

catchup/networkFetcher.go Outdated Show resolved Hide resolved

algoganesh added 2 commits August 15, 2022 12:41

PR comments

37846db

Merge branch 'master' of https://github.com/algorand/go-algorand into…

70ba2c3

… catchup-networkFetch-interface

algoganesh marked this pull request as ready for review August 15, 2022 16:47

algonautshant reviewed Aug 16, 2022

View reviewed changes

winder requested a review from shiqizng August 16, 2022 14:02

add tests

d3d9591

algonautshant reviewed Aug 17, 2022

View reviewed changes

algonautshant previously approved these changes Aug 18, 2022

View reviewed changes

PR comments

0a11973

algoganesh dismissed algonautshant’s stale review via 0a11973 August 18, 2022 04:46

winder approved these changes Aug 18, 2022

View reviewed changes

winder added Skip-Release-Notes and removed Enhancement labels Aug 18, 2022

winder merged commit a29e35a into master Aug 18, 2022

winder deleted the catchup-networkFetch-interface branch August 18, 2022 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catchup: NetworkFetcher service to export fetchBlock #4388

Catchup: NetworkFetcher service to export fetchBlock #4388

algoganesh commented Aug 10, 2022 •

edited

Loading

codecov bot commented Aug 10, 2022 •

edited

Loading

algonautshant left a comment

algonautshant Aug 11, 2022

algonautshant left a comment

algonautshant left a comment

algoganesh commented Aug 16, 2022

algonautshant Aug 17, 2022

algonautshant Aug 17, 2022

algoganesh Aug 17, 2022

algonautshant Aug 18, 2022 •

edited

Loading

algonautshant left a comment

algoganesh commented Aug 18, 2022

Catchup: NetworkFetcher service to export fetchBlock #4388

Catchup: NetworkFetcher service to export fetchBlock #4388

Conversation

algoganesh commented Aug 10, 2022 • edited Loading

Summary

codecov bot commented Aug 10, 2022 • edited Loading

Codecov Report

algonautshant left a comment

Choose a reason for hiding this comment

algonautshant Aug 11, 2022

Choose a reason for hiding this comment

algonautshant left a comment

Choose a reason for hiding this comment

algonautshant left a comment

Choose a reason for hiding this comment

algoganesh commented Aug 16, 2022

algonautshant Aug 17, 2022

Choose a reason for hiding this comment

algonautshant Aug 17, 2022

Choose a reason for hiding this comment

algoganesh Aug 17, 2022

Choose a reason for hiding this comment

algonautshant Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

algonautshant left a comment

Choose a reason for hiding this comment

algoganesh commented Aug 18, 2022

algoganesh commented Aug 10, 2022 •

edited

Loading

codecov bot commented Aug 10, 2022 •

edited

Loading

algonautshant Aug 18, 2022 •

edited

Loading