[squid-dev] [PATCH] PackableStream for cachemgr reports

[Alex Rousskov]

On 08/09/2015 01:47 AM, Amos Jeffries wrote:
> On 9/08/2015 1:14 p.m., Alex Rousskov wrote:
>> On 08/08/2015 02:03 AM, Amos Jeffries wrote:
>>
>>>>> * convert old Action classes to new API
>>>>> * refactor old C-code report generators to be Action classes
>>>>>  - fixing display output syntax to minimal YAML as we go on both the
>>>>> above. It mostly is already, but some reports have wrong syntax.
>>>>
>>>> It feels wrong that you want Actions to output using strict syntax but
>>>> give them an "free form" or "anything goes" std::ostream as a tool to
>>>> accomplish that. Should not Action dumping methods receive a specialized
>>>> class that makes it easy to produce the output you want and hard (if not
>>>> impossible) to make syntax mistakes?
>>
>>
>>> Action itself is currently that class you speak about AFAICT.
>>
>> Mgr::Action is a cache manager command [processor]. Action knows where
>> to get the information we need to write. Formatting obtained information
>> is a very different matter. Yes, we could add formatting methods to
>> Action itself, but I am not sure that is a good idea.
> 
> ... with children inheriting which do all the formatting display parts.

The distinction between the parent class and children is irrelevant
here. The fact that Action (and/or Action children) do formatting now
does not change what Action (and/or Action children) are defined and
meant to do. They are not meant to know formatting details, but somebody
has to format and the current Action (and/or Action children) code does it.


>>> Maybe in
>>> future the "update Action API to receive client desired format" will be
>>> a formatter class rather than just header value.
>>
>> Perhaps, but what is the value of replacing dump(StoreEntry) with
>> dump(ostream) now, when what we actually want is dump(void) or
>> dump(Formatter)? It feels like you want to rewrite the same code twice,
> 
> The last attempt was 4 years ago. The one before that 6 years prior again.

Why is that relevant? Just because nobody has done the right thing for
many years does not imply we should do something else which does not
even move us in the right direction and causes problems for the admins.


> If you can agree that we start with a Mgr::Formatter class that does not
> do anything initially but dump out a free-form "notice" box with a line
> of text in it. The other reports output and formatter API/functionality
> to be deferred to adding by other patches as need arises. Then I think
> I/we can go forward with that.


We pretty much already have what you describe today: The current code
uses StoreEntry::append*() calls. One could view (or even implement)
that slice of StoreEntry API as dumb Formatter class that you have
described. This is the easier part of the problem that has been already
solved.

The more difficult part is introducing a non-free-form Formatter class.


> Primarily that we can do the formatting cleanup without adding a
> circular dependency between libbase and libmem in the current code
> situation.


This answer to my "Why do the same change twice?" question does not
compute/correlate for me. I do not see why two cleanups are better than
one here (if that is what you are implying). Also, "without adding X"
cannot be the reason because the existing code does not add anything.



>>> For now its just a free-form mix in plain text. To which a stream is
>>> suited. And possible something the formatter classes could use better
>>> than a direct Packable object pointer/reference.
>>
>> The code for free-form formatting already exists. We do not want it in
>> the future. Why replace one free-form with a slightly different
>> free-form when what we need is YAML-form?
>>
> 
> I want to see the reports actually looking like sets of tables 

This desire seems unrelated to my concern.


> For now I was taking the best approach from the currently existing but
> incomplete designs in Squid code and picking the one that appears to be
> most suited for completing. StoreEntryStream in current trunk appeared
> to be the one that did not limit future display options.

I do not see how changing StoreEntry->append() to ostream::operator <<
helps you to make "reports actually looking like sets of tables" and/or
to move us closer to YAML support. Again, it feels like we are making
admins life harder without any significant internal improvements.


> If you can agree that we start with a Mgr::Formatter class that does not
> do anything initially but dump out a free-form "notice" box with a line
> of text in it.

I hope we can come up with a minimal API (only usable for the very basic
output such as one-line notices and such). We will need to discuss and
agree on what that minimum is exactly.

If YAML support is one of the ultimate goals, then I would propose
something like this:

// Assembles information for a single Cache Manager "page".
// Supplied information goes into a configured stream.
// Kids implement various information formatting methods.
class Page: public std::ostream {
public:
    Page(std::ostream &os);
    virtual ~Page() {}

    // a comment
    virtual Page &comment(const SBuf &text) = 0;

    /// starts recording a list of values
    virtual Page &beginList() = 0;
    /// finishes recording a list of values
    virtual Page &endList() = 0;

    /// starts recording a list of values
    virtual Page &beginListItem() = 0;
    /// finishes recording a list of values
    virtual Page &endListItem() = 0;

    /// starts recording a key:value pair with a given key
    virtual Page &beginKv(const SBuf &key) = 0;
    /// finishes recording a key:value pair
    virtual Page &endKv() = 0;
...
};

The methods below comment() may be needed for more advanced stuff like
the menu but probably not needed for "notice boxes". If that API is what
you were describing as a free-form "notice box" or "line of text", then
we are in agreement.

The attached file has a sample implementation of that API for a YAML
formatter. It currently violates most YAML rules but can become
compliant with more work. Here is a sample output for the menu page
(adjusted for indentation that is not automated in the sample code):


---
- 5min: menu item description to be extracted from Action::desc
  protection: public
- 60min: menu item description to be extracted from Action::desc
  protection: public
- disk_io: menu item description to be extracted from Action::desc
  protection: public
- events: menu item description to be extracted from Action::desc
  protection: public
- shutdown: menu item description to be extracted from Action::desc
  protection: public
# This is not really related to the menu example above
TestKey: several words per 1 value are OK
---



>  ... the remainder is specific to the proposed PackableStream or
> StoreEntryStream code. Which would be dropped if you agree on the above
> approach for how to get a Formatter class transition going.

Sorry, have to run. I will read/review the rest later.

Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: Page.cc
Type: text/x-c++src
Size: 3697 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150809/b01e5f5c/attachment.cc>