[squid-dev] [PREVIEW] Store API changes blocking bug #7

[Amos Jeffries]

On 12/11/2015 12:22 p.m., Alex Rousskov wrote:
> Hello,
> 
>     The attached compressed diff represents a 70% complete first step
> towards bug #7 fix. This step focus is on fixing "all Stores are the
> same" API that forced us to bloat the base Store class with methods
> needed only in Store::Root() Controller or other specific storage
> classes. Besides unblocking bug 7 development, one of the primary
> objectives of this step was to avoid functionality changes (so that they
> will be considered for v4.0 inclusion instead of forcing a branching point).
> 
> There are a few technical details below, but I am posting this preview
> to ask the following high-level questions:
> 
> 1. Should I move most of the remaining [Ss]tore*.{cc,h} code to src/store/?
> 
> 2. Should I place most of the moved code in #1 under Store namespace,
> and perform other aggressive renamings as needed to polish Store API?
> 
> Item #1 will require many changes to #include statements throughout the
> code but is kind of incomplete (some might say pointless?) without #2.
> Doing #2 will cause lots of changes in the caller code throughout Squid.
> Both will significantly delay bug #7 fix due to sheer amount of renaming
> work. If there is no strong consensus that these changes should be done
> now, then I will _not_ do them, finish the previewed changes, and then
> proceed with the actual bug #7 fix.

If they are not necessary, then I think procrastinate.


> 
> Also, src/store/Disk.* (i.e. a single cache_dir) kind of clashes with
> src/disk.* (low-level disk I/O operations), but I cannot find a better
> name for Store::Disk. The old SwapDir name feels rather inadequate for
> some cache_dir types, and especially awkward inside the Store namespace:
> Store::SwapDir. Better names for Store::Disk and Store::Disks classes
> are welcomed.

One of them will definitely need to rename. We cannot have any files in
src/ that matches case-insensitively with a file in one of the
sub-directories.

There are several OS and at least some of the community self-builds that
I know of building Squid with case insensitive filesystems or
case-ignorant automake.


> 
> Please keep the virtually-no-functionality-changes restriction in mind
> when answering these questions!
> 
> 
> 
> The attached patch polishes class naming and source file locations:
> 
>   src/SwapDir.h => src/store/Disk.h (and Controller.h)
>   src/SwapDir.cc => src/store/Disk.cc
>   src/StoreHashIndex.h => src/store/Disks.h
>   src/store_dir.cc => src/store/Controller.cc (and Disks.cc)
> 
> The code movement to files in parenthesis is not tracked by bzr because
> bzr cannot track file splits, and most of the moved code had to be
> splitted across multiple files to untangle various messes. When deciding
> what to tell "bzr mv", I picked file pairs that would allow us to track
> the most complex, most voluminous code but there is probably no single
> correct way to do that.

Good. Thats what I would have done too (and have in the past).

> 
> Again, I tried to preserve the functionality of the moved code (but
> without better bzr support it is especially difficult to validate that
> all moved code was moved intact).
> 
> 
> The Store namespace hierarchy now looks like this:
> 
> * Storage: Any storage. Similar to the old Store class, but leaner.
> 
> * Controller: Combined memory/disks caches and transients.
> 
> * Controlled: Memory cache, disk(s) cache, or transient Storage.
> 
> * Disks: All disk caches combined.
> 
> * Disk: A single cache_dir Storage.
> 
> * Memory: A memory cache.
> 
> * Transients: Entries capable of being collapsed for CF.
> 
> The last two are not moved/finalized yet, but it should not be too
> difficult.
> 
> Untested; "make all" works (with hacks) but "make check" fails. This is
> PREVIEW!
> 
> 
> Thank you,
> 
> Alex.
> 

Thank you.

Amos