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

uses MAX_LEADER_SCHEDULE_STAKES for cluster-nodes cache capacity #2151

Merged

Conversation

behzadnouri
Copy link

Problem

Because of the TTL based eviction it does not make sense to cache more than MAX_LEADER_SCHEDULE_STAKES epochs.

Summary of Changes

Set CLUSTER_NODES_CACHE_NUM_EPOCH_CAP equal to MAX_LEADER_SCHEDULE_STAKES.

Some more context here: #1735 (comment)

Because of the TTL based eviction it does not make sense to cache more
than MAX_LEADER_SCHEDULE_STAKES epochs.
Copy link

@bw-solana bw-solana 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 to me. Left one question

@@ -52,7 +53,8 @@ pub(crate) mod broadcast_utils;
mod fail_entry_verification_broadcast_run;
mod standard_broadcast_run;

const CLUSTER_NODES_CACHE_NUM_EPOCH_CAP: usize = 8;
const_assert_eq!(CLUSTER_NODES_CACHE_NUM_EPOCH_CAP, 5);

Choose a reason for hiding this comment

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

Why the assert? Are you worried about max leader schedule stakes adjusting up? If so, maybe we should min this with some constant cap?

Copy link
Author

Choose a reason for hiding this comment

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

Just as an explicit signal that if that constant is changed, it has ramifications here.
Also serves as a documentation that what the capacity actually is.

@behzadnouri behzadnouri merged commit 736bf93 into anza-xyz:master Jul 18, 2024
41 checks passed
@behzadnouri behzadnouri deleted the cluster-nodes-cache-num-epoch-cap branch July 18, 2024 16:15
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.

2 participants