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

[Amos Jeffries]

On 10/08/2015 9:59 a.m., Alex Rousskov wrote:
> 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.

s/Formatter/Packable/ and then this Alex guy tied the patch up that
starts Store.h include removal.

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

Yes.

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

 LDADD=
   store/libstore.la
   mem/libmem.la

... undefined symbols StoreEntry::* in libmem.

LDADD=
  mem/libmem.la
  store/libstore.la

... undefined symbols Mem::AllocatorProxy::* in store/libstore.la.


Since we dont have libstore yet the problem shows up indirectly with
other libraries in LDADD listed after libmem and presenting a symbol
used by Store code inline functions (ie. libhttp header types). When the
store code shuffles the above loop appears more clearly.


Anyhow. Thats hopefully not going to be relevant with a simple enough
Formatter and Packable.

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

You asked. As you say my reason does not preclude meeting your concern.
Impass avoided this time around.

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

To use your argument from above:
 2 changes [layers] are better than 3.

Getting all the way down to 1 change did not seem possible a week ago
when I started this. Now it looks more hopeful.

> 
>> 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);

Does this mean you want to go head with PackableStream for the libmem
problem ?

If we plan to get rid of PackableStream / StoreEntryStream in the end
product that ctor could take a Packable&.


This assumes that the Page object does the actual append() to the
Packable it references.

For the record; I have considered the alternative design where the Page
is just a struct holding tokens for output at each position in the
display state.
PageSimple {
.comment = ': ';
.beginList = ':\n';
.endList = '\n';
.tableRow = '\t';
.tableEndRow = '\n'
... and so on.
}

Useful for some syntaxes, but we can avoid things like tableEndRow
states with yours.


>     virtual ~Page() {}
> 
>     // a comment
>     virtual Page &comment(const SBuf &text) = 0;
> 

I was suggesting just the above bit in the first patch.

Although the immediate uses seem more like notice box / announcements
than regular text comment. The display end would highlight them in bold
colors or something to attract attention.


The below ...

B)

>     /// 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;
> 

A)
>     /// 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;
> ...

(A) deferred to next patch updating a report that uses just a kv-pair
output list.

Then (B) or similar when a tabular report is done.

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

Okay.

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

Nevermind. I'm dropping it for now and running with the above.

Amos