Skip to content

Improve aggregator dependencies management #382

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

Merged
merged 9 commits into from
Aug 3, 2022

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Aug 1, 2022

  • Move most RwLock creation & management inside the trait implementations when needed instead of using them on the whole traits.
  • Remove all Option in the DependencyManager, heavily simplifying its usage.

@Alenar Alenar requested review from jpraynaud and ghubertpalo August 1, 2022 16:04
@github-actions
Copy link

github-actions bot commented Aug 1, 2022

Unit Test Results

253 tests  ±0   253 ✔️ ±0   3m 47s ⏱️ -45s
  20 suites ±0       0 💤 ±0 
    7 files   ±0       0 ±0 

Results for commit cb959d7. ± Comparison against base commit 60b36c4.

♻️ This comment has been updated with latest results.

Alenar added 9 commits August 2, 2022 12:50
By moving the RwLock directly inside the MemoryBeaconStore.
A RwLock was only needed by the DumbSnapshotUploader that ... already used one.
By moving the RwLock directly inside the only implementation that needed it (LocalSnapshotStore).
By moving the RwLock directly inside it.
By moving the RwLock directly inside it.
By moving the RwLock directly inside it.
By moving the RwLock directly inside it.
…erver dependencies

By moving the RwLock directly inside it.
A dependency should not be optionnal if it's feature is mandatory, this heavely simplify the use of the DependencyManager.
@Alenar Alenar force-pushed the djo/improve_aggregator_dependencies_management branch from e8d900d to cb959d7 Compare August 2, 2022 15:34
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM

@Alenar Alenar merged commit 907f674 into main Aug 3, 2022
@Alenar Alenar deleted the djo/improve_aggregator_dependencies_management branch August 3, 2022 08:29
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.

3 participants