-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Improvement for Node handling #14628
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
CI Failure Feedback 🧐(Checks updated until commit b5d02d9)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:
where Configuration options
See more information about the |
fc95807
to
b684f60
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #14628 +/- ##
==========================================
- Coverage 58.48% 57.78% -0.70%
==========================================
Files 86 89 +3
Lines 5270 5614 +344
Branches 220 245 +25
==========================================
+ Hits 3082 3244 +162
- Misses 1968 2125 +157
- Partials 220 245 +25 ☔ View full report in Codecov by Sentry. |
b684f60
to
0e4038f
Compare
0e4038f
to
9dced98
Compare
d066ac7
to
0af907e
Compare
java/src/org/openqa/selenium/grid/node/ForwardWebDriverCommand.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
0af907e
to
c9c6236
Compare
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
a7a838a
to
411df25
Compare
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
46a9894
to
b5d02d9
Compare
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.
Description
Motivation and Context
checkSessionCount() - to check and drain node after sessions are reached. Steps in order, so in logs, we saw something like
It might confuse that the session created in a draining node.
So, moving it to block finally { }
The trade-off is the session count will include both success and unsuccess. When the session comes, the remaining count decreases.
It could be suitable for autoscaling. For example, if a faulty Node (for unknown reasons), the session cannot be created, and the node will be drained to signal the scaler to scale a new one. Instead of keeping Node running, it biases the scaler already had enough nodes running.
Also, there was a request #13865 - Automatically drain node after n-failed session attempts. By moving that check to
finally
blocks, it would be best for both.Add the
drain()
method to the shutdown hook. Node will send drain event to remove Node immediately once Runtime stopped.Fixes #13769 - Exception
NoSuchSessionException
should throw out when command is routed to Node that session just timed out.Types of changes
Checklist