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

[Alex Rousskov]

On 08/09/2015 09:39 PM, Amos Jeffries wrote:
>>> 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=


I begin to understand why the progress on this thread is so inefficient:
You are apparently trying to solve several problems at once, some of
them correctly, some not. When I question the wrong decisions, you
defend the right ones, and the resulting conversation makes no sense.
The lack of the description of the primary problems the patch was
attempting to solve (i.e., the "Why"s) did not help either.


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

Great, at least the discussion was not a total waste of time.


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


I have nothing against PackableStream* classes as such. If they are
useful for something, they should be polished and committed, of course.

I am against making half-way changes to the cache manager output.
Library dependencies can be changed without changing output.
Alternatively, we can solve linking problems while switching to the new
and _final_ output format. If you are doing the legwork, it is your call
whether to combine the two activities or separate them.


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

The Page essentially takes and uses PackableStream[Buffer]. There is no
good reason to keep using C-style printf()s if we have to change the
output anyway.


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

I doubt we can render two formats (e.g., text and YAML) _nicely_ using
one rendering code (with a few configurable delimiters), but if that
works, great! Note that YamlPage and PlainPage should only contain the
code unique to each format, so if those classes end up being empty, it
would be trivial to remove them completely. Actions do not know about
those classes because they use Page (instead of the current StoreEntry).


>>     virtual ~Page() {}
>>
>>     // a comment
>>     virtual Page &comment(const SBuf &text) = 0;
>>
> 
> I was suggesting just the above bit in the first patch.

Fine with me, but your thoughts below seem to indicate that it is not
actually what you want to do (or that it is impossible to do using a
comment()).


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

Then comment() is not appropriate. It would be appropriate for something
like "no data collected for this page" or "internal error while
rendering statistics for this page". That is, for something that scripts
do not need to interpret (beyond the fact that there no actual data in
the output).

You may add a Page::announce() method of some sort, of course, but it
has to be _implemented_ using basic YAML constructs like lists, hashes,
key:value pairs, comments, etc. Thus, you may need some of the other
methods from the sketch. This is important: To avoid changing same
output more than once, we have to use the right constructs in any
changed code.


HTH,

Alex.