Thanks, but it isn’t clear to me exactly what is at stake here. If this is a “design” level proposal, perhaps it should go through the RFC process rather than straight to a vote.
Either way, a short summary of the problem description, the possible paths forward, and the advantages/disadvantages of each might be helpful context before we start voting. > On Dec 16, 2019, at 12:13 PM, Mark Hanson <mhan...@pivotal.io> wrote: > > It has been said I have a negative vote which is counter intuitive. > > VOTE SUBJECT: > > Should we continue migrating from AttributesFactory usage to RegionFactory > usage and merge the RegionFactory copy constructor. > > > +1 to Migrate to RegionFactory from AttributesFactory and merge the > RegionFactory copy constructor > 0 don’t care > -1 stop migrating from AttributesFactory to RegionFactory and wait for > Management V2 API. > > >> On Dec 16, 2019, at 11:08 AM, Mark Hanson <mhan...@pivotal.io> wrote: >> >> 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 >>> <mailto: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 >>>> <mailto: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 >>>>> <mailto: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 >>>>>>> <mailto: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 >>>>>>>>> <mailto: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 >>>>>>>>>> <mailto: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> < >>>>>> 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 >>>>>> >> >
