On 08/12/2013 12:29 PM, Christian Boos wrote: > On 8/10/2013 10:29 PM, Eli Carter wrote: >> Ok, I've updated the patch series: >> >> part1: implicit sync > > LGTM.
Committed to 0.12-stable, 1.0-stable, and trunk. (I followed what looked to be the merge pattern of 0.12, merge to 1.0, merge to trunk.) >> part2: warn on slow SQL >> Unmodified from the initial post; when debug_sql is enabled, logs a >> warning if an SQL statement takes over 10 seconds to execute. > > Wouldn't it be better to fetch the value of "10" seconds from the > configuration? ([trac] debug_sql_timing_threshold?). 3 seconds may be > already too long for many people... Makes sense, but getting a configuration option into that code is starting to get a little awkward; it doesn't have easy access to the trac env, and adding a new option done in the same manner as the 'log' parameter complained about an unexpected keyword argument. I'll have to look at it a bit more... >> part3: uncache previous_rev() >> Makes previous_rev() unconditionally go to the repository without >> hitting the node_change table. (Was "part3a".) > > I would prefer that you keep the previous code, but commented out, and > the comment saying in addition "commented out for now", making it clear > this is a workaround for the way the SQL query is currently implemented. > That clearly shows that we tried to use the cache, should anyone want to > improve speed by using the cache again. I'm not sure this is a workaround. The version control system is naturally going to have the right data structures for finding the previous revision; why put a database as a cache around that? >> part4: warn on slow next_rev() >> This logs a warning if next_rev() takes more than 10 seconds to >> complete, suggesting enabling [trac] debug_sql. (Was "part3b".) >> The purpose of this is to give the admin that first breadcrumb to >> lead them to the underlying cause of their slowness. >> > > Same thing, we could use the configured threshold for the timing. > Default could still be 10s of course, but people could then easily make > it more sensitive without hacking the code. That looks doable... > (And we could add another setting for timing the requests themselves, > that would be useful as well.) > > Lastly, what do you think about the suggestion of turning the "next > revision" link into a relative one, in the code browser page? I think it's a good idea. I'd like to complete what I have already first though. Eli -- You received this message because you are subscribed to the Google Groups "Trac Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/trac-dev. For more options, visit https://groups.google.com/groups/opt_out.
