Skip to content

mempool: p2p refactor#5919

Merged
alexanderbez merged 23 commits intomasterfrom
bez/p2p-refactor-mempool-reactor
Jan 22, 2021
Merged

mempool: p2p refactor#5919
alexanderbez merged 23 commits intomasterfrom
bez/p2p-refactor-mempool-reactor

Conversation

@alexanderbez
Copy link
Contributor

Description

Prepares the mempool reactor for the newly designed p2p changes per ADR 062.

ref: #5670

@alexanderbez alexanderbez added C:mempool Component: Mempool C:p2p Component: P2P pkg labels Jan 18, 2021
@alexanderbez alexanderbez marked this pull request as ready for review January 21, 2021 16:26
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #5919 (31d9128) into master (62d7a5d) will decrease coverage by 0.14%.
The diff coverage is 57.07%.

@@            Coverage Diff             @@
##           master    #5919      +/-   ##
==========================================
- Coverage   59.95%   59.80%   -0.15%     
==========================================
  Files         270      271       +1     
  Lines       24696    24890     +194     
==========================================
+ Hits        14806    14886      +80     
- Misses       8285     8387     +102     
- Partials     1605     1617      +12     
Impacted Files Coverage Δ
p2p/shim.go 51.90% <0.00%> (ø)
proto/tendermint/blockchain/message.go 43.39% <ø> (ø)
blockchain/v0/reactor.go 76.28% <33.33%> (-1.15%) ⬇️
p2p/router.go 62.26% <39.13%> (-1.16%) ⬇️
p2p/peer.go 50.90% <41.22%> (-5.46%) ⬇️
statesync/reactor.go 57.39% <42.85%> (+1.03%) ⬆️
state/services.go 40.00% <50.00%> (ø)
evidence/pool.go 61.80% <56.14%> (-3.80%) ⬇️
mempool/reactor.go 64.33% <63.07%> (-18.17%) ⬇️
node/node.go 57.06% <82.60%> (+0.22%) ⬆️
... and 19 more

@alexanderbez alexanderbez self-assigned this Jan 21, 2021
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

Looks great!

// should be used in cases where guarantees cannot be made about when and how
// many times closure is executed.
type Closer struct {
closeOnce deadlock.Once
Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad that we have to duplicate all of the code just for this import. Would it be possible to have two files which set the type as a variable or something?

Copy link
Contributor Author

@alexanderbez alexanderbez Jan 21, 2021

Choose a reason for hiding this comment

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

Not sure I follow? Is there an easier approach when build flags are involved?

Copy link
Contributor

@erikgrinaker erikgrinaker Jan 21, 2021

Choose a reason for hiding this comment

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

I was thinking we could use an interface or something, and just do e.g. var Once OnceInterface = deadlock.Once, and same in a different file with sync.Once. However, turns out deadlock.Once doesn't do anything so we can just move it to a separate file and not use deadlock.Once at all.

https://github.com/sasha-s/go-deadlock/blob/1595213edefa28ca5047b00340c63557f9c051d0/deadlock.go#L58

@erikgrinaker
Copy link
Contributor

Looks like the E2E tests are failing here btw.

@alexanderbez
Copy link
Contributor Author

Looks like the E2E tests are failing here btw.

Yes, because I missed adding an explicit height > 0 check (hence the timeout because height will always be zero until we have everything setup). Fixed it and now it all passes.

@alexanderbez alexanderbez merged commit 68bd211 into master Jan 22, 2021
@alexanderbez alexanderbez deleted the bez/p2p-refactor-mempool-reactor branch January 22, 2021 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:mempool Component: Mempool C:p2p Component: P2P pkg

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants