Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
753ee74
Maintenance: add ostream report API to Mgr::Action
yadij Apr 29, 2024
58d9f91
Remove code comment
yadij May 7, 2024
60d3225
Merge branch 'master' into arc-mgr-ostream-4
yadij May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/mgr/Action.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
/* DEBUG: section 16 Cache Manager API */

#include "squid.h"
#include "base/PackableStream.h"
#include "CacheManager.h"
#include "comm/Connection.h"
#include "HttpReply.h"
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


entry->flush();

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

void
Mgr::Action::dump(StoreEntry *entry)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
PackableStream os(*entry);
report(os);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
9 changes: 8 additions & 1 deletion src/mgr/Action.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#include "ipc/forward.h"
#include "mgr/forward.h"

#include <iosfwd>

class StoreEntry;

namespace Mgr
Expand Down Expand Up @@ -76,11 +78,16 @@ class Action: public RefCountable
/// calculate and keep local action-specific information
virtual void collect() {}

/// write manager report output to a stream.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
*
* \deprecated implement report() instead
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

virtual void dump(StoreEntry *) {}
virtual void dump(StoreEntry *);

private:
const CommandPointer cmd; ///< the command that caused this action
Expand Down
1 change: 1 addition & 0 deletions src/tests/stub_libmgr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const char * Mgr::Action::name() const STUB_RETVAL(nullptr)
static Mgr::Command static_Command;
const Mgr::Command & Mgr::Action::command() const STUB_RETVAL(static_Command)
StoreEntry * Mgr::Action::createStoreEntry() const STUB_RETVAL(nullptr)
void Mgr::Action::dump(StoreEntry *) STUB
static Mgr::Action::Pointer dummyAction;

#include "mgr/ActionParams.h"
Expand Down
Loading