-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
[grid] Expose register status via Node status response #15448
base: trunk
Are you sure you want to change the base?
Conversation
Signed-off-by: Viet Nguyen Duc <[email protected]>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
nodeRegistered.set(true); | ||
node.register(); |
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.
Do we need this flag in two places?
Can we just use the new one for both?
protected boolean draining; | ||
protected boolean registered; |
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.
These fields should be volatile to ensure different threads will see the changes.
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Motivation and Context
Fixes SeleniumHQ/docker-selenium#2705
Currently, Node status response looks like
"ready": true
is returned byhasCapability()
- this status reflects Node is ready (connected to Bus, has slots available).However, this does not include registration status.
To have reliable data for adding Node health checks (Docker) or Node startup/readiness probes (K8s), via
/status
response, we expose one more attributeregistered
(true/false) for registration status with Hub.Now, response when Node just up, not registered to Hub yet
Once it is able to register to Hub
Types of changes
Checklist
PR Type
Enhancement
Description
Added
registered
attribute to Node status response for registration status.Updated
hasCapacity
andhasCapability
methods to check Node availability.Introduced
isRegistered
andregister
methods inNode
class.Enhanced readiness check to include Node registration status.
Changes walkthrough 📝
NodeStatus.java
Update capacity checks with Node availability
java/src/org/openqa/selenium/grid/data/NodeStatus.java
hasCapability
andhasCapacity
methods to check Nodeavailability.
Node.java
Add registration status tracking to Node
java/src/org/openqa/selenium/grid/node/Node.java
registered
attribute to track Node registration status.isRegistered
andregister
methods for registrationhandling.
StatusHandler.java
Include registration status in Node status response
java/src/org/openqa/selenium/grid/node/StatusHandler.java
registered
attribute to Node status response.NodeServer.java
Improve readiness check with registration status
java/src/org/openqa/selenium/grid/node/httpd/NodeServer.java
registered
attribute.