OpenLayers OpenLayers

Ticket #410 (closed bug: fixed)

Opened 2 years ago

Last modified 2 years ago

multi-wms servers should use deterministic server id

Reported by: crschmidt Assigned to: sderle
Priority: minor Milestone: 2.4 Release
Component: Layer Version: SVN
Keywords: Cc:
State:

Description

Non-deterministic server choice is a bad idea. When I view a tile, I may get caching headers with that URL from a properly configured server. By randomly changing the server I request from, the browser has to store up to numUrls caches for each tile.

Instead, make this deterministic based on the integer value of the request parameters, modded into the severs array. Deployed on http://labs.metacarta.com/wms-c/demo.html, patch incoming against SVN.

Attachments

multi.patch (3.5 kB) - added by crschmidt on 11/18/06 11:03:59.
multi-wms.patch (0.9 kB) - added by tschaub on 11/20/06 12:31:10.
alternative deterministic server indexing for multi-wms
deterministic-server-select.patch (4.7 kB) - added by sderle on 12/22/06 18:48:28.
deterministic server selection based on simple real number hashing algorithm
deterministic-server-select2.patch (4.7 kB) - added by tschaub on 12/27/06 12:26:33.
same as above but with left,bottom instead of minx,miny
410-final.patch (6.5 kB) - added by sderle on 03/19/07 19:54:28.
merger of my and Chris's patch
multiUrls-fix.patch (2.7 kB) - added by euzuro on 03/21/07 12:45:43.
this fixes the httplayer in opera and ie, and does not break in ff either. the idea is that you cannot safely pass the url variable to Util.getArgs() anymore now that it could be either a string or an array of urls (which, of course, is a perfect example of why making a variable like that which can be of two different types is generally not a good idea). So what we do is we do the deterministic url selecting bit *before* that call... and then we rebuild the paramString again after the call and the check. I have added some basic coments to at least correctly document the type of the 'url' property
410-really-final.patch (6.2 kB) - added by sderle on 03/21/07 16:46:39.
patch incorporating Erik's last patch plus recommended changes

Change History

11/18/06 09:50:19 changed by crschmidt

  • keywords set to review.

Patch attached, please review.

11/18/06 11:03:59 changed by crschmidt

  • attachment multi.patch added.

11/18/06 11:04:14 changed by crschmidt

Newest patch includes tests.

11/19/06 09:20:39 changed by crschmidt

  • keywords deleted.

I'm not sure I'm happy with the level of randomness this provides. Don't review/commit yet. I'll see if I can come up with some better way of doing this.

11/20/06 12:31:10 changed by tschaub

  • attachment multi-wms.patch added.

alternative deterministic server indexing for multi-wms

11/20/06 12:33:21 changed by tschaub

The multi-wms.patch is a quick hack at an alternative way to determine a server index. This would need to be done for each layer type that allowed multiple servers. It is also seems horribly inefficient and probably not ready for serious review - just a thought.

12/06/06 16:35:59 changed by sderle

  • owner set to sderle.

12/22/06 18:48:28 changed by sderle

  • attachment deterministic-server-select.patch added.

deterministic server selection based on simple real number hashing algorithm

12/22/06 18:51:11 changed by sderle

  • keywords set to review.

Latest patch provides deterministic server selection for WMS, TMS, ka-Map via HTTPRequest. Slight changes are needed to individual layers because those layers contain the information on tile bounds that's used to hash an index into the layer.url array. We could conceivably apply said changes to other layers as well (MapServer, WMS.Untiled) but I figured it belonged in these layers first. Tests are included in the patch.

Please review and apply. All tests should pass.

12/26/06 14:10:28 changed by tschaub

bounds.minx and bounds.miny should be changed to bounds.left and bounds.bottom in the WMS.js part of your patch (though I like the more explicit better)

12/27/06 12:26:33 changed by tschaub

  • attachment deterministic-server-select2.patch added.

same as above but with left,bottom instead of minx,miny

12/28/06 20:11:46 changed by euzuro

  • milestone changed from 2.3 Release to 2.4 Release.

03/08/07 13:23:13 changed by sderle

this is great but it needs a fall back for layers that don't call selectUrl

03/19/07 19:54:28 changed by sderle

  • attachment 410-final.patch added.

merger of my and Chris's patch

03/19/07 19:59:37 changed by crschmidt

  • keywords changed from review to commit.

I'm happy with it. Commit it.

03/19/07 20:00:32 changed by sderle

  • status changed from new to closed.
  • resolution set to fixed.

fixed by r2821.

03/21/07 12:34:58 changed by euzuro

  • status changed from closed to reopened.
  • resolution deleted.

This patch breaks tests in both IE and Opera. And for a good reason. Patch forthcoming.

03/21/07 12:45:43 changed by euzuro

  • attachment multiUrls-fix.patch added.

this fixes the httplayer in opera and ie, and does not break in ff either. the idea is that you cannot safely pass the url variable to Util.getArgs() anymore now that it could be either a string or an array of urls (which, of course, is a perfect example of why making a variable like that which can be of two different types is generally not a good idea). So what we do is we do the deterministic url selecting bit *before* that call... and then we rebuild the paramString again after the call and the check. I have added some basic coments to at least correctly document the type of the 'url' property

03/21/07 13:00:10 changed by euzuro

I would like to note that before this ticket should be closed as 'fixed' again, it would sure be nice if someone could take the three minutes necessary to put some comments in the seletctUrl function.

I do not consider myself the wisest of the wise, yet neither am i the most novice programmer or OL user, and I can tell you that I had *no* idea what is going on in this function at first glance. I had to do an svn blame to see when it was added, then read up on the ticket for more info.

I see this comment, inline lower in the code, where the function gets called:

    // in which case we will deterministically select one of them in 
    // order to evenly distribute requests to different urls.

This is nice but we should see that explanation (namely, the 'deterministically' bit) up in the function comment as well.

To me, any time you are doing wierd math like:

(Math.sqrt(5) - 1) / Math.sqrt(3)

... you should probably have at least a loose explanation of what is going on there.

(note that this value, by the way, should be replaced by a constant so that it's not getting re-computed for each character in the paramstring ((and so that the line doesnt go over the 79 character limit)) )

03/21/07 16:46:39 changed by sderle

  • attachment 410-really-final.patch added.

patch incorporating Erik's last patch plus recommended changes

03/21/07 16:47:07 changed by sderle

  • keywords changed from commit to review.

03/22/07 13:48:49 changed by euzuro

  • status changed from reopened to closed.
  • resolution set to fixed.

fixed with r2852. only added small comments bit, tests fix from sde's final patch. all is groovy.

03/22/07 13:50:02 changed by sderle

  • keywords deleted.