General comment:

The typedef for the shared_ptr declare in LogSegment.h: using LogSegmentRef = std::shared_ptr<LogSegment>;
I didn"t find a way to enable forward declaration for it.
So I'd to explicitly use std::shared_ptr<LogSegment> in some of the H files (the ones w/o #include "LogSegment.h")

For example, trying to use LogSegmentRef in CDentry.h resolved in:

../src/mds/CDentry.h:253:20: error: ‘LogSegmentRef’ has not been declared
  253 |   void _mark_dirty(LogSegmentRef& ls);
      |                    ^~~~~~~~~~~~~
``

General comment:

The typedef for the shared_ptr declare in LogSegment.h: using LogSegmentRef = std::shared_ptr<LogSegment>; I didn"t find a way to enable forward declaration for it. So I'd to explicitly use std::shared_ptr<LogSegment> in some of the H files (the ones w/o #include "LogSegment.h")

For example, trying to use LogSegmentRef in CDentry.h resolved in:

../src/mds/CDentry.h:253:20: error: ‘LogSegmentRef’ has not been declared
  253 |   void _mark_dirty(LogSegmentRef& ls);
      |                    ^~~~~~~~~~~~~
``

Since a pointer type can just work with forward declared class/struct types, the LogSegment.h header may not have been included in all the places where LogSegment* is used.

You might have to exhaustively check for those cases and explicitly include the LogSegment.h header to make the LogSegmentRef definition available appropriately.

OR

You'll need to place the using declaration for LogSegmentRef in all files where the LogSegment.h header has not been included explicitly.
eg.

class LogSegment;
using LogSegmentRef = std::shared_ptr<LogSegment>;

this should work in .cc as well as .h files.

You might have to exhaustively check for those cases and explicitly include the LogSegment.h header to make the LogSegmentRef definition available appropriately.

OR

You'll need to place the using declaration for LogSegmentRef in all files where the LogSegment.h header has not been included explicitly. eg.

class LogSegment;
using LogSegmentRef = std::shared_ptr<LogSegment>;

yes, you are right it's legal to declare the same typedef multiple times..but it may cause maintenance issue.
Yet, my current approach is also kind of duplicating the typedef.
So I think your suggestion is cleaner, I"ll apply it. thanks.

@tshacked Genrally, the use of using in header files is considered a bit off-putting.

WDYT?

Actually. I wasn't aware of it.
The using keyword here is for getting the typedef-name LogSegmentRef. Is there other alternative?

@tshacked Genrally, the use of using in header files is considered a bit off-putting.

WDYT?

Actually. I wasn't aware of it.

The using keyword here is for getting the typedef-name LogSegmentRef. Is there other alternative?

This is definitely our standard pattern for Ref types AFAIK. We don't like to do an import of the standard library with something like "using namespace std;" but this should be fine. @vshankar

@tshacked Please add the signed-off-by for all the commits.

@tshacked Please add the signed-off-by for all the commits.

@lxbsz it is missing in 2 commits which were generated by github when I accepted reviewer suggestions.
Is it not good enough that it appears on the main commit-msg? is it mandatory for each commit?

@tshacked Genrally, the use of using in header files is considered a bit off-putting.

WDYT?

Actually. I wasn't aware of it.
The using keyword here is for getting the typedef-name LogSegmentRef. Is there other alternative?

This is definitely our standard pattern for Ref types AFAIK. We don't like to do an import of the standard library with something like "using namespace std;" but this should be fine. @vshankar

(sorry for getting back late on this)

ACK. Makes sense (although it would have been a tiny bit better if we could have possibly use the same convention, but the MDS codebase is a bit weirdly organized :)

@tshacked you do plan to squash the commits (some, not all) when this is ready to be QA'd, isn't it?

jenkins retest this please

jenkins test make check

This change causes the following failed test

• https://pulpito.ceph.com/vshankar-2022-10-12_04:56:59-fs-wip-vshankar-testing-20221011-145847-testing-default-smithi/7063202/
• https://pulpito.ceph.com/vshankar-2022-10-12_04:56:59-fs-wip-vshankar-testing-20221011-145847-testing-default-smithi/7063307/

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!