Re: [VOTE] Adding a couple user APIs dealing with RegionFactory.

[Mark Hanson]

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

Re: [VOTE] Adding a couple user APIs dealing with RegionFactory.

[Owen Nichols]

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  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 > > 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 >>  >> >> 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 >>>  >> 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?
> 
> +   RegionFactory createRegionFactory(RegionFactory
> regionFactory);
> 
> On Fri, Dec 13, 2019 at 9:35 AM Mark Hanson    >> wrote:
> 
>> Hi Udo,
>> 
>> I disagree. I believe the currently published best practice is to use
>> RegionFactory. As a part of the w

Re: [VOTE] Adding a couple user APIs dealing with RegionFactory.

[Owen Nichols]

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  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  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 >> > 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 >>> > 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?
> 
> +   RegionFactory createRegionFactory(RegionFactory
> regionFactory);
> 
> On Fri, Dec 13, 2019 at 9:35 AM Mark Hanson  > 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
>

Re: [VOTE] Adding a couple user APIs dealing with RegionFactory.

[Mark Hanson]

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  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 > > 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 >> > 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?
 
 +   RegionFactory createRegionFactory(RegionFactory
 regionFactory);
 
 On Fri, Dec 13, 2019 at 9:35 AM Mark Hanson >>> > 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 > >
> 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 

[VOTE] Adding a couple user APIs dealing with RegionFactory.

[Mark Hanson]

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  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  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?
>>> 
>>> +   RegionFactory createRegionFactory(RegionFactory
>>> regionFactory);
>>> 
>>> On Fri, Dec 13, 2019 at 9:35 AM Mark Hanson  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 
 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 
>>> 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 
>>> 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/ap