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

service/header: Refactor HeaderService to separate distinct components into clearer sub-components #317

Merged
merged 16 commits into from
Jan 5, 2022

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Dec 27, 2021

This PR does two things:

  1. refactors HeaderService in order to make it agnostic to the details of its sub-services. HeaderService's no longer has any function and only contains pointers to its subcomponents for later use once we scope out the RPC component. All ingredients necessary to construct its sub-services are created during node construction. Sub-service lifecycle management is also delegated to the DI application.
  2. splits up sub-services a bit better into more isolated functions. For example, rather than having P2PExchange start the server-side component (which is necessary for all nodes), but not need it for its exchange component when in bridge mode, the server-side component is split out into a separate lifecycle which is present for every node type as all nodes can serve headers, but not all nodes request headers from the same method (p2p vs. directly from core).

This PR is quite large but its necessary moving forward as HeaderService is the largest and most complicated component of celestia-node, and many other services are dependent on it, so keeping it clean and its sub-services nicely self-contained is crucial.

Another thing to mention is that this PR includes commits from #277, rendering that PR unnecessary on its own, and should also precede #303.

Resolves #287

@renaynay renaynay force-pushed the header-service-lifecycle-manager branch from 995a7ea to b75078d Compare December 27, 2021 11:18
node/services/service.go Outdated Show resolved Hide resolved
node/bridge_test.go Outdated Show resolved Hide resolved
service/header/sync.go Outdated Show resolved Hide resolved
@renaynay renaynay changed the title [DRAFT] -- HeaderService refactoring in progress service/header: Refactor HeaderService to only manage its sub-services' lifecycles Dec 28, 2021
@renaynay renaynay marked this pull request as ready for review December 28, 2021 11:27
@renaynay renaynay force-pushed the header-service-lifecycle-manager branch from e0ad250 to c59f5cb Compare January 3, 2022 13:37
@renaynay renaynay changed the title service/header: Refactor HeaderService to only manage its sub-services' lifecycles service/header: Refactor HeaderService to separate distinct components into clearer sub-components Jan 3, 2022
@renaynay renaynay force-pushed the header-service-lifecycle-manager branch from 2ad37dc to e2b712b Compare January 3, 2022 14:29
service/block/service.go Outdated Show resolved Hide resolved
service/header/core_exchange.go Outdated Show resolved Hide resolved
service/header/p2p_exchange.go Show resolved Hide resolved
service/header/pubsub_manager.go Outdated Show resolved Hide resolved
service/header/sync.go Outdated Show resolved Hide resolved
service/header/sync.go Outdated Show resolved Hide resolved
node/core/core.go Outdated Show resolved Hide resolved
node/services/service.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Jan 3, 2022
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Well done!(even when I prefer medium-rare more 😄)

node/bridge_test.go Show resolved Hide resolved
service/header/p2p_exchange.go Show resolved Hide resolved
liamsi
liamsi previously approved these changes Jan 5, 2022
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

This looks like great work! I definitely need to check out the code locally soon and go through each line again to get all the context again!

@renaynay renaynay dismissed stale reviews from liamsi and Wondertan via 475729e January 5, 2022 12:16
@renaynay renaynay force-pushed the header-service-lifecycle-manager branch from 10ac82b to 475729e Compare January 5, 2022 12:16
@renaynay renaynay merged commit e112545 into celestiaorg:main Jan 5, 2022
@renaynay renaynay deleted the header-service-lifecycle-manager branch January 5, 2022 12:20
Bidon15 pushed a commit to Bidon15/celestia-node that referenced this pull request Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

service/block: BlockService not necessary for bridge nodes
3 participants