On Sat, Jan 10, 2009 at 06:03:42PM -0500, Jason A. Smith wrote: > On Sat, 2009-01-10 at 11:25 +0000, Carlo Marcelo Arenas Belon wrote: > > On Thu, Jan 08, 2009 at 04:13:13PM -0500, Jason A. Smith wrote: > > > > > > Recently I started testing the svn version of the web scripts and found > > > a few bugs. > > > > could you elaborate on the bug this patch is fixing?, the code from > > trunk and the other 2 active branches seems similar enough to consider > > this a problem also outside of trunk. > > I have not tested nor looked at any other trunks unfortunately. I have > a working 3.0.7 installation, and this problem does not exist there.
AFAIK, it doesn't exist in trunk either as I am unable to reproduce it. could it be that some patch you are adding is triggering the behaviour? > > > Fixed graph zooming and make sure the default summary graph size > > > overrides the size selected for the cluster graphs. > > > > "graphargs" should contain (in the host view) all host specific parameters > > that are needed to create the graphs like h (hostname), r (range) and > > st (start time) but doesn't have z (size) and therefore by moving it to the > > beginning of the generated URL all it changed is the order used, at > > least for the report graphs which are the ones that are being patched. > > The problem I noticed was mainly in the cluster view, when selecting a > specific metric to look at or the size, it would also change the size of > the top summary graphs in addition to the lower host graphs. I assume > this is an unintended consequence of probably a few patches interacting > together since previous versions of ganglia did not do this, but I did > not bother to track it down. if a "z" variable ever gets into graphargs that would most likely break the code the way you describe, but as I mentioned before it doesn't seem to be doing that, or at least it doesn't seem to be doing that in a clean checkout from trunk that I'd been testing for this bug. > I think the problem also occurred in the > host view, but I don't really remember. The meta view already had the > graph variable placed first in the arg list, so patching in this way > also makes all three main graph views work the same. The only change, > as you say is the order of the "graphargs" to force the medium size to > override what is in the variable. I agree that the way it is coded is fragile and your patch is just making all the references more consistent by changing the order, but as I argue below, I think that relying on the variable to be overridden and the order of the variables to be of significance (as your patch suggests) is the wrong approach to solving the problem. In any case, at least for now, Committed revision 1942. > > for the metrics graph, the order does (sadly) make a difference as the > > zoom relies on having "z" redefined to "large" through the template, but > > the patch doesn't apply to that section of the code. > > I am not sure what you mean here, the patch does apply to the "zoom" > feature, since it does touch the graph image links also. In addition to > the summary graphs at the top being affected, I noticed that zooming was > also broken. I was commenting on the way the template for host view is constructed and in the fact that for the metric graphs, the position for graphargs was important as it shows "z=medium" as part of "graphargs" and then relies in the template to override that with "z=large" for the zoom to work. Your patch changes the way the URLs for the "report" graphs are being generated so it matches the way the ones for the "metric" graphs but doesn't change the code there as it is already working even if in a hacky way. > > could we instead remove the hardcoded values and manage the URLs in a > > way that makes them not dependent on the order of the parameters so that > > variables are overridden? > > Possibly, this was just the easiest fix that I thought of though, since > it keeps the graph args variable in the list, so they can share things > like the time range, so they don't have to be managed separately, just > the order was changed, to act like an override. In host_view.php:54, graphargs is defined for each metric to have a hardcoded size "medium" and that is then overridden in the HREF through the template so that the link used when the graph is clicked has : c=$cluster&h=$hostname&...&z=medium&...&z=large My argument was that it will be IMHO better if the graph size would be configurable and therefore both sizes for the graph detached from the "graphargs" variable so that the code won't have to rely on a variable being overridden or the order the arguments have as it does now. Carlo ------------------------------------------------------------------------------ Check out the new SourceForge.net Marketplace. It is the best place to buy or sell services for just about anything Open Source. http://p.sf.net/sfu/Xq1LFB _______________________________________________ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
