Re: [VOTE] Inclusion of GEODE-7531 into 1.11

[John Blum]

+1

On Tue, Dec 17, 2019 at 3:25 PM Dick Cavender  wrote:

> +1
>
> On Tue, Dec 17, 2019 at 3:03 PM Patrick Johnson 
> wrote:
>
> > +1
> >
> > > On Dec 17, 2019, at 3:01 PM, Owen Nichols  wrote:
> > >
> > > +1
> > >
> > >> On Dec 17, 2019, at 2:57 PM, Udo Kohlmeyer  wrote:
> > >>
> > >> Hi there Apache Devs.
> > >>
> > >> I can we please vote on the inclusion of GEODE-7531. This issue is to
> > fix an bug which assumes that all `Pool` objects are of type `PoolImpl`
> and
> > immediately casts Pool -> PoolImpl.
> > >>
> > >> In the case of testing with Mocks, the type casts fail, because the
> > Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to
> type
> > check before the cast.
> > >>
> > >>
> > >> Regards
> > >>
> > >> Udo
> > >>
> > >
> >
> >
>


-- 
-John
Spring Data Team

Re: [VOTE] Inclusion of GEODE-7159 into 1.11

[John Blum]

+1

On Tue, Dec 17, 2019 at 3:25 PM Dick Cavender  wrote:

> +1
>
> On Tue, Dec 17, 2019 at 3:01 PM Patrick Johnson 
> wrote:
>
> > +1
> >
> > > On Dec 17, 2019, at 2:59 PM, Owen Nichols  wrote:
> > >
> > > +1
> > >
> > >> On Dec 17, 2019, at 2:58 PM, Udo Kohlmeyer  wrote:
> > >>
> > >> Hi there Apache Devs.
> > >>
> > >> I can we please vote on the inclusion of GEODE-7159. This issue is to
> > fix an bug which assumes that all `Pool` objects are of type `PoolImpl`
> and
> > immediately casts Pool -> PoolImpl. This issue is related to GEODE-7531,
> > and is specific to the emergencyClose method.
> > >>
> > >> In the case of testing with Mocks, the type casts fail, because the
> > Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to
> type
> > check before the cast.
> > >>
> > >>
> > >> Regards
> > >>
> > >> Udo
> > >>
> > >
> >
> >
>


-- 
-John
Spring Data Team

Re: [VOTE] Inclusion of GEODE-7531 into 1.11

[Dick Cavender]

+1

On Tue, Dec 17, 2019 at 3:03 PM Patrick Johnson  wrote:

> +1
>
> > On Dec 17, 2019, at 3:01 PM, Owen Nichols  wrote:
> >
> > +1
> >
> >> On Dec 17, 2019, at 2:57 PM, Udo Kohlmeyer  wrote:
> >>
> >> Hi there Apache Devs.
> >>
> >> I can we please vote on the inclusion of GEODE-7531. This issue is to
> fix an bug which assumes that all `Pool` objects are of type `PoolImpl` and
> immediately casts Pool -> PoolImpl.
> >>
> >> In the case of testing with Mocks, the type casts fail, because the
> Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to type
> check before the cast.
> >>
> >>
> >> Regards
> >>
> >> Udo
> >>
> >
>
>

Re: [VOTE] Inclusion of GEODE-7159 into 1.11

[Dick Cavender]

+1

On Tue, Dec 17, 2019 at 3:01 PM Patrick Johnson  wrote:

> +1
>
> > On Dec 17, 2019, at 2:59 PM, Owen Nichols  wrote:
> >
> > +1
> >
> >> On Dec 17, 2019, at 2:58 PM, Udo Kohlmeyer  wrote:
> >>
> >> Hi there Apache Devs.
> >>
> >> I can we please vote on the inclusion of GEODE-7159. This issue is to
> fix an bug which assumes that all `Pool` objects are of type `PoolImpl` and
> immediately casts Pool -> PoolImpl. This issue is related to GEODE-7531,
> and is specific to the emergencyClose method.
> >>
> >> In the case of testing with Mocks, the type casts fail, because the
> Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to type
> check before the cast.
> >>
> >>
> >> Regards
> >>
> >> Udo
> >>
> >
>
>

Re: [VOTE] Inclusion of GEODE-7531 into 1.11

[Patrick Johnson]

+1

> On Dec 17, 2019, at 3:01 PM, Owen Nichols  wrote:
> 
> +1
> 
>> On Dec 17, 2019, at 2:57 PM, Udo Kohlmeyer  wrote:
>> 
>> Hi there Apache Devs.
>> 
>> I can we please vote on the inclusion of GEODE-7531. This issue is to fix an 
>> bug which assumes that all `Pool` objects are of type `PoolImpl` and 
>> immediately casts Pool -> PoolImpl.
>> 
>> In the case of testing with Mocks, the type casts fail, because the 
>> Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to type 
>> check before the cast.
>> 
>> 
>> Regards
>> 
>> Udo
>> 
> 

Re: [VOTE] Inclusion of GEODE-7531 into 1.11

[Owen Nichols]

+1

> On Dec 17, 2019, at 2:57 PM, Udo Kohlmeyer  wrote:
> 
> Hi there Apache Devs.
> 
> I can we please vote on the inclusion of GEODE-7531. This issue is to fix an 
> bug which assumes that all `Pool` objects are of type `PoolImpl` and 
> immediately casts Pool -> PoolImpl.
> 
> In the case of testing with Mocks, the type casts fail, because the 
> Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to type 
> check before the cast.
> 
> 
> Regards
> 
> Udo
> 

Re: [VOTE] Inclusion of GEODE-7159 into 1.11

[Patrick Johnson]

+1

> On Dec 17, 2019, at 2:59 PM, Owen Nichols  wrote:
> 
> +1
> 
>> On Dec 17, 2019, at 2:58 PM, Udo Kohlmeyer  wrote:
>> 
>> Hi there Apache Devs.
>> 
>> I can we please vote on the inclusion of GEODE-7159. This issue is to fix an 
>> bug which assumes that all `Pool` objects are of type `PoolImpl` and 
>> immediately casts Pool -> PoolImpl. This issue is related to GEODE-7531, and 
>> is specific to the emergencyClose method.
>> 
>> In the case of testing with Mocks, the type casts fail, because the 
>> Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to type 
>> check before the cast.
>> 
>> 
>> Regards
>> 
>> Udo
>> 
> 

Re: [VOTE] Inclusion of GEODE-7159 into 1.11

[Owen Nichols]

+1

> On Dec 17, 2019, at 2:58 PM, Udo Kohlmeyer  wrote:
> 
> Hi there Apache Devs.
> 
> I can we please vote on the inclusion of GEODE-7159. This issue is to fix an 
> bug which assumes that all `Pool` objects are of type `PoolImpl` and 
> immediately casts Pool -> PoolImpl. This issue is related to GEODE-7531, and 
> is specific to the emergencyClose method.
> 
> In the case of testing with Mocks, the type casts fail, because the 
> Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to type 
> check before the cast.
> 
> 
> Regards
> 
> Udo
> 

[VOTE] Inclusion of GEODE-7159 into 1.11

[Udo Kohlmeyer]

Hi there Apache Devs.

I can we please vote on the inclusion of GEODE-7159. This issue is to 
fix an bug which assumes that all `Pool` objects are of type `PoolImpl` 
and immediately casts Pool -> PoolImpl. This issue is related to 
GEODE-7531, and is specific to the emergencyClose method.


In the case of testing with Mocks, the type casts fail, because the 
Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to 
type check before the cast.



Regards

Udo

[VOTE] Inclusion of GEODE-7531 into 1.11

[Udo Kohlmeyer]

Hi there Apache Devs.

I can we please vote on the inclusion of GEODE-7531. This issue is to 
fix an bug which assumes that all `Pool` objects are of type `PoolImpl` 
and immediately casts Pool -> PoolImpl.


In the case of testing with Mocks, the type casts fail, because the 
Pool-mocks are not of type PoolImpl. The simplest fix (for now) was to 
type check before the cast.



Regards

Udo

Re: Reviewer for GEODE-7534: Add example for query with bind params (documentation)

[Jason Huynh]

Hi Alberto,

Looks like Dan and Dave have both reviewed it.  I took a look and didn't
see anything wrong so I squash merged it in.

Thanks!
-Jason

On Fri, Dec 13, 2019 at 3:30 AM Alberto Gomez 
wrote:

> Hi,
>
> I'd appreciate some extra reviewer (I already had one, thanks @Dave
> Barnes) and if everything is ok someone to merge the following pull request:
>
> https://github.com/apache/geode/pull/4452
>
> Thanks,
>
> Alberto
>
>

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 

Odg: Odg: Odg: Odg: Odg: Lucene upgrade

[Mario Kevo]

Hi Jason,

Nice catch! I tried with larger number of retries(with your changes) and it 
passed.
I will try to make it time based.

Thanks for a help!

BR,
Mario

Šalje: Jason Huynh 
Poslano: 13. prosinca 2019. 23:10
Prima: Mario Kevo 
Kopija: geode 
Predmet: Re: Odg: Odg: Odg: Odg: Lucene upgrade

Hi Mario,

I think I see what is going on here.  The logic for "reindex" code was a bit 
off ( it expected reindex features to be complete by a certain release).  I 
have a PR on develop to adjust that calculation 
(https://github.com/apache/geode/pull/4466)

The expectation is that when lucene reindex (indexing a region with a data 
already in it) is enabled - any query will now throw the 
LuceneIndexingInProgressException instead of possibly waiting a very long time 
to receive a query result.  The tests themselves are coded to retry 10 times, 
knowing it will take awhile to reindex.  If you bump this number up or, better 
yet, make it time based (awaitility, etc), it should get you past this problem 
(once the pull request gets checked in and pulled into your branch)

Thanks!
-Jason


On Thu, Dec 12, 2019 at 5:07 AM Mario Kevo  wrote:
Hi Jason,

Yes, the same tests failed:

RollingUpgradeQueryReturnsCorrectResultAfterTwoLocatorsWithTwoServersAreRolled

RollingUpgradeQueryReturnsCorrectResultsAfterServersRollOverOnPartitionRegion

Sometimes this tests passed but more times it failed.
As I said when change tests to put lower number of entries it passed every time 
or set to wait for repo in LuceneQueryFunction.java.

waitUntilFlushed is called by verifyLuceneQueryResults before executing 
queries. Also tried to wait until isIndexingInProgress return false, but 
reached timeout and failed.
In tests it tried to execute a query after all members are rolled.

BR,
Mario


Šalje: Jason Huynh mailto:jhu...@pivotal.io>>
Poslano: 11. prosinca 2019. 23:08
Prima: Mario Kevo 
Kopija: geode mailto:dev@geode.apache.org>>
Predmet: Re: Odg: Odg: Odg: Lucene upgrade

Hi Mario,

Is the same test failing?  If it's a different test, could you tell us which 
one?
If it's a rolling upgrade test, then we might have to mark this as expected 
behavior and modify the tests to waitForFlush (wait until the queue is 
drained).  As long as the test is able to roll all the servers and not get 
stuck waiting for a queue to flush (which will only happen once all the servers 
are rolled now).

If the test hasn't rolled all the servers and is trying to execute a query, 
then we'd probably have to modify the test to not do the query in the middle or 
expect that exception to occur.

Thanks,
-Jason

On Wed, Dec 11, 2019 at 6:43 AM Mario Kevo  wrote:
Hi Jason,

This change fix IndexFormatTooNewException, but now we have

 org.apache.geode.cache.lucene.LuceneQueryException: Lucene Index is not 
available, currently indexing

So this means that query doesn't wait until all indexes are created.
In LuceneQueryFunction.java it is set to not wait for repo [execute(context, 
false)]. If we have a bigger queue(like in the test) it will failed as it will 
not wait until indexes are created. I also tried to put just few objects and it 
passed as it had enough time to create indexes.
Do we need to change this part to wait for repo, or put a lower number of 
entries in tests?

BR,
Mario




Šalje: Jason Huynh mailto:jhu...@pivotal.io>>
Poslano: 6. prosinca 2019. 20:53
Prima: Mario Kevo 
Kopija: geode mailto:dev@geode.apache.org>>
Predmet: Re: Odg: Odg: Lucene upgrade

Hi Mario,

I made a PR against your branch for some of the changes I had to do to get past 
the Index too new exception.  Summary - repo creation, even if no writes occur, 
appear to create some meta data that the old node attempts to read and blow up 
on.

The pr against your branch just prevents the repo from being constructed until 
all old members are upgraded.
This requires test changes to not try to validate using queries (since we 
prevent draining and repo creation, the query will just wait)

The reason why you probably were seeing unsuccessful dispatches, is because we 
kind of intended for that with the oldMember check.  In-between the server 
rolls, the test was trying to verify, but because not all servers had upgraded, 
the LuceneEventListener wasn't allowing the queue to drain on the new member.

I am not sure if the changes I added are acceptable or not -maybe if this ends 
up working then we can discuss on the dev list.

There will probably be other "gotcha's" along the way...


On Fri, Dec 6, 2019 at 1:12 AM Mario Kevo  wrote:
Hi Jason,

I tried to upgrade from 6.6.2 to 7.1.0 and got the following exception:

org.apache.lucene.index.IndexFormatTooNewException: Format version is not 
supported (resource BufferedChecksumIndexInput(segments_2)): 7 (needs to be 
between 4 and 6)

It looks like the fix is not good.

What I see (from