Hi All, In working with Udo, I moved the needed public API into a helper class. The RegionFactory copy constructor still remains in RegionFactory but is protected. The only way to use the method now is by creating a helper class that extends RegionFactory.
Thanks for you all your help. I think this resolves the discussion. Thanks, Mark > On Dec 16, 2019, at 12:56 PM, Owen Nichols <onich...@pivotal.io> wrote: > > A -1 vote on a code change should be framed as a “request for change”. Udo, > you’ve made it clear what you don’t want, but not what it would take to make > PR #4409 acceptable to you. > > “Management V2 API” is unlikely to solve all problems in the near term, and > even to do so, it needs a sound underlying API to build on. Can you suggest > a way to resolve this that makes sense both for the current codebase as well > as paving the eventual way for the future? > >> 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 >>> <mailto: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> <mailto: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> <mailto: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> <mailto: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> <mailto: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> <mailto: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> <mailto: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>> < >>>>>>> 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 >
