Skip to content
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

Replace faulty load balancer with queue Fixes #1673 #1674

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 14 additions & 27 deletions java/server/src/org/openqa/grid/internal/ProxySet.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@
import org.openqa.selenium.remote.DesiredCapabilities;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand All @@ -42,6 +41,7 @@
public class ProxySet implements Iterable<RemoteProxy> {

private final Set<RemoteProxy> proxies = new CopyOnWriteArraySet<RemoteProxy>();
private final List<RemoteProxy> nextProxyToUse = new LinkedList<RemoteProxy>();

private static final Logger log = Logger.getLogger(ProxySet.class.getName());
private volatile boolean throwOnCapabilityNotPresent = true;
Expand Down Expand Up @@ -80,6 +80,7 @@ public RemoteProxy remove(RemoteProxy proxy) {
for (RemoteProxy p : proxies) {
if (p.equals(proxy)) {
proxies.remove(p);
nextProxyToUse.remove(p);
return p;
}
}
Expand All @@ -88,6 +89,7 @@ public RemoteProxy remove(RemoteProxy proxy) {

public void add(RemoteProxy proxy) {
proxies.add(proxy);
nextProxyToUse.add(proxy);
}

public boolean contains(RemoteProxy o) {
Expand Down Expand Up @@ -121,38 +123,23 @@ public boolean isEmpty() {
return proxies.isEmpty();
}

public List<RemoteProxy> getSorted() {
List<RemoteProxy> sorted = new ArrayList<>(proxies);
Collections.sort(sorted, proxyComparator);
return sorted;
}

private Comparator<RemoteProxy> proxyComparator = new Comparator<RemoteProxy>() {
@Override
public int compare(RemoteProxy o1, RemoteProxy o2) {
double p1used = (o1.getTotalUsed() * 1.0) / o1.getTestSlots().size();
double p2used = (o2.getTotalUsed() * 1.0) / o2.getTestSlots().size();

if (p1used == p2used) return 0;
return p1used < p2used? -1 : 1;
}
};
public TestSession getNewSession(Map<String, Object> desiredCapabilities) {
// sorting is not working, use a linked list to always put the last used node at the end
log.info("Available nodes: " + nextProxyToUse);

public TestSession getNewSession(Map<String, Object> desiredCapabilities) {
// sort the proxies first, by default by total number of
// test running, to avoid putting all the load of the first
// proxies.
List<RemoteProxy> sorted = getSorted();
log.info("Available nodes: " + sorted);

for (RemoteProxy proxy : sorted) {
for (RemoteProxy proxy : nextProxyToUse) {
TestSession session = proxy.getNewSession(desiredCapabilities);
if (session != null) {
synchronized(nextProxyToUse) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modifying the list as you're iterating it? if it's not already, it's likely to lead to some concurrent modification exceptions or have some side effects.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi there. concurrent modification exceptions? the changes are atomic to the synchronized operation, so there won't be any concurrent modification (exactly why i have the synchronized block); which is also within the breakout of the loop (the session is not null so will be returned). if there would be a problem it would already have been there in the TestSession session = proxy.getNewSession(desiredCapabilities); line,i'm guessing that must be atomic

nextProxyToUse.remove(proxy);
nextProxyToUse.add(proxy);
// nextProxyToUse.add(nextProxyToUse.size(), proxy);
}
return session;
}
}
return null;
}
}

public Iterator<RemoteProxy> iterator() {
return proxies.iterator();
Expand Down
78 changes: 0 additions & 78 deletions java/server/test/org/openqa/grid/internal/ProxySetTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,8 @@
import static org.junit.Assert.assertNull;

import org.junit.Test;
import org.openqa.grid.common.RegistrationRequest;
import org.openqa.selenium.remote.BrowserType;
import org.openqa.selenium.remote.DesiredCapabilities;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class ProxySetTest {

Expand Down Expand Up @@ -58,77 +53,4 @@ public void removeIfPresent() {
registry.stop();
}
}

@Test
public void testProxySortingByIdle() throws Exception {
Registry registry = Registry.newInstance();
try {
ProxySet set = registry.getAllProxies();

set.add(buildStubbedRemoteProxy(registry, 10));
set.add(buildStubbedRemoteProxy(registry, 2));
set.add(buildStubbedRemoteProxy(registry, 0));
set.add(buildStubbedRemoteProxy(registry, 1));

List<RemoteProxy> sortedList = set.getSorted();

//Check that there are indeed 4 proxies registered
assertEquals(4, sortedList.size());

//Check the order of proxies, to make sure the totalUsed is ascending
assertEquals(0, sortedList.get(0).getTotalUsed());
assertEquals(1, sortedList.get(1).getTotalUsed());
assertEquals(2, sortedList.get(2).getTotalUsed());
assertEquals(10, sortedList.get(3).getTotalUsed());

//Check the ordered proxies to make sure proxyId's are in correct order
assertEquals("http://remote_host:0", sortedList.get(0).getId());
assertEquals("http://remote_host:1", sortedList.get(1).getId());
assertEquals("http://remote_host:2", sortedList.get(2).getId());
assertEquals("http://remote_host:10", sortedList.get(3).getId());

} finally {
registry.stop();
}

}

public StubbedRemoteProxy buildStubbedRemoteProxy(Registry registry, int totalUsed){
RegistrationRequest req = RegistrationRequest.build("-role", "webdriver","-host","localhost");
req.getCapabilities().clear();

DesiredCapabilities capability = new DesiredCapabilities();
capability.setBrowserName(BrowserType.CHROME);
req.addDesiredCapability(capability);

Map<String, Object> config = new HashMap<>();
config.put(RegistrationRequest.REMOTE_HOST, "http://remote_host:" + totalUsed);
req.setConfiguration(config);

StubbedRemoteProxy tempProxy = new StubbedRemoteProxy(req, registry);
tempProxy.setTotalUsed(totalUsed);

return tempProxy;
}

public class StubbedRemoteProxy extends BaseRemoteProxy {

private int testsRunning;

public StubbedRemoteProxy(RegistrationRequest request,
Registry registry) {

super(request, registry);
}


public void setTotalUsed(int count){
this.testsRunning = count;
}

@Override
public int getTotalUsed() {
return this.testsRunning;
}
}
}