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.
Received on Wed Sep 07 2011 - 00:26:11 MDT
This archive was generated by hypermail 2.2.0 : Wed Sep 07 2011 - 12:00:06 MDT