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

[Alex Rousskov]

Hello,

    This first step towards bug #7 fix focuses on fixing several Store
API problems and polishing Store code. Many Store problems remain to be
solved, but this is a significant step forward IMO, even though no
functionality changes were intended during this step. The next steps
will address bug #7 itself.

Similar changes were previously posted for preview. Since then, we
polished the code and ran a few lab tests. The summary below repeats
most of the preview information.

The current "all Stores are the same" API forces 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).

We resolved about 15 XXXs and 10 TODOs (although these counts are
inflated by many duplicated/repeated problems). We added a few new XXXs
and TODOs as well, but they are just marking already problematic code,
not adding more problems or genuinely new work.

Class renaming and source file movement map:

> src/SwapDir.h => src/store/Disk.h (and Controller.h)
> src/SwapDir.cc => src/store/Disk.cc
> src/StoreHashIndex.h => src/store/Disks.h (and LocalSearch.h)
> src/store_dir.cc => src/store/Controller.cc (and Disks.cc, LocalSearch.cc)
> src/disk.* => src/fs_io.*


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 split
across multiple files to untangle various messes. When deciding what to
tell "bzr mv", we 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.

src/disk.* files were renamed to src/fs_io.* to avoid "src/foo conflicts
with src/store/Foo" problems expected on some case-
insensitive platforms. This addresses Amos' preview request.

We 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. Root API.
* 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 to do that later because there are few direct references to
them from the high-level code.


Related polishing touches:

Moved a lot of misplaced code into the right class and/or source file.

Simplified Store::search() interface to match the actual code that does
not support any search parameters. Removed the search API from all other
stores because the code did not really support store-specific searches.
Resisted the temptation to rename parameterless search() to iterate() or
similar because the actual future of this API is murky. We may add
search parameters or even remove the method completely. This could
quickly snowball into a separate project.

Removed Store::get(x,y,z) API as unused and unsupported.

Removed FreeObject() template as unused (and possibly technically flawed).

Simplified default Store initialization/cleanup sequence. Removed empty
disk_init(). The non-default Store::Init() parameter is used by the unit
testing code only.

Simplified Store::dereference() API by moving the second parameter to
dedicated Controller::dereferenceIdle() method that is the only ones
using that parameter.


Improvements to be merged in as separate commits if these changes are
approved:

Fixed STUB_RETREF() implementation to return the right type.
Removed bogus STUB_RETREF() comment about leaks in _unreachable_ code.
Deprecated STUB_RETSTATREF() as essentially duplicating STUB_RETREF().

Made RefCount pointers behave more like regular pointers.

Use STUB API correctly in the currently-unused ssl/stub_libsslutil.cc.


On 11/11/2015 09:31 PM, Amos Jeffries wrote:
> On 12/11/2015 12:22 p.m., Alex Rousskov wrote:
>> src/store/Disk.* (i.e. a single cache_dir) kind of clashes with
>> src/disk.* (low-level disk I/O operations)

> 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.

As mentioned above, I renamed src/disk.* to src/fs_io.* because most of
its routines are about "file" or "file system" I/O. This is not ideal
because we have a [somewhat misnamed] "fs" directory that is not really
about "file systems", but I could not come up with a better name for
src/disk.*.


Thank you,

Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug7-api-fixes-t12.patch.gz
Type: application/gzip
Size: 70646 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20151117/2b7f8f8e/attachment-0001.bin>