-
Notifications
You must be signed in to change notification settings - Fork 589
v2.3: remove calls to get EpochInfo from LeaderTpuCacheUpdater (backport of #6748) #6870
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
Conversation
Cherry-pick of fecc916 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
7e53f01
to
28a6154
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2.3 #6870 +/- ##
=========================================
- Coverage 82.8% 82.8% -0.1%
=========================================
Files 850 850
Lines 379914 379909 -5
=========================================
- Hits 314809 314746 -63
- Misses 65105 65163 +58 🚀 New features to boost your workflow:
|
@t-nelson the previous PR has been approved, can we merge both of these now (the second will need to be rebased and approved afterwards of course)? |
@mergify rebase |
☑️ Nothing to do, the required conditions are not met
|
hmm not sure why github has this flagged for conflicts... there are no conflict markers in the total diff nor either individual commit gonna try to close and reopen |
Maybe just drop commit 39e0078 from this branch? |
Maybe just drop commit 39e0078 from this branch? This branch has been rebased on top of previous commit so it has the commit that has been added to the 2.3. I can just drop now this commit (?), if sounds reasonable |
give it a shot i guess? honestly no idea where the error is coming from |
use EpochSchedule instead of EpochInfo
28a6154
to
acc9fd9
Compare
I did |
conflicts flag is gone anyway 🤞 |
@steviez wanna sme here too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and nice to leverage the existing math (from EpochSchedule
) to figure out the last slot in the epoch
Problem
We have logic related to
EpochInfo
updates which is actually not needed as soon as we haveEpochSchedule
.Summary of Changes
This PR removes rpc call for
EpochInfo
. All we need it for is to getslots_in_epoch
which is passed tofanout
function to have upper bound for the fanout on the clusters where epoch is smaller than2 * MAX_FANOUT_SLOTS == 200
(probably relevant for tests only?).This is an automatic backport of pull request #6748 done by [Mergify](https://mergify.com).