[Runtime Epoch Split] (4/n) Make ShardTracker accessible from RuntimeWithEpochManagerAdapter. #8769
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robin-near
force-pushed
the
robin-epoch4
branch
3 times, most recently
from
d28ac14
to
5132a16
Compare
|
@robin-near: do you know if someone from core team will review this or do you want me to find a reviewer for you? |
Hmm, I was expecting the core-team alias to resolve to an engineer. Let me try again. |
robin-near
force-pushed
the
robin-epoch3
branch
from
76551b0
to
fded7b2
Compare
robin-near
force-pushed
the
robin-epoch4
branch
from
5132a16
to
7ecedc7
Compare
jakmeier
approved these changes
Mar 22, 2023
| pub fn new(tracked_config: TrackedConfig, epoch_manager: Arc<dyn EpochManagerAdapter>) -> Self { | ||
| ShardTracker { | ||
| tracked_config, | ||
| tracking_shards_cache: Arc::new(SyncLruCache::new(1024)), |
There was a problem hiding this comment.
Can you describe in the code why 1024? The number makes sense to me but writing down the basic thought process why 1024 is large enough but also not too small would be useful.
There was a problem hiding this comment.
I missed that - thanks!
…WithEpochManagerAdapter.
robin-near
force-pushed
the
robin-epoch4
branch
from
7ecedc7
to
9043100
Compare
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a reminder of the overall goal, we want to split
RuntimeWithEpochManagerAdapter, so that any code that needs the runtime will use anArc<RuntimeAdapter>, and any code that uses the epoch manager will use theEpochManagerHandle.We're doing this refactoring bottom-up, i.e. propagating
RuntimeWithEpochManagerAdapteraround at the top-level, but making some lower-level components useArc<RuntimeAdapter>and/orEpochManagerHandle.Now, it turns out that
ShardsManager, one of the lower-level components, would need not onlyEpochManagerHandle, but also an additionalShardTrackerbuilt on top ofEpochManagerHandle, whose functionality was previously covered byRuntimeWithEpochManagerAdapterinterface.ShardTrackeris not technically part ofEpochManagereither, because it isn't inherent to the epoch part of the protocol, but is controlled by the node operator's config options.ShardTrackeris a field ofNightshadeRuntime, but is not available in theKeyValueRuntime; instead, theKeyValueRuntimeimplements all theShardTrackerfunctionalities via theRuntimeWithEpochManagerAdapterinterface. This would not work.This PR moves
ShardTrackerto the near-epoch-manager crate, adds an additionalshard_tracker()function toRuntimeWithEpochManagerAdapterto allow passing it down from a higher-level component to a lower-level component, and performs a refactoring to makeKeyValueRuntimeprovide theShardTrackerfunctionality via itsEpochManagerAdapterimplementation. This also required changingShardTrackerto useEpochManagerAdapter, as opposed to the concreteEpochManagerHandle.