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

@adamemerson @pritha-srivastava @mattbenjamin i think this PR would benefit from a similar layer-collapsing (see #41798) to drop the RGWSI_Role and RGWSI_Role_RADOS services. the rest of this looks pretty reasonable

I would hold off on some of this. @dang mentioned having plans to pull apart SysObj into a few different kinds of object as part of Zipper in the last refactor meeting, so rewriting things in terms of the services might be premature.

I would hold off on some of this. @dang mentioned having plans to pull apart SysObj into a few different kinds of object as part of Zipper in the last refactor meeting, so rewriting things in terms of the services might be premature.

zipper got a rgw::sal::RGWRole interface since this PR was created, so we'll need to rebase with that in mind. but any calls involving SysObj should be internal to rgw::sal::RadosRole so i don't think we're blocked on anything here

thanks @cbodley @adamemerson , I will look into #41798

@adamemerson @pritha-srivastava @mattbenjamin i think this PR would benefit from a similar layer-collapsing (see #41798) to drop the RGWSI_Role and RGWSI_Role_RADOS services. the rest of this looks pretty reasonable

@cbodley : I think I understand what is to be done to merge Dan's rgw::sal changes, Abhishek's changes here and collapsing the services. Just to be clear here - I need to collapse RGWSI_Role_RADOS and RGWSI_Role into one class (RGWSI_Role) as has been done in #41798?

@adamemerson @pritha-srivastava @mattbenjamin i think this PR would benefit from a similar layer-collapsing (see #41798) to drop the RGWSI_Role and RGWSI_Role_RADOS services. the rest of this looks pretty reasonable

@cbodley : I think I understand what is to be done to merge Dan's rgw::sal changes, Abhishek's changes here and collapsing the services. Just to be clear here - I need to collapse RGWSI_Role_RADOS and RGWSI_Role into one class (RGWSI_Role) as has been done in #41798?

honestly i would prefer not to have a role service at all. i'm not convinced we need the RGWRoleCtl either, if rgw::sal::RGWRole already covers all of its functionality

@adamemerson @pritha-srivastava @mattbenjamin i think this PR would benefit from a similar layer-collapsing (see #41798) to drop the RGWSI_Role and RGWSI_Role_RADOS services. the rest of this looks pretty reasonable

@cbodley : I think I understand what is to be done to merge Dan's rgw::sal changes, Abhishek's changes here and collapsing the services. Just to be clear here - I need to collapse RGWSI_Role_RADOS and RGWSI_Role into one class (RGWSI_Role) as has been done in #41798?

honestly i would prefer not to have a role service at all. i'm not convinced we need the RGWRoleCtl either, if rgw::sal::RGWRole already covers all of its functionality

rgw::sal::RGWRole with rgw::sal::RadosRole covers all the functionality. But where is the code that handles the replication, if we do away with the service classes?

@adamemerson @pritha-srivastava @mattbenjamin i think this PR would benefit from a similar layer-collapsing (see #41798) to drop the RGWSI_Role and RGWSI_Role_RADOS services. the rest of this looks pretty reasonable

@cbodley : I think I understand what is to be done to merge Dan's rgw::sal changes, Abhishek's changes here and collapsing the services. Just to be clear here - I need to collapse RGWSI_Role_RADOS and RGWSI_Role into one class (RGWSI_Role) as has been done in #41798?

honestly i would prefer not to have a role service at all. i'm not convinced we need the RGWRoleCtl either, if rgw::sal::RGWRole already covers all of its functionality

rgw::sal::RGWRole with rgw::sal::RadosRole covers all the functionality. But where is the code that handles the replication, if we do away with the service classes?

Should the zone info and be_handler be added to RadosRole/RGWRole for that?

But where is the code that handles the replication, if we do away with the service classes?

metadata sync uses RGWMetadataManager::get() to read metadata, and RGWMetadataManager::put() to write it. for each 'type' of metadata, we register a RGWMetadataHandler with the manager so it knows how to read/write that type. so it's the RGWRoleMetadataHandler part that really matters here

past that, i'm not really clear on how the backend stuff fits in

Should the zone info and be_handler be added to RadosRole/RGWRole for that?

ok, i guess i missed all the stuff with class RGWSI_Role_Module and the 'backend handler'.. i take back what i said about "the rest of this looks pretty reasonable" :(

i guess the only purpose of the RGWSI_Role service is to provide ownership of this RGWSI_Role_Module memory? i wonder if we can store that in RGWRoleMetadataHandler or RGWMetadataManager instead

But where is the code that handles the replication, if we do away with the service classes?

metadata sync uses RGWMetadataManager::get() to read metadata, and RGWMetadataManager::put() to write it. for each 'type' of metadata, we register a RGWMetadataHandler with the manager so it knows how to read/write that type. so it's the RGWRoleMetadataHandler part that really matters here

past that, i'm not really clear on how the backend stuff fits in

Should the zone info and be_handler be added to RadosRole/RGWRole for that?

ok, i guess i missed all the stuff with class RGWSI_Role_Module and the 'backend handler'.. i take back what i said about "the rest of this looks pretty reasonable" :(

i guess the only purpose of the RGWSI_Role service is to provide ownership of this RGWSI_Role_Module memory? i wonder if we can store that in RGWRoleMetadataHandler or RGWMetadataManager instead

Yes, it looks like RGWSI_Role_Module and struct Svc {
RGWSI_Zone *zone{nullptr};
RGWSI_Meta *meta{nullptr};
RGWSI_MetaBackend *meta_be{nullptr};
RGWSI_SysObj *sysobj{nullptr};
} svc; can be added to RGWRoleMetadataHandler (or RadosRole), Can we do away with the be_handler? I can see that being used in RGWCtl, which ultimately calls svc.meta_be (which is RGWSI_MetaBackend_Sobj). I think the RGW_SI_Role and RGW_SI_Role_RADOS can be done away with if we move the above two (module and svc) to RGWRoleMetadataHandler. It appears that we can also do away with RGWCtl class and have the functionality in RadosRole (introduced by Dan's refactoring).
I can see that RGWRoleMetadataHandler gets instantiated in RGWCtlDef::init and passed in the constructor of RGWRoleCtl, but don't see it being used in the methods of RGWRoleCtl.

We have the option of ditching the entire RGWSI and module and backend business and just subclassing RGW Metadata Handler directly, which would be my preference, if we have to involve the RGW Metadata Handler stuff at all.

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 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!