cloud_roles: convert to new-style module #17519
Conversation
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
partition exposes the archival manifest stm. making it fwd declaration means that anyone including the header has to make sure to include other headers. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
This was picking up the unexpected formatting of operator<< defined as printing out milliseconds instead of `Ns` format which is assumed by ducktape tests. Removing the lowres operator<< overload is a bigger project. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
If it is not set then nullptr is passed into strlen while building the sstring, and its undefined behavior that is caught by address sanitizer. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47136#018e9746-a61f-454e-ae2d-b4f8d5698fb0 |
dotnwat
requested review from
abhijat,
rockwotj,
andrwng,
mmaslankaprv and
graphcareful
| #include "apply_abs_credentials.h" | ||
| #include "apply_abs_oauth_credentials.h" | ||
| #include "apply_aws_credentials.h" | ||
| #include "apply_gcp_credentials.h" |
There was a problem hiding this comment.
Can you remind me: is the removal of the prefixing module a requirement of becoming a new-style module? Or is this to make it clearer what is internal or external? It'd be great for reviewers and future readers to leave some notes in the commit message about the distinction
There was a problem hiding this comment.
the public headers are moved into the <module>/include/<module> directory. everything else is remaining is now private. that means we have to remove the prefix, otherwise it'd be referring to the headers in the public directory, but these are private.
There was a problem hiding this comment.
Ah, got it. Needed a reminder that prefixed includes will be our public interface, and everything else is private. Thanks for explaining (again)
| @@ -28,7 +29,6 @@ | |||
|
|
|||
| namespace cluster { | |||
| class partition_manager; | |||
| class archival_metadata_stm; | |||
There was a problem hiding this comment.
making it fwd declaration
means that anyone including the header has to make sure to include other
headers.
I'm kind of confused about this. Do you mean callers that want to use partition::archival_meta_stm() need to include archival_metadata_stm.h? Or that just including cluster/partition.h always mandates also including archival/archival_metadata_stm.h? If the former, that seems like expected behavior? And if the latter, it's not quite clear to me why that is -- could you point out why that is?
There was a problem hiding this comment.
What I mean is that archival_metadata_stm is part of the interface, rather than the implementation. So if my module includes partition.h then only part of the public interface exported by the header will work without including additional headers. the fwd declaration is usually only used when the interface takes a reference and the definition is imported in the .cc file. example:
class {
public:
archival_metadata_stm& get_stm();
};vs
class {
private:
archival_metadata_stm& stm;
};thoughts?
There was a problem hiding this comment.
So if my module includes partition.h then only part of the public interface exported by the header will work without including additional headers
Right, but I'm wondering why that's problematic. I get that the type is part of the interface, but the implementation of the type technically isn't, necessarily, unless a user of partition also starts calling functions of the stm, no? Nothing in the header requires knowledge of implementation details of the stm
I guess a secondary question then is, will similar changes also need to be done for all the other getters that return pointers/references? Looking at the remote partition, manifest view, feature table, etc.
There was a problem hiding this comment.
discussed offline. generally it's nice to have headers not require other headers to be able to use their interfaces. this particular change was just to get something to compile i suppose i could have / should have had the user include the archival stm header instead. alternatively, and i think preferred, is the more effort required approach of making the archival stm header cheaper to include.
There was a problem hiding this comment.
definitely feel free to change back. the module conversion project is going to be 100 steps forward and 20 steps backwards. a second pass after conversion can resolve all of the remaining header-only dependencies and try to shrink the headers that are published publicaly.
cloud_roles: convert to new-style module
Backports Required
Release Notes