On 05/15/2011 11:16 PM, Amos Jeffries wrote: > On Sun, 15 May 2011 15:11:08 -0600, Alex Rousskov wrote: >> For the complete set of changes (all three parts), and for testing, >> please see the following Launchpad branch: >> lp:~measurement-factory/squid/3p2-rock
> Just done a quick read-through... I can finally respond to these comments. I have just posted the current SMP caching patch a few minutes ago. If it does not get through (the compressed patch is about 145KB), I will repost with a link instead. > includes/Array.h > - can go in separately and immediately if you wish. Done as trunk r11708. > src/adaptation/icap/* > - changes appear to be relatively separate. I suspect that RST handling > may be of additional use with some 3.1 error recovery. If you agree that > can go in separately and let me know to port it back for use there. Committed separately as trunk r11705 (ICAP_ERR_GONE) and r11707 (TCP RST on ICAP errors). I recommend porting ICAP_ERR_GONE, at least, but neither are critical. > src/cf.data.pre: > - you are adding documentation for cache_dir "rock" type. That should > be in the other patch where rock is actually added. Sorry. I am trying to avoid this and similar problems now, using a single patch and even considering "bzr merge". > - contains documentation for logformat width changes. Please remove > from this patch. AFAIK the width changes are still waiting on a separate > commit to happen. If that was already committed, please point me at the > trunk revno and apply this extra documentation fix separately so I can > group it with the relevant feature patch set. User-visible changes are now committed as trunk r11709. There will be more changes to remove no longer needed type casts. > - contains documentation from "Dynamic adaptation plan over multiple > vectoring points". > Please remove and apply separately if still relevant, so it too can > be grouped properly to its feature patch set. Done as trunk r11710. > src/base/RunnersRegistry.*: > - what are runners? please document a bit clearer than "different > modules". src/base/RunnersRegistry.h contains a more detailed documentation now. In short, anything that needs to be activated/deactivated by a 3rd party can use this API, but it was designed primarily with main.cc and similar initialization/shutdown sequences in mind, to avoid linking dependencies and general code pollution. > src/store_rebuild.cc: > - Please convert the debugs() level-1 to DBG_IMPORTANT while moving the > code to storeRebuildLoadEntry() and storeRebuildParseEntry(). > (search for ", 1," to find other places in the patch needing same) Done. Initially, I tried to avoid changing moved code to ease synchronization with trunk, but there are probably too many changes there already for a few debugs() to make a significant difference. > - Also instead of using HERE they need "WARNING:" prefix and a suffix > statement that Squid will erase the broken or collision object from cache. Done, I think. > src/main.cc: > - chunk line 1232 has similar debugs() issues missing DBG_CRITICAL and > "FATAL:" prefix in the new version added. Done. > In StoreEntry::swapoutPossible() the test for "if (expectedEnd < 0)" > looks out of place. IMO the current-min > global-max should go above it > to short-cut very large unknown-length objects repeating the swapout > tests when they grow bigger than cacheable. Please confirm that. I think you are right. The order of the checks has been changed now. Thank you, Alex.
