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