Conversation
|
This PR represents the original work that was started off by @dima-groupon as part of #265 and later rebased and revived by @mach6 |
|
Here's how the apis and their corresponding responses would look like GET endpoints:
{
"configuration": {
"capabilityMatcher": "org.openqa.grid.internal.utils.DefaultCapabilityMatcher",
"newSessionWaitTimeout": -1,
"throwOnCapabilityNotPresent": true,
"cleanUpCycle": 5000,
"custom": {},
"host": "192.168.1.5",
"servlets": [],
"withoutServlets": [],
"browserTimeout": 0,
"debug": false,
"port": 4444,
"role": "hub",
"timeout": 1800
},
"nodes": [
{
"host": "192.168.1.5",
"port": 5555,
"id": "http://192.168.1.5:5555"
},
{
"host": "192.168.1.5",
"port": 6666,
"id": "http://192.168.1.5:6666"
}
],
"registrationUrl": "http://192.168.1.5:4444//grid/register/",
"consoleUrl": "http://192.168.1.5:4444//grid/console",
"newSessionRequestCount": 0,
"usedProxyCount": 2,
"totalProxyCount": 2,
"activeSessionCount": 3
}
[
{
"host": "192.168.1.5",
"port": 5555,
"id": "http://192.168.1.5:5555",
"slotUsage": {
"total": 11,
"busy": 2,
"breakup": [
{
"browser": "chrome",
"usage": {
"total": 5,
"busy": 2
}
},
{
"browser": "internet explorer",
"usage": {
"total": 1,
"busy": 0
}
},
{
"browser": "firefox",
"usage": {
"total": 5,
"busy": 0
}
}
]
}
},
{
"host": "192.168.1.5",
"port": 6666,
"id": "http://192.168.1.5:6666",
"slotUsage": {
"total": 11,
"busy": 1,
"breakup": [
{
"browser": "chrome",
"usage": {
"total": 5,
"busy": 1
}
},
{
"browser": "internet explorer",
"usage": {
"total": 1,
"busy": 0
}
},
{
"browser": "firefox",
"usage": {
"total": 5,
"busy": 0
}
}
]
}
}
]
{
"isBusy": true,
"sessions": [
{
"browserName": "chrome",
"sessionId": "157569ab-e0d8-4d04-b78d-7244d9d2d0c4"
},
{
"browserName": "chrome",
"sessionId": "30c089bd-ff76-4b95-a157-78519c488986"
}
],
"java": {
"version": "1.8.0_66"
},
"os": {
"name": "Mac OS X",
"arch": "x86_64",
"version": "10.12.3"
},
"slotUsage": {
"total": 11,
"busy": 2,
"breakup": [
{
"browser": "chrome",
"usage": {
"total": 5,
"busy": 2
}
},
{
"browser": "internet explorer",
"usage": {
"total": 1,
"busy": 0
}
},
{
"browser": "firefox",
"usage": {
"total": 5,
"busy": 0
}
}
]
},
"htmlRenderer": "org.openqa.grid.selenium.proxy.WebProxyHtmlRenderer",
"build": {
"version": "3.0.1",
"revision": "unknown",
"time": "unknown"
},
"percentUsed": 40.0,
"lastSessionStart": 1486196956545,
"config": {
"hubHost": "localhost",
"hubPort": 4444,
"id": "http://192.168.1.5:5555",
"capabilities": [
{
"browserName": "chrome",
"seleniumProtocol": "WebDriver",
"maxInstances": 5,
"platform": "MAC"
},
{
"browserName": "firefox",
"seleniumProtocol": "WebDriver",
"maxInstances": 5,
"platform": "MAC"
},
{
"browserName": "internet explorer",
"seleniumProtocol": "WebDriver",
"maxInstances": 1,
"platform": "MAC"
}
],
"downPollingLimit": 2,
"hub": "http://localhost:4444/grid/register",
"nodePolling": 5000,
"nodeStatusCheckTimeout": 5000,
"proxy": "org.openqa.grid.selenium.proxy.DefaultRemoteProxy",
"register": true,
"registerCycle": 5000,
"unregisterIfStillDownAfter": 60000,
"cleanUpCycle": 5000,
"custom": {},
"host": "192.168.1.5",
"maxSession": 5,
"servlets": [],
"withoutServlets": [],
"browserTimeout": 0,
"debug": false,
"port": 5555,
"role": "node",
"timeout": 1800
}
}
{
"31006720-cddd-408d-8f65-09813b8b5cb7": {
"slotInfo": {
"host": "192.168.1.5",
"port": 6666,
"id": "http://192.168.1.5:6666"
},
"browser": "chrome"
},
"eb38d217-3445-4f03-bf58-3428d28fb130": {
"slotInfo": {
"host": "192.168.1.5",
"port": 5555,
"id": "http://192.168.1.5:5555"
},
"browser": "chrome"
},
"8e9b194d-72ce-4b69-aecf-a0672f854b57": {
"slotInfo": {
"host": "192.168.1.5",
"port": 5555,
"id": "http://192.168.1.5:5555"
},
"browser": "chrome"
}
}
{
"isForwardingRequest": false,
"proxy": {
"host": "192.168.1.5",
"port": 5555,
"nodeId": "http://192.168.1.5:5555"
},
"protocol": "WebDriver",
"lastActivityWasAt": 253698,
"isOrphaned": false,
"internalKey": "bb556e03-95c4-4abd-bbae-7fbcfe16c026",
"requestedCapabilities": {
"browserName": "chrome"
},
"requestPath": "/wd/hub"
} |
8e2fa51 to
aa151e9
Compare
|
@krmahadevan thanks for this! This is going to take a little time to vet with the Se community. I'll look at the existing work in more detail in the days to come but in the meantime, will you please add tests? Cheers! |
|
@mach6 - Any news on how is the review with the SE community going on ? Thought of quickly checking on this! |
|
@krmahadevan in general, agreement that since all of these api's are GET only, it is okay and will add value -- though there is concern about the long term implications of supporting additional api's and the fact that we'll have a mixed bag of RESTful (GET) operations and RESTish (POST/DELETE/GET) operations (the existing apis). Once we complete the tests and remaining refinement, I believe I can merge this. |
There was a problem hiding this comment.
I liked it better when we were hashing the Id for the benefit of the HTTP URL. This looks silly to me http://localhost:4444/api/v1/proxy/http://192.168.1.5:5555 due to http:// ... http://
There was a problem hiding this comment.
Fixed this by accepting both forms viz., with and without the protocol
|
@mach6 - I have added tests as well. We should now be good to get this merged. |
03623c1 to
b50f820
Compare
|
ping @mach6 .. Gentle reminder Doug! |
There was a problem hiding this comment.
The problem is a proxy id can be anything a user wants it to be. Only by default or if the (user followed the default convention) will it be http://{host}:{port}
If you look at java -jar selenium-server-standalone -role node -h and the documentation for id it says this;
-id
<String> : optional unique identifier for the node. Defaults to the url
of the remoteHostMeaning it can be http://ftp://$$$123/*hello/@123/rdp:// or any crazy text I want it to be -- as long as it is unique. :)
So, we really need to one way hash the proxy id or similar and require the user to match all proxy id's to their hash by making a GET on a /proxies resource (or I guess it could be the /nodes resource)..
Come to think of it -- perhaps it is confusing having /nodes and /proxy/{id} .. we probably should combine these into one.
Something like;
/proxies -- get a list of all proxies
/proxies/{id} - get for the specified proxy
Similarly instead of /session and /sessions perhaps this would be better;
/sessions -- get a list of all sessions
/sessions/{id} - get for the specified session
There was a problem hiding this comment.
The problem is a proxy id can be anything a user wants it to be. Only by default or if the (user followed the default convention) will it be http://{host}:{port}
In that case the earlier change itself was sufficient, because it was basically querying based on whatever the user passes. So we can revert it back.
So, we really need to one way hash the proxy id or similar and require the user to match all proxy id's to their hash by making a GET on a /proxies resource (or I guess it could be the /nodes resource)..
This additional step IMHO Its not user friendly. I don't think there is anything wrong with using whatever the user has provided as the id for a proxy (even if it means that we are going to default to the url of the remoteHost) which should also be fine IMO rather than resorting to hashing it and making it more complex.
If you look at java -jar selenium-server-standalone -role node -h and the documentation for id it says this;
I don't remember seeing this in Selenium 2.53.1. Following is what I see
java -jar selenium-server-standalone-2.53.1.jar -role node -help
-------------------------------
Running as a grid node:
-------------------------------
Usage: java -jar selenium-server.jar -role node [options]
-host:
<IP | hostname> : usually not needed and determined
automatically. For exotic network configuration, network with
VPN, specifying the host might be necessary.
-port:
<xxxx> : the port the remote/hub will listen on. Default to 4444.
-cleanupCycle:
<XXXX> in ms. How often a proxy will check for timed out thread.
-timeout:
<XXXX> the timeout in seconds before the hub automatically ends
a test that hasn't had any activity in the last X seconds. The
browser will be released for another test to use. This typically
takes care of the client crashes.
-browserTimeout:
The timeout in seconds a browser can hang
-hub:
<http://localhost:4444/grid/register> : the url that will be used
to post the registration request. This option takes precedence
over -hubHost and -hubPort options.
-hubHost:
<IP | hostname> : the host address of a hub the registration
request should be sent to. Default to localhost. Option -hub
takes precedence over this option.
-hubPort:
<xxxx> : the port listened by a hub the registration request
should be sent to. Default to 4444. Option -hub takes precedence
over this option.
-proxy:
the class that will be used to represent the node. By default
org.openqa.grid.selenium.proxy.DefaultRemoteProxy.
-maxSession:
max number of tests that can run at the same time on the node,
independently of the browser used.
-registerCycle:
how often in ms the node will try to register itself again.Allow
to restart the hub without having to restart the nodes.
-nodePolling:
in ms. Interval between alive checks of node how often the hub
checks if the node is still alive.
-unregisterIfStillDownAfter:
in ms. If the node remains down for more than
unregisterIfStillDownAfter millisec, it will disappear from the
hub.Default is 1min.
-downPollingLimit:
node is marked as down after downPollingLimit alive checks.
-nodeStatusCheckTimeout:
in ms. Connection and socket timeout which is used for node alive
check.
This synopsis lists options available in node role only. To get help
on the command line options available for other roles run the server
with -help name and the corresponding -role name value.
Just thinking aloud here. I am not sure if this is something that should be left to the user to customize because this is internally used by the node to uniquely identify itself (which you called out). In that case, should a user really fiddle around with this ? Its not altering or tweaking the behavior of the node in any way (which I believe is what the different parameters in the GridNodeConfiguration stand for. So all that said and done, is this something that we should really worry about ? Thoughts ?
There was a problem hiding this comment.
In that case the earlier change itself was sufficient, because it was basically querying based on whatever the user passes. So we can revert it back.
Well, that makes the HTTP GET URL look silly again. I'll think about / bounce the consideration off of others a little more ..
Just thinking aloud here. I am not sure if this is something that should be left to the user to customize because this is internally used by the node to uniquely identify itself (which you called out). In that case, should a user really fiddle around with this
The hub registration protocol makes these allotments. There are use cases where 'autodetecting' or 'assuming' the proxy id is not sufficient. In Se2.x the allotments were also there -- except you could only specify proxy id via code or the config json, if I recall correctly.
There was a problem hiding this comment.
There are use cases where 'autodetecting' or 'assuming' the proxy id is not sufficient.
@mach6 - Can you please help call out what those use cases might be ? I would be curious to learn/hear about them
|
@krmahadevan thx for the follow-up here. I left a few more new comments for your consideration |
Following was done : * Cleaned up the end-points. * Added a new end-point for querying hub information.
b50f820 to
09e74b2
Compare
Consolidated implementations for the end-points for proxies and sessions and updated tests accordingly.
09e74b2 to
961c181
Compare
|
@mach6 - I have consolidated the proxy end-point and the session end-point into the same thing so that it works for querying all proxies/sessions (or) for a particular proxy/session. I would really like to get this PR wrapped up. Please let me know what else is pending on this. |
|
ping @mach6 - Gentle reminder! |
|
ping @mach6 - Any updates on this ? |
|
@mach6 - Pls let me know if I should just close off this PR. I haven't heard of anything on this PR and its more than a couple of months... |
|
Sorry for the continued delay. please do not close |
|
@mach6 - What is holding this PR ? Can you please let me know. I was hoping that this would be wrapped up a long time back. |
Following was done : * Cleaned up the end-points. * Added a new end-point for querying hub information. * Consolidated implementations for the end-points for proxies and sessions and updated tests accordingly. Signed-off-by: Doug Simmons <doug@mach6.net>
Xin the preceding checkbox, I verify that I have signed the Contributor License Agreement