Skip to content

Commit 4a3f3ef

Browse files
diemoldiemol
committed
[grid] Simplifying DefaultSlotSelector logic
The idea has always been to keep as available as possible the Nodes with diverse configurations. Such as the ones supporting different browsers, like a Node supporting Chrome, Firefox and Safari. But if we have a Node that only supports Chrome, we'd like to give as many Chrome sessions as possible to keep the one with Safari as available as possible, so when a Safari session request comes in, it can be served right away. The previous logic had that in mind, but the implementation was rather complex. This simplified version orders Nodes by the number of browsers they support. Therefore, it would offer "specialized" Nodes first (e.g. only supporting one browser), and the more diverse ones at the bottom.
1 parent b4b7674 commit 4a3f3ef

3 files changed

Lines changed: 130 additions & 221 deletions

File tree

‎java/server/src/org/openqa/selenium/grid/distributor/Distributor.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,8 +155,8 @@ protected Distributor(
155155
.with(new SpanDecorator(tracer, req -> "distributor.status")));
156156
}
157157

158-
public Either<SessionNotCreatedException, CreateSessionResponse> newSession(
159-
HttpRequest request) throws SessionNotCreatedException {
158+
public Either<SessionNotCreatedException, CreateSessionResponse> newSession(HttpRequest request)
159+
throws SessionNotCreatedException {
160160

161161
Span span = newSpanAsChildOf(tracer, request, "distributor.create_session_response");
162162
Map<String, EventAttributeValue> attributeMap = new HashMap<>();
@@ -199,7 +199,7 @@ public Either<SessionNotCreatedException, CreateSessionResponse> newSession(
199199

200200
if (!hostsWithCaps) {
201201
String errorMessage = String.format(
202-
"No host supports the capabilities required: %s",
202+
"No Node supports the required capabilities: %s",
203203
payload.stream().map(Capabilities::toString).collect(Collectors.joining(", ")));
204204
SessionNotCreatedException exception = new SessionNotCreatedException(errorMessage);
205205
span.setAttribute(AttributeKey.ERROR.getKey(), true);
@@ -212,7 +212,7 @@ public Either<SessionNotCreatedException, CreateSessionResponse> newSession(
212212
return Either.left(exception);
213213
}
214214

215-
// Find a host that supports the capabilities present in the new session
215+
// Find a Node that supports the capabilities present in the new session
216216
Set<SlotId> slotIds = slotSelector.selectSlot(firstRequest.getCapabilities(), model);
217217
if (!slotIds.isEmpty()) {
218218
selected = Optional.of(reserve(slotIds.iterator().next(), firstRequest));

‎java/server/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java‎

Lines changed: 17 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -24,38 +24,27 @@
2424
import org.openqa.selenium.grid.data.SlotId;
2525

2626
import java.util.Comparator;
27-
import java.util.HashMap;
28-
import java.util.HashSet;
29-
import java.util.LinkedHashSet;
30-
import java.util.List;
31-
import java.util.Map;
32-
import java.util.Optional;
3327
import java.util.Set;
34-
import java.util.logging.Logger;
35-
import java.util.stream.Collectors;
36-
import java.util.stream.Stream;
3728

3829
import static com.google.common.collect.ImmutableSet.toImmutableSet;
3930

4031
public class DefaultSlotSelector implements SlotSelector {
4132

42-
private static final Logger LOG = Logger.getLogger(DefaultSlotSelector.class.getName());
43-
4433
@Override
4534
public Set<SlotId> selectSlot(Capabilities capabilities, Set<NodeStatus> nodes) {
46-
Stream<NodeStatus> firstRound = nodes.stream()
47-
// Find a node that supports this kind of thing
48-
.filter(node -> node.hasCapacity(capabilities));
49-
50-
// of the nodes that survived the first round, separate into buckets and prioritize by browser "rarity"
51-
Stream<NodeStatus> prioritizedNodes = getPrioritizedNodeStream(firstRound, capabilities);
52-
53-
//Take the further-filtered Stream and prioritize by load, then by session age
54-
55-
return prioritizedNodes
35+
// First, filter the Nodes that support the required capabilities. Then, the filtered Nodes
36+
// get ordered in ascendant order by the number of browsers they support.
37+
// With this, Nodes with diverse configurations (supporting many browsers, e.g. Chrome,
38+
// Firefox, Safari) are placed at the bottom so they have more availability when a session
39+
// requests a browser supported only by a few Nodes (e.g. Safari only supported on macOS
40+
// Nodes).
41+
// After that, Nodes are ordered by their load, last session creation, and their id.
42+
return nodes.stream()
43+
.filter(node -> node.hasCapacity(capabilities))
5644
.sorted(
45+
Comparator.comparingLong(this::getNumberOfSupportedBrowsers)
5746
// Now sort by node which has the lowest load (natural ordering)
58-
Comparator.comparingDouble(NodeStatus::getLoad)
47+
.thenComparingDouble(NodeStatus::getLoad)
5948
// Then last session created (oldest first), so natural ordering again
6049
.thenComparingLong(NodeStatus::getLastSessionCreated)
6150
// And use the node id as a tie-breaker.
@@ -67,99 +56,13 @@ public Set<SlotId> selectSlot(Capabilities capabilities, Set<NodeStatus> nodes)
6756
.collect(toImmutableSet());
6857
}
6958

70-
/**
71-
* Takes a Stream of NodeStatus, along with the Capabilities of the current request, and prioritizes the
72-
* request by removing NodeStatus that offer Capabilities that are more rare. e.g. if there are only a
73-
* couple Edge nodes, but a lot of Chrome nodes, the Edge nodes should be removed from
74-
* consideration when Chrome is requested. This does not currently take the amount of load on the
75-
* server into consideration--it only checks for availability, not how much availability
76-
*
77-
* @param nodes Stream of nodestatus attached to the Distributor (assume it's filtered for only those that offer these Capabilities)
78-
* @param capabilities Passing in the whole Capabilities object will allow us to prioritize more than just browser
79-
* @return Stream of distinct NodeStatus with the more rare Capabilities removed
80-
*/
8159
@VisibleForTesting
82-
Stream<NodeStatus> getPrioritizedNodeStream(Stream<NodeStatus> nodes, Capabilities capabilities) {
83-
//TODO for the moment, we're not going to operate on the Stream that was passed in--we need to
84-
// alter and futz with the contents, so the stream isn't the right place to operate. This
85-
// will likely be optimized back into the algo, but not yet
86-
Set<NodeStatus> filteredNodeSet = nodes.collect(Collectors.toSet());
87-
88-
//A "bucket" is a list of nodes that can use a particular browser. The "edge" bucket is the
89-
// complete list of nodes that support "edge". By separating nodes into buckets, we will
90-
// know which browsers have fewer nodes available for the browsers we're not interested in, and
91-
// can prioritize based on the browsers that have more availability
92-
Map<String, Set<NodeStatus>> nodeBuckets = sortNodesToBucketsByBrowser(filteredNodeSet);
93-
94-
//First, check to see if all buckets are the same size. If they are, just send back the full list of nodes
95-
// (i.e. the nodes are all "balanced" with regard to browser priority)
96-
if (allBucketsSameSize(nodeBuckets)) {
97-
return nodeBuckets.values().stream().distinct().flatMap(Set::stream);
98-
}
99-
100-
//Then, starting with the smallest bucket that isn't the current browser being prioritized,
101-
// remove all nodes in that bucket from consideration, then rebuild the buckets. Then do the
102-
// "same size" check again, and keep doing this until either a) there is only one bucket, or b)
103-
// all buckets are the same size
104-
105-
//Note: there should never be a case where a bucket will have *more* nodes available for the
106-
// given browser than the one being requested. The first filter in this check looks for
107-
// "equal", not "equal-or-greater" as a result of this assumption
108-
109-
//There might be unforeseen cases that challenge this assumption. TODO Create unit tests to prove it
110-
111-
//TODO a List of Map.Entry is silly. whatever this structure needs to be needs to be returned by
112-
// the sortHostsToBucketsByBrowser method in a way that we don't have to sort it separately like this
113-
final List<Map.Entry<String, Set<NodeStatus>>> sorted = nodeBuckets.entrySet().stream().sorted(
114-
Comparator.comparingInt(v -> v.getValue().size())
115-
).collect(Collectors.toList());
116-
117-
// Until the buckets are the same size, keep removing nodes that have more "rare" browser capabilities
118-
Map<String, Set<NodeStatus>> newNodeBuckets;
119-
for (Map.Entry<String, Set<NodeStatus>> entry : sorted) {
120-
//Don't examine the bucket containing the browser in question--we're prioritizing the other browsers
121-
//TODO This shouldn't be necessary, because if the list is sorted by size, this won't be possible until
122-
// they're all the same size. Create a unit test to prove it
123-
if (entry.getKey().equals(capabilities.getBrowserName())) {
124-
continue;
125-
}
126-
127-
//Remove all nodes from this bucket from the full set of eligible nodes
128-
final Set<NodeStatus> filteredNodes = filteredNodeSet.stream()
129-
.filter(node -> !entry.getValue().contains(node))
130-
.collect(Collectors.toSet());
131-
132-
//Rebuild the buckets by browser
133-
newNodeBuckets = sortNodesToBucketsByBrowser(filteredNodes);
134-
135-
//Check the bucket sizes--if they're the same, then we're done
136-
if (allBucketsSameSize(newNodeBuckets)) {
137-
LOG.fine("Nodes have been balanced according to browser priority");
138-
return newNodeBuckets.values().stream().distinct().flatMap(Set::stream);
139-
}
140-
}
141-
142-
return nodeBuckets.values().stream().distinct().flatMap(Set::stream);
143-
}
144-
145-
Map<String, Set<NodeStatus>> sortNodesToBucketsByBrowser(Set<NodeStatus> nodes) {
146-
//Make a hash of browserType -> list of nodes that support it
147-
Map<String, Set<NodeStatus>> buckets = new HashMap<>();
148-
149-
for (NodeStatus node : nodes) {
150-
for (Slot slot : node.getSlots()) {
151-
String name = Optional.ofNullable(slot.getStereotype().getBrowserName()).orElse("");
152-
buckets.computeIfAbsent(name, n -> new LinkedHashSet<>()).add(node);
153-
}
154-
}
155-
156-
return buckets;
60+
long getNumberOfSupportedBrowsers(NodeStatus nodeStatus) {
61+
return nodeStatus.getSlots()
62+
.stream()
63+
.map(slot -> slot.getStereotype().getBrowserName().toLowerCase())
64+
.distinct()
65+
.count();
15766
}
15867

159-
@VisibleForTesting
160-
boolean allBucketsSameSize(Map<String, Set<NodeStatus>> buckets) {
161-
Set<Integer> intSet = new HashSet<>();
162-
buckets.values().forEach(bucket -> intSet.add(bucket.size()));
163-
return intSet.size() == 1;
164-
}
16568
}

0 commit comments

Comments
 (0)