[grid] Improve SlotMatcher and SlotSelector on request browserVersion #14914
+187
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 7369c6a)
|
diemol
requested changes
Dec 19, 2024
Comment on lines
207
to
227
| public String getBrowserVersion() { | ||
| return slots.parallelStream() | ||
| .map(slot -> slot.getStereotype().getBrowserVersion()) | ||
| .filter(Objects::nonNull) | ||
| .min(NodeStatus::semVerComparator) | ||
| .orElse(""); | ||
| } | ||
|
|
||
| public static int semVerComparator(String v1, String v2) { | ||
| // Custom semver comparator with empty strings first | ||
| if (v1.isEmpty() && v2.isEmpty()) return 0; | ||
| if (v1.isEmpty()) return -1; // Empty string comes first | ||
| if (v2.isEmpty()) return 1; | ||
|
|
||
| String[] parts1 = v1.split("\\."); | ||
| String[] parts2 = v2.split("\\."); | ||
|
|
||
| int maxLength = Math.max(parts1.length, parts2.length); | ||
| for (int i = 0; i < maxLength; i++) { | ||
| String part1 = i < parts1.length ? parts1[i] : "0"; | ||
| String part2 = i < parts2.length ? parts2[i] : part1; |
There was a problem hiding this comment.
This code shouldn't be here. It probably belongs to the DefaultSlotMatcher.
There was a problem hiding this comment.
I moved it to CapabilitiesComparator for reusable. Since this is used for both sorting in selector and slot matcher
Comment on lines
56
to
60
| // Then sort by stereotype browserVersion (descending order). SemVer comparison with | ||
| // considering empty value at first. | ||
| .thenComparing( | ||
| Comparator.comparing( | ||
| NodeStatus::getBrowserVersion, NodeStatus::semVerComparator)) |
There was a problem hiding this comment.
The semVerComparator returns true or false. This is more suitable for a filter. Which is done below, through the matcher.
There was a problem hiding this comment.
This is still needed since multiple matches returned from SlotMatcher should be sorted to move the latest version to top
VietND96
force-pushed
the
grid-slot-matcher
branch
2 times, most recently
from
744678d to
00449d7
Compare
Signed-off-by: Viet Nguyen Duc <[email protected]>
VietND96
force-pushed
the
grid-slot-matcher
branch
from
00449d7 to
7369c6a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Description
This improvement helps autoscaling control strictly on a request without the capability to set
browserVersionwill be assigned to list Slots with top priority on stereotype withoutbrowserVersionor a latestbrowserVersion(based on SemVer comparison).Improve DefaultSlotMatcher, where the request with cap
browserVersion=130can match to slot with stereotypebrowserVersion=130.0(based on SemVer comparison return 0). Few corner case also covered e.gMotivation and Context
This pull request introduces several important changes to the Selenium Grid project, focusing on improving the way browser versions are compared and sorted. The changes include implementing a custom semantic version comparator, updating the slot selection logic, and adding relevant tests.
Improvements to browser version comparison:
java/src/org/openqa/selenium/grid/data/NodeStatus.java: Added agetBrowserVersionmethod and a customsemVerComparatormethod for comparing semantic versions with empty strings considered first.Updates to slot selection logic:
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java: Modified thematchesmethod to use the newsemVerComparatorfor browser version comparison.java/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java: Updated the slot selection logic to sort nodes by browser version using the new comparator.Addition of tests:
java/test/org/openqa/selenium/grid/data/NodeStatusTest.java: Added tests for thesemVerComparatormethod to ensure correct browser version matching.java/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java: Added tests to verify nodes are ordered correctly by browser version. [1] [2]Miscellaneous:
rust/src/lock.rs: Added license information to the file.Types of changes
Checklist
PR Type
Enhancement
Description
Enhanced browser version handling in Selenium Grid with the following improvements:
Implemented semantic version comparison for better browser version matching
Improved slot selection and matching logic:
Added comprehensive test coverage:
Code quality improvements:
Changes walkthrough 📝
DefaultSlotMatcher.java
Improved browser version matching logicjava/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
semVerComparatorfor moreflexible matching
NodeStatus.java
Added semantic version comparison functionalityjava/src/org/openqa/selenium/grid/data/NodeStatus.java
getBrowserVersionmethod to retrieve minimum browser versionsemVerComparatorfor semantic version comparisonisNumberfor version parsingDefaultSlotSelector.java
Enhanced slot selection with version-based sortingjava/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java
NodeStatusTest.java
Added tests for semantic version comparisonjava/test/org/openqa/selenium/grid/data/NodeStatusTest.java
DefaultSlotSelectorTest.java
Added tests for version-based node orderingjava/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java
lock.rs
Added missing license headerrust/src/lock.rs