-
Notifications
You must be signed in to change notification settings - Fork 558
Preliminary readiness management #1107
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
d2/src/main/java/com/linkedin/d2/balancer/servers/ReadinessStatusManager.java
Outdated
Show resolved
Hide resolved
d2/src/main/java/com/linkedin/d2/balancer/servers/ReadinessStatusManager.java
Show resolved
Hide resolved
d2/src/main/java/com/linkedin/d2/balancer/servers/ReadinessStatusManager.java
Outdated
Show resolved
Hide resolved
b926424 to
4768f6c
Compare
d2/src/main/java/com/linkedin/d2/balancer/servers/ReadinessStatusManager.java
Outdated
Show resolved
Hide resolved
| ## [Unreleased] | ||
|
|
||
| ## [29.76.0] - 2025-09-15 | ||
| - Preliminary readiness management |
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.
Nit: what's preliminary about it? Would it make more sense to say initial implementation of readiness management system or similar?
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.
No, please see the doc in the PR description, also explained in this class's doc. It's preliminary as a short-term solution since it's based on d2 announcements, which shouldn't be in long run with topology aware SD will change it to base on internal components' readiness.
d2/src/main/java/com/linkedin/d2/balancer/servers/NoOpReadinessStatusManager.java
Show resolved
Hide resolved
d2/src/main/java/com/linkedin/d2/balancer/servers/ReadinessStatusManager.java
Outdated
Show resolved
Hide resolved
| * Note: if readinessStatusManager.requireStaticD2ServersAnnounced is set to true, and some D2 clusters have | ||
| * isToBeAnnouncedFromStaticConfig set to true, then any of them is de-announced will become NOT_SERVING. | ||
| */ | ||
| NOT_SERVING, |
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.
Nit: should we mention that NOT_SERVING is/should be the default ReadinessStatus if there's no other information? It looks like in our internal implementation we use it as the default, not sure if we want to document that here
(just considering I could also imagine an argument for INCONSISTENT as the default)
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.
INCONSISTENT is when some announcement/de-announcement happened and still ongoing or failed. At start up, no announcement is made yet, so not serving is default. I will comment it in the impl class in container. The interface here should be general.
Summary
As designed in the doc.
Register each announcer's announcement status to readiness manager in container, see container PR.
Test Done
UT.
QEI deploy indis-canary, see container PR