Actually, I would say that it would not be necessary to have a copy constructor if it were not for the way the tests are written that assume an AttributesFactory. I think the discussion boils down to this…
Do we migrate to the RegionFactory API from AttributesFactory or do we wait for the Management V2 API. I would heartily support the V2 API, I have used the REST version of it and it was great. That said, when will that arrive. We currently have a deprecated API and a current (wrapper) API. The Management V2 is an expected, but not realized API at this point. As a community, I would like to decide if I should revert my changes to modernize the test, but going to RegionFactory and just put in the core fixes. I can do that. I am fine with that. I just don’t want to sit in the middle. I don’t see how I can move to RegionFactory API without a significant test restructuring without the copy constructor. VOTE SUBJECT: Stop migrating from AttributesFactory to RegionFactory and wait for Management V2 API. Again, I am 100% fine either way, I just want a clear direction. :) This would mean reverting my changes to update, (totally fine) and then putting in only the fixes. In the near future, we would also stop migrating from AttributesFactory to RegionFactory while we wait for the Management V2 API. Thanks, Mark > On Dec 16, 2019, at 10:47 AM, Udo Kohlmeyer <ukohlme...@pivotal.io> wrote: > > -1 to adding a copy constructor onto any /**Factory*/ classes. > > I have never seen a copy constructor on a Factory pattern in the wild. > > That said, having done some Googling, this is something that people talk > about, so it cannot be THAT foreign. > > There is one thing I want to point out here. There is work currently being > done on the new Management/Configuration V2 API's. One of the goals of this > project is to have a single API to create/update/delete and management the > configuration and creation (realization of configuration). I'm advocating > that new Management/Configuration v2 API's need to be interacting with it, > rather than a user directly. This, adding a "copy constructor" of > configuration should be added onto the v2 API's rather than on the > /**Factory*/ classes. > > That said, having */*Factories/* as public API's that are use to create a > /*Region*/, that is correct for the module. But any utility capability to > copy configuration needs to be addressed by the Management API and not the > /*Factory*/ classes. > > --Udo > > > On 12/13/19 10:01 AM, Darrel Schneider wrote: >> The v2 management api allows regions to be created remotely in cluster >> configuration and on running servers. But it does not allow you to create a >> region on a client. Non-spring applications can use RegionFactory to create >> the region on the client side. >> >> >> On Fri, Dec 13, 2019 at 9:56 AM Dan Smith <dsm...@pivotal.io> wrote: >> >>> +1 to adding a way to copy the RegionAttributes. >>> >>> BTW, I really wish this RegionFactory was an interface. I don't know if >>> adding a copy constructor makes it harder to migrate to an interface later, >>> but maybe just having this single public method might be better? >>> >>> + <K, V> RegionFactory<K, V> createRegionFactory(RegionFactory<K, V> >>> regionFactory); >>> >>> On Fri, Dec 13, 2019 at 9:35 AM Mark Hanson <mhan...@pivotal.io> wrote: >>> >>>> Hi Udo, >>>> >>>> I disagree. I believe the currently published best practice is to use >>>> RegionFactory. As a part of the work I have been doing, I have been >>>> migrating code from the AttributesFactory pattern to the RegionFactory >>>> pattern. To support that, I believe a copy constructor for RegionFactory >>> is >>>> necessary. And thus a createRegionFactory >>>> >>>> Hence my changes. >>>> >>>> Thanks, >>>> Mark >>>> >>>>> On Dec 11, 2019, at 4:41 PM, Udo Kohlmeyer <ukohlme...@pivotal.io> >>>> wrote: >>>>> I think at this point I'd be looking at the new V2 Management API's for >>>> Regions. >>>>> I think any new "public" effort that we'd be adding to the product >>>> should be done through the Management API's for Regions, rather than >>>> exposing new public API's that in reality should not be made "public". >>>>> --Udo >>>>> >>>>> On 12/11/19 3:53 PM, Mark Hanson wrote: >>>>>> Basically the point is to allow a use to copy a RegionFactory because >>>> under certain circumstances it is necessary. I found that when migrating >>>> from AttributesFactory. >>>>>> Thanks, >>>>>> Mark >>>>>> >>>>>>> On Dec 11, 2019, at 3:48 PM, Anthony Baker <aba...@pivotal.io> >>> wrote: >>>>>>> Mark, >>>>>>> >>>>>>> Can you share how the API changes will help the user? >>>>>>> >>>>>>> Thanks, >>>>>>> Anthony >>>>>>> >>>>>>> >>>>>>>> On Dec 11, 2019, at 2:57 PM, Mark Hanson <mhan...@pivotal.io> >>> wrote: >>>>>>>> Hi All, >>>>>>>> >>>>>>>> There was a suggestion that since I am making a couple user visible >>>> API changes that I might want to ask the dev list. >>>>>>>> Basically I was migrating code from AttributesFactory to >>>> RegionFactory and hit a few snags along the way. >>>>>>>> Please see https://github.com/apache/geode/pull/4409 < >>>> https://github.com/apache/geode/pull/4409> specifically Cache.java, >>>> RegionFactory.java, and for extra credit GemfireCacheImpl.java >>>>>>>> I have commented at the relevant changes. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Mark >>>>
