-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Add default sessionTimeout to NodeStatus to increase backward compatibility #15229
Conversation
…compatibility 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:
|
Signed-off-by: Viet Nguyen Duc <[email protected]>
CI Feedback 🧐(Feedback updated until commit bd9c5f3)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
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.
I am fine with merging this but in general users need to have all components on the same version.
@diemol. Yes, ideally should have both in the same version. However, some users still need the old browser version. |
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
From 4.21.0+, there was an improvement where Node will expose the
sessionTimeout
via /status endpoint (NodeStatus) for RemoteNode can get that value for setting JdkHttpClientreadtimeout
. It helps RemoteWebDriver timeout behavior correctly. In PR #13854.Recently, there have been a few concerns that a Node with an older version (4.20.0 backward) could not register to Hub (4.21+) with the reason "Session timeout must be set". It is due to Node endpoint /status doesn't expose
sessionTimeout
, andNodeStatus
considered it isnull
when parsing JSON response.Adding the default sessionTimeout (300 seconds) to NodeStatus. Later, any Node versions can register to Hub version 4.29.0+ in upcoming release.
We add this to increase backward compatibility, since there are people who are setting up Grid for cross-browser with multiple versions rely on docker-selenium (where a Node image with specific Grid version and browser version at the time that image was built).
Relates to: #14564, SeleniumHQ/docker-selenium#2615
Types of changes
Checklist
PR Type
Bug fix, Enhancement
Description
Added a default
sessionTimeout
of 300 seconds inNodeStatus
.Improved backward compatibility for older Node versions registering to newer Hubs.
Ensured seamless integration for Nodes with missing
sessionTimeout
in/status
endpoint.Changes walkthrough 📝
NodeStatus.java
Add default sessionTimeout in NodeStatus
java/src/org/openqa/selenium/grid/data/NodeStatus.java
sessionTimeout
to 300 seconds.NodeStatus.fromJson
to handle missingsessionTimeout
.