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

yadij · 2023-10-31T13:37:47Z

No description provided.

src/mgr/Action.h

kinkie

LGTM, I suppose there will be followups

src/mgr/Action.cc

src/mgr/Action.h

src/mgr/BasicActions.cc

Co-authored-by: Alex Rousskov <[email protected]>

kinkie

LGTM

src/mgr/Action.h

rousskov · 2023-11-10T21:02:44Z

src/mgr/Action.cc

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

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


With the PR-proposed "just write the report" semantics, it is not possible to convert some of the existing Mgr::Action children (and some future ones). For example, Mgr::FunAction objects that handle asynchronous mgr:objects and mgr:vm_objects reports cannot be converted because their existing dump() methods do more than "just writing the report" and, in non-SMP cases¹, do not "write the report" at all. The guts of their dump() actions are essentially one no-output line:

eventAdd("statObjects", statObjects, state, 0.0, 1);

The fundamental property of asynchronous cache manager actions cannot be removed by conversion or refactoring that might be relevant to this TODO. This TODO goes against core Action architecture/features that require that asynchronous production of some cache manager reports, including dump() calls that may not produce output (because an asynchronous action is required to start producing it) or do not finish producing output (again, because an asynchronous action is required to continue and finish producing output).

N.B. There is more about this problem in the other change request. I am not sure which change request has a better context for this discussion. The other change request has a specific suggestion for addressing both problems.

Footnotes

In the corresponding SMP cases, FunAction::dump() writes a "by kidN {" prefix. ↩

Those bad designs need to be replaced and are part of why this TODO has a condition ("when ...") stated.

Alex: With the PR-proposed "just write the report" semantics, it is not possible to convert some of the existing Mgr::Action children (and some future ones). ... The fundamental property of asynchronous cache manager actions cannot be removed by conversion or refactoring that might be relevant to this TODO. This TODO goes against core Action architecture/features that ...

Amos: Those bad designs need to be replaced and are part of why this TODO has a condition ("when ...") stated.

This change request has already evaluated and rejected that condition, providing detailed reasoning and examples. No new information or arguments have been provided in response to that change request. There is currently no consensus that any future hypothetical refactoring (unspecified in this PR) can justify this TODO existence.

kinkie · 2023-12-16T11:02:25Z

@yadij how would you like to proceed here?

yadij · 2024-05-07T12:44:41Z

This PR using Action::report() is now waiting on PR #1806 merge.

rousskov

This PR using Action::report() is now waiting on PR #1806 merge.

Understood. Please request my review when that controversial prerequisite PR is closed and this PR is adjusted accordingly.

rousskov · 2024-05-07T13:01:16Z

src/mgr/Action.cc

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

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


Alex: With the PR-proposed "just write the report" semantics, it is not possible to convert some of the existing Mgr::Action children (and some future ones). ... The fundamental property of asynchronous cache manager actions cannot be removed by conversion or refactoring that might be relevant to this TODO. This TODO goes against core Action architecture/features that ...

Amos: Those bad designs need to be replaced and are part of why this TODO has a condition ("when ...") stated.

This change request has already evaluated and rejected that condition, providing detailed reasoning and examples. No new information or arguments have been provided in response to that change request. There is currently no consensus that any future hypothetical refactoring (unspecified in this PR) can justify this TODO existence.

kinkie reviewed Oct 31, 2023

View reviewed changes

src/mgr/Action.h Show resolved Hide resolved

rousskov self-requested a review October 31, 2023 14:28

kinkie previously approved these changes Oct 31, 2023

View reviewed changes

rousskov requested changes Nov 2, 2023

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Nov 2, 2023

yadij dismissed kinkie’s stale review via eb79ab1 November 3, 2023 09:54

yadij and others added 4 commits November 8, 2023 16:30

Add Cache Manager stream output API

bb1f53d

Upgrade basic Actions to new display API

b0d03bc

Apply suggestions from code review

02bcd49

Co-authored-by: Alex Rousskov <[email protected]>

Rename print() to report()

9679054

yadij force-pushed the arc-mgr-ostream-1 branch from eb79ab1 to 9679054 Compare November 8, 2023 06:31

yadij added 4 commits November 8, 2023 19:43

Extra polish

646b162

MOre polish requested by reviewer

fcf81af

More polish requested by reviewer

30e3170

Restore old column widths

77d4df2

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Nov 9, 2023

yadij requested a review from rousskov November 9, 2023 10:28

rousskov removed their request for review November 9, 2023 14:08

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Nov 9, 2023

yadij mentioned this pull request Nov 10, 2023

Add Cache Manager report formatter #1583

Draft

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Nov 10, 2023

yadij requested review from rousskov and kinkie November 10, 2023 13:44

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

2fd67ba

rousskov mentioned this pull request Nov 22, 2023

Replace several String uses in cache manager #1596

Open

kinkie previously approved these changes Nov 26, 2023

View reviewed changes

rousskov requested changes Dec 11, 2023

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Dec 11, 2023

This comment was marked as resolved.

Sign in to view

rousskov added the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label Dec 14, 2023

This comment was marked as resolved.

Sign in to view

rousskov removed the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label Dec 14, 2023

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 Jan 9, 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 Feb 14, 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 Mar 5, 2024

kinkie added the M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels label Mar 15, 2024

rousskov mentioned this pull request Apr 30, 2024

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

Open

Update Action.h

bbff3af

yadij dismissed kinkie’s stale review via bbff3af May 7, 2024 12:40

yadij changed the title ~~Add Cache Manager stream output API~~ Refactor the basic cache manager Action classes for Action::report() May 7, 2024

yadij changed the title ~~Refactor the basic cache manager Action classes for Action::report()~~ Refactor the basic manager Action classes for Action::report() May 7, 2024

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

9b3482e

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

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

rousskov reviewed May 7, 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 May 7, 2024

kinkie added the M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels label Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yadij commented Oct 31, 2023 •

edited

Loading

kinkie left a comment

kinkie left a comment

rousskov Nov 10, 2023

yadij May 7, 2024

rousskov May 7, 2024

This comment was marked as resolved.

This comment was marked as resolved.

kinkie commented Dec 16, 2023

yadij commented May 7, 2024

rousskov left a comment

rousskov May 7, 2024

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

Are you sure you want to change the base?

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

Conversation

yadij commented Oct 31, 2023 • edited Loading

kinkie left a comment

Choose a reason for hiding this comment

kinkie left a comment

Choose a reason for hiding this comment

rousskov Nov 10, 2023

Choose a reason for hiding this comment

Footnotes

yadij May 7, 2024

Choose a reason for hiding this comment

rousskov May 7, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

kinkie commented Dec 16, 2023

yadij commented May 7, 2024

rousskov left a comment

Choose a reason for hiding this comment

rousskov May 7, 2024

Choose a reason for hiding this comment

yadij commented Oct 31, 2023 •

edited

Loading