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

[Runtime Epoch Split] (2/n) Narrow EpochManagerAdapter error type to EpochError wherever possible. #8767

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

robin-near
Copy link
Contributor

As a reminder of the overall goal, we want to split RuntimeWithEpochManagerAdapter, so that any code that needs the runtime will use an Arc<RuntimeAdapter>, and any code that uses the epoch manager will use the EpochManagerHandle.

We're doing this refactoring bottom-up, i.e. propagating RuntimeWithEpochManagerAdapter around at the top-level, but making some lower-level components use Arc<RuntimeAdapter> and/or EpochManagerHandle.

That means we need to be able to obtain an Arc<RuntimeAdapter> and an EpochManagerHandle from a RuntimeWithEpochManagerAdapter. However, this is not trivial at all:

  1. KeyValueRuntime, the implementation of RuntimeWithEpochManagerAdapter for testing, does not contain an EpochManager at all, so it's not possible to extract an EpochManagerHandle from it (which is essentially an arc mutex of EpochManager). That means instead of using EpochManagerHandle, we need to use Arc<EpochManagerAdapter> in the meantime.
  2. Extracting an Arc<EpochManagerAdapter> from a Arc<RuntimeWithEpochManagerAdapter> is not trivial. Even though RuntimeWithEpochManagerAdapter is a trait that extends EpochManagerAdapter, trait upcast is not allowed by Rust in general. So we need to resort to a workaround.

This PR addresses an issue arising from (1), that current code that expects an EpochManagerHandle uses functions from EpochManager which mostly returns EpochError. EpochManagerAdapter, on the other hand, returns mostly near_chain_primitive::Error, so changing to the adapter type breaks existing code. So, in this PR we are changing the error type returned from most of the EpochManagerAdapter functions to also be EpochError. This is a pretty painless transition, except in some cases we need to do a two-hop conversion from EpochError to chain Error and then to some other error (and two-hop conversions cannot be supported by .into() in general), so I've defined .into_chain_error() to help with the two-hop conversion cases.

@robin-near robin-near requested a review from a team March 21, 2023 17:54
@robin-near robin-near marked this pull request as ready for review March 21, 2023 17:54
@robin-near robin-near requested a review from a team as a code owner March 21, 2023 17:54
@robin-near robin-near requested a review from nikurt March 21, 2023 17:54
Copy link
Contributor

@nikurt nikurt left a comment

Choose a reason for hiding this comment

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

Looks good.
Most of the changes are simple replacements Error -> EpochError.
The newly added into_chain_error() makes conversion of types of errors seamless.

@near-bulldozer near-bulldozer bot merged commit dc8f61c into master Mar 22, 2023
@near-bulldozer near-bulldozer bot deleted the robin-epoch2 branch March 22, 2023 16:45
@robin-near robin-near linked an issue Jul 19, 2023 that may be closed by this pull request
2 tasks
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.

Separate EpochManager from Runtime
2 participants