Maintenance: add ostream report API to Mgr::Action #1806

yadij · 2024-04-29T21:39:59Z

This new API method allows new or updated cache manager reports
to write their output to a stream without having to import any
StoreEntry related API.

This new API method allows new or updated cache manager reports to write their output to a stream without having to import any StoreEntry related API.

rousskov

This PR duplicates the primary problem of still-open PR #1560. We have already spent a lot of time on that problem. IMHO, it is unethical to repost these changes like that, without any cross references and warnings.

yadij · 2024-05-02T22:35:55Z

This PR duplicates the primary problem of still-open PR #1560. We have already spent a lot of time on that problem. IMHO, it is unethical to repost these changes like that, without any cross references and warnings.

Really? AFIAK this is the only part of that PR which has not been complained about. With this vetoed you are now essentially blocking conversion of cachemgr to using ostream for report generation.

rousskov

This PR duplicates the primary problem of still-open PR #1560.

AFIAK this is the only part of that PR which has not been complained about.

The two change requests in the latest #1560 (review) (dated November 10, 2023) are clearly attached to code duplicated in this PR. There are no other open change requests. This PR contains the primary part of PR 1560 that is currently¹ blocking that PR.

With this vetoed you are now essentially blocking conversion of cachemgr to using ostream for report generation.

I am not. I am blocking one problematic conversion. There are ways to determine the right conversion goal (which could be std::ostream or some custom class) and make progress in this area, but this duplicated PR is not the right place to discuss any of that.

Some other problematic parts may have been fixed earlier or may be discovered in the future, of course. I am only describing the current state. ↩

Apparently marking an unwanted API as deprecated is enough to seriously break Squid somehow.

yadij · 2024-05-07T12:36:04Z

This PR contains the primary part of PR 1560 that is currently1 blocking that PR.

Surely you jest. Words on a code comment are not sufficient technical reason to veto maintenance updates.

I have removed the comment entirely to avoid further conflict. If you have an actual technical reason to block, please state clearly what that is.

rousskov

Alex: This PR contains the primary part of PR 1560 that is currently blocking that PR.

Amos: Surely you jest. Words on a code comment are not sufficient technical reason to veto maintenance updates.

My text stated a fact. It was not a joke. I do not block PRs in jest.

Words in a code comment may be important enough to block a PR, especially words containing deprecation notices (and TODO plans), as was the case in PR 1560.

Our definitions of "maintenance update" probably differ, but that difference is probably not important right now. After this PR code changes finalize, I may request the removal of controversial "Maintenance" prefix from this PR title. Please consider removing it now to save time.

rousskov · 2024-05-07T13:30:14Z

src/mgr/Action.h

@@ -76,11 +78,14 @@ class Action: public RefCountable
    /// calculate and keep local action-specific information
    virtual void collect() {}

+    /// write manager report output to a stream.


The proposed description does not differentiate this new method enough from the existing dump() method. AFAICT, the new method is not usable for actions that cannot write the entire report immediately (i.e. for non-atomic() actions). Those actions should continue to use the dump() method. We should clarify that.

Also, there is not enough value in converting existing dump() code to report() unless that conversion also establishes YAML compliance. Thus, we should make this new method specific to YAML reports.

For example:

Suggested change

/// write manager report output to a stream.

/// Write the entire YAML-compliant report to the given stream.

/// Actions that cannot produce YAML reports and non-atomic()

/// actions need to override dump() method instead.

rousskov · 2024-05-07T13:35:14Z

src/mgr/Action.cc

@@ -112,11 +113,17 @@ Mgr::Action::fillEntry(StoreEntry* entry, bool writeHttpHeader)
        entry->replaceHttpReply(rep);
    }

-    dump(entry);
+    dump(entry); // TODO: replace with report() when all children are converted


I have removed the comment entirely

You have not removed this TODO comment (that was one of the two detailed reasons for blocking PR 1560). The other changes in this PR do not address the corresponding PR 1560 concerns.

Suggested change

dump(entry); // TODO: replace with report() when all children are converted

dump(entry);

rousskov · 2024-05-07T13:45:29Z

src/mgr/Action.cc

+Mgr::Action::dump(StoreEntry *entry)
+{
+    PackableStream os(*entry);
+    report(os);


We should decide whether std::ostream is the best type for future Actions. Ongoing YAML conversion efforts suggest that it might not be. Hopefully, we will know the answer after the spaces() issue is resolved. I will come back to this concern at that time.

Besides that, why not use an overload for supporting the transition?
report() is a better name, and I'd support for it but we can do it with a different strategy:

decide what the API will look like

overload dump()

progressively convert and possibly rename (or keep dump to avoid purely cosmetic changes)

I believe C++ term "overload" is not applicable here, but the rest of your description matches my expectations of the anticipated transition. None of my blocking change requests contradict that natural transition strategy.

If you did mean that the new method should reuse the old dump() method name (i.e. that we should overload the old method), then I am currently against that particular aspect of the transition sketch because report() is a better name, because more visual clues identifying transitioned callers may be helpful, and because overload adds complexity.

rousskov · 2024-05-07T13:51:29Z

src/mgr/Action.h

@@ -76,11 +78,14 @@ class Action: public RefCountable
    /// calculate and keep local action-specific information
    virtual void collect() {}

+    /// write manager report output to a stream.
+    virtual void report(std::ostream &) {}
+
    /** start writing action-specific info to Store entry;
     * may collect info during dump, especially if collect() did nothing
     * non-atomic() actions may continue writing asynchronously after returning
     */


Suggested change

*/

* Use report() to produce atomic() YAML-compliant reports.

*/

rousskov · 2024-05-07T13:54:21Z

src/mgr/Action.cc


    entry->flush();

    if (atomic())
        entry->complete();
 }

+void
+Mgr::Action::dump(StoreEntry *entry)


Suggested change

Mgr::Action::dump(StoreEntry *entry)

Mgr::Action::dump(StoreEntry * const entry)

Maintenance: add ostream report API to Mgr::Action

753ee74

This new API method allows new or updated cache manager reports to write their output to a stream without having to import any StoreEntry related API.

kinkie previously approved these changes Apr 29, 2024

View reviewed changes

rousskov self-requested a review April 30, 2024 18:44

rousskov added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Apr 30, 2024

rousskov requested changes Apr 30, 2024

View reviewed changes

rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Apr 30, 2024

rousskov reviewed May 2, 2024

View reviewed changes

Remove code comment

58d9f91

Apparently marking an unwanted API as deprecated is enough to seriously break Squid somehow.

yadij dismissed kinkie’s stale review via 58d9f91 May 7, 2024 12:26

yadij requested a review from rousskov May 7, 2024 12:26

yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 7, 2024

yadij requested a review from kinkie May 7, 2024 12:27

Merge branch 'master' into arc-mgr-ostream-4

60d3225

yadij mentioned this pull request May 7, 2024

Refactor the basic manager Action classes for Action::report() #1560

Open

rousskov requested changes May 7, 2024

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label May 7, 2024

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintenance: add ostream report API to Mgr::Action #1806

Maintenance: add ostream report API to Mgr::Action #1806

yadij commented Apr 29, 2024

rousskov left a comment

yadij commented May 2, 2024

rousskov left a comment

yadij commented May 7, 2024

rousskov left a comment

rousskov May 7, 2024

rousskov May 7, 2024

rousskov May 7, 2024

kinkie May 8, 2024

rousskov May 8, 2024

rousskov May 8, 2024

rousskov May 7, 2024

rousskov May 7, 2024

-    /// write manager report output to a stream.
+    /// Write the entire YAML-compliant report to the given stream.
+    /// Actions that cannot produce YAML reports and non-atomic()
+    /// actions need to override dump() method instead.

	dump(entry); // TODO: replace with report() when all children are converted
	dump(entry);

	*/
	* Use report() to produce atomic() YAML-compliant reports.
	*/

	Mgr::Action::dump(StoreEntry *entry)
	Mgr::Action::dump(StoreEntry * const entry)

Maintenance: add ostream report API to Mgr::Action #1806

Are you sure you want to change the base?

Maintenance: add ostream report API to Mgr::Action #1806

Conversation

yadij commented Apr 29, 2024

rousskov left a comment

Choose a reason for hiding this comment

yadij commented May 2, 2024

rousskov left a comment

Choose a reason for hiding this comment

Footnotes

yadij commented May 7, 2024

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment