Re: [REQUEST] Squash merge please

[Alexander Murmann]

I wonder if Kirk's and Naba's statements are necessarily at odds.

Make the change easy (warning: this may be hard), then make the easy
> change."

 -Kent Beck

Following Kent Beck's advise might reasonably split into two commits. One
refactor commit and a separate commit that introduces the actual change.
They should be individually revertible and might be easier understood if
split out. I vividly remember a change on our code base where someone did a
huge amount of refactoring that resulted than in one parameter changing in
order to get the desired functionality change. If that was in one commit,
it would be hard to see the actual change. If split out, it's beautiful and
crystal clear.

I am unsure how that would be reflected in terms of JIRA ticket references.
Usually we assume that if there is a commit with the ticket number, the
issue is resolved. Maybe the key here is to create a separate refactoring
ticket.

Would that allow us to have our cake and eat it too?

On Fri, Dec 13, 2019 at 3:16 PM Nabarun Nag  wrote:

> It is to help with bisect operations when things start failing ... helps us
> it revert and build faster.
> also with cherry picking features / fixes to previous versions .
> And keeping the git history clean with no unnecessary “merge commits”.
>
> Regards
> Naba
>
>
> On Fri, Dec 13, 2019 at 2:25 PM Kirk Lund  wrote:
>
> > -1 I really like to sometimes have more than 1 commit in a PR and keep
> them
> > separate when they merge to develop
> >
> > On Tue, Oct 22, 2019 at 5:12 PM Nabarun Nag  wrote:
> >
> > > Hi Geode Committers,
> > >
> > > A kind request for using squash commit instead of using merge.
> > > This will really help us in our bisect operations when a
> > > regression/flakiness in the product is introduced. We can automate and
> go
> > > through fewer commits faster, avoiding commits like "spotless fix" and
> > > "re-trigger precheck-in" or other minor commits in the merged branch.
> > >
> > > Also, please use the commit format : (helps us to know who worked on
> it,
> > > what is the history)
> > >
> > >
> > >
> > > *GEODE-: 
> > > * explanation line 1* explanation line
> 2*
> > >
> > > This is not a rule or anything, but a request to help out your fellow
> > > committers in quickly detecting a problem.
> > >
> > > For inspiration, we can look into Apache Kafka / Spark where they have
> a
> > > complete linear graph for their main branch HEAD [see attachment]
> > >
> > > Regards
> > > Naba.
> > >
> > >
> > >
> >
>

Re: [REQUEST] Squash merge please

[Nabarun Nag]

It is to help with bisect operations when things start failing ... helps us
it revert and build faster.
also with cherry picking features / fixes to previous versions .
And keeping the git history clean with no unnecessary “merge commits”.

Regards
Naba


On Fri, Dec 13, 2019 at 2:25 PM Kirk Lund  wrote:

> -1 I really like to sometimes have more than 1 commit in a PR and keep them
> separate when they merge to develop
>
> On Tue, Oct 22, 2019 at 5:12 PM Nabarun Nag  wrote:
>
> > Hi Geode Committers,
> >
> > A kind request for using squash commit instead of using merge.
> > This will really help us in our bisect operations when a
> > regression/flakiness in the product is introduced. We can automate and go
> > through fewer commits faster, avoiding commits like "spotless fix" and
> > "re-trigger precheck-in" or other minor commits in the merged branch.
> >
> > Also, please use the commit format : (helps us to know who worked on it,
> > what is the history)
> >
> >
> >
> > *GEODE-: 
> > * explanation line 1* explanation line 2*
> >
> > This is not a rule or anything, but a request to help out your fellow
> > committers in quickly detecting a problem.
> >
> > For inspiration, we can look into Apache Kafka / Spark where they have a
> > complete linear graph for their main branch HEAD [see attachment]
> >
> > Regards
> > Naba.
> >
> >
> >
>

Re: [REQUEST] Squash merge please

[Kirk Lund]

-1 I really like to sometimes have more than 1 commit in a PR and keep them
separate when they merge to develop

On Tue, Oct 22, 2019 at 5:12 PM Nabarun Nag  wrote:

> Hi Geode Committers,
>
> A kind request for using squash commit instead of using merge.
> This will really help us in our bisect operations when a
> regression/flakiness in the product is introduced. We can automate and go
> through fewer commits faster, avoiding commits like "spotless fix" and
> "re-trigger precheck-in" or other minor commits in the merged branch.
>
> Also, please use the commit format : (helps us to know who worked on it,
> what is the history)
>
>
>
> *GEODE-: 
> * explanation line 1* explanation line 2*
>
> This is not a rule or anything, but a request to help out your fellow
> committers in quickly detecting a problem.
>
> For inspiration, we can look into Apache Kafka / Spark where they have a
> complete linear graph for their main branch HEAD [see attachment]
>
> Regards
> Naba.
>
>
>

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

[Jason Huynh]

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 
> *Poslano:* 11. prosinca 2019. 23:08
> *Prima:* Mario Kevo 
> *Kopija:* geode 
> *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 
> *Poslano:* 6. prosinca 2019. 20:53
> *Prima:* Mario Kevo 
> *Kopija:* geode 
> *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
> *RollingUpgradeQueryReturnsCorrectResultsAfterServersRollOverOnPartitionRegion*
> *.java*) is when it doing upgrade of a *locator* it will shutdown and
> started on the newer version. The problem is that *server2* become a lead
> and cannot read lucene index on the newer version(Lucene index format has
> ch

Re: [REQUEST] Squash merge please

[Owen Nichols]

+1

> On Oct 28, 2019, at 12:47 PM, Jacob Barrett  wrote:
> 
> +1
> 
> 
>> On Oct 22, 2019, at 5:12 PM, Nabarun Nag  wrote:
>> 
>> Hi Geode Committers,
>> 
>> A kind request for using squash commit instead of using merge. 
>> This will really help us in our bisect operations when a 
>> regression/flakiness in the product is introduced. We can automate and go 
>> through fewer commits faster, avoiding commits like "spotless fix" and 
>> "re-trigger precheck-in" or other minor commits in the merged branch. 
>> 
>> Also, please use the commit format : (helps us to know who worked on it, 
>> what is the history)
>>GEODE-: 
>> 
>>* explanation line 1
>>* explanation line 2
>> 
>> This is not a rule or anything, but a request to help out your fellow 
>> committers in quickly detecting a problem.
>> 
>> For inspiration, we can look into Apache Kafka / Spark where they have a 
>> complete linear graph for their main branch HEAD [see attachment]
>> 
>> Regards
>> Naba.
>> 
>> 
> 

Re: PR Checks issues?

[Robert Houghton]

We could have the content of the PR jobs check that actual sources changed,
before doing any real 'work' or 'validation' of the change. If the patch is
only CI or readme (not docs, those get compiled) then we can exit with a
clean state. Saves time, and money!

If the issue is the lack of Concourse seeing the PR, there are several
issues and potential resolutions that I am happy to go into offline.

On Fri, Dec 13, 2019 at 11:31 AM Michael Oleske  wrote:

> Hi Geode Dev!
>
> This PR https://github.com/apache/geode/pull/4406 is a change to the
> readme.  However it took 3 empty commits to get it to go green enough to be
> allowed to be merged.  This seems odd, especially with just a readme
> change.  Is there something going with how CI works for PRs?  This was an
> incredibly frustrating experience.
>
> Thanks!
> -michael
>

Re: PR Checks issues?

[Owen Nichols]

Hi Michael, that does sound like a frustrating experience.  Thanks for saying 
something — it is harder to track how often this is occurring if people don’t 
report the issue on the dev list.

Geode’s PR pipeline uses https://hub.docker.com/r/teliaoss/github-pr-resource 
 to detect changes to PRs 
against Geode and trigger PR checks.  We don’t know why, but sometimes it 
misses things.  Some options are:
1) just deal with it
2) find a different plugin that is more reliable, if such a thing exists?
3) dive into the inner workings of the Telia project and see if we can fix 
their bug for them?

If you see your PR checks stuck in “expecting” state and they don’t proceed to 
“pending” within a few minutes, you can push another commit (e.g. an empty 
commit) to try again.  It’s unusual to have to do this more than once, but 
statistically about 1% of the time it could be necessary (based on a wild guess 
that Telia might be missing up to 10% of all PR commits).

Missing PR checks can also be a symptom of a merge conflict, although this is 
less common.  You may notice that the LGTM checks and/or GitHub itself 
sometimes report that there is a conflict, but Telia does its own merge that 
sometimes sees conflicts even when GitHub believes they could be auto-resolved. 
 Pushing an empty commit will force Telia to try again to merge the latest 
develop, or better yet, it never hurts to merge the latest develop yourself.

Does someone want to volunteer to look into 2) or 3)?  Is there another 
solution (I assume you weren’t suggesting getting rid of required checks 
entirely)?

-Owen

> On Dec 13, 2019, at 11:30 AM, Michael Oleske  wrote:
> 
> Hi Geode Dev!
> 
> This PR https://github.com/apache/geode/pull/4406 is a change to the
> readme.  However it took 3 empty commits to get it to go green enough to be
> allowed to be merged.  This seems odd, especially with just a readme
> change.  Is there something going with how CI works for PRs?  This was an
> incredibly frustrating experience.
> 
> Thanks!
> -michael

Re: PR Checks issues?

[Michael Oleske]

Well lack of review was one part.  The major part was that if you click on
the checks from the other empty commits, you'll see that the required
passing jobs never triggered (only the LGTM checks).

-michael

On Fri, Dec 13, 2019 at 11:43 AM Joey McAllister 
wrote:

> Hi Michael,
>
> That may have been connected, at least in part, to a lack of a review. I
> have given my review on adding this info to the README, which I think looks
> good, and things seem to have gone green.
>
> That said, as I mentioned in my review, I also wouldn't mind seeing a
> thumbs up (or other feedback) from someone else in the community who will
> be using these LGTM alerts regularly.
>
> Best,
> Joey
>
> On Fri, Dec 13, 2019 at 11:31 AM Michael Oleske 
> wrote:
>
> > Hi Geode Dev!
> >
> > This PR https://github.com/apache/geode/pull/4406 is a change to the
> > readme.  However it took 3 empty commits to get it to go green enough to
> be
> > allowed to be merged.  This seems odd, especially with just a readme
> > change.  Is there something going with how CI works for PRs?  This was an
> > incredibly frustrating experience.
> >
> > Thanks!
> > -michael
> >
>

Re: PR Checks issues?

[Joey McAllister]

Hi Michael,

That may have been connected, at least in part, to a lack of a review. I
have given my review on adding this info to the README, which I think looks
good, and things seem to have gone green.

That said, as I mentioned in my review, I also wouldn't mind seeing a
thumbs up (or other feedback) from someone else in the community who will
be using these LGTM alerts regularly.

Best,
Joey

On Fri, Dec 13, 2019 at 11:31 AM Michael Oleske  wrote:

> Hi Geode Dev!
>
> This PR https://github.com/apache/geode/pull/4406 is a change to the
> readme.  However it took 3 empty commits to get it to go green enough to be
> allowed to be merged.  This seems odd, especially with just a readme
> change.  Is there something going with how CI works for PRs?  This was an
> incredibly frustrating experience.
>
> Thanks!
> -michael
>

PR Checks issues?

[Michael Oleske]

Hi Geode Dev!

This PR https://github.com/apache/geode/pull/4406 is a change to the
readme.  However it took 3 empty commits to get it to go green enough to be
allowed to be merged.  This seems odd, especially with just a readme
change.  Is there something going with how CI works for PRs?  This was an
incredibly frustrating experience.

Thanks!
-michael

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

[Darrel Schneider]

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

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

[Dan Smith]

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

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

[Mark Hanson]

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/apache/geode/pull/4409 
  specifically Cache.java, 
 RegionFactory.java, and for extra credit GemfireCacheImpl.java
 
 I have commented at the relevant changes.
 
 Thanks,
 Mark

Re: Issues with TransactionIds managed by CacheTransactionManager in C++ native client

[Blake Bender]

Transactions are an area of the codebase I've never dealt with, so I'm sure
most/all of what you mention is true.  In particular, the only thing I know
about TransactionId is that it's always set to -1, so functionally it's
essentially useless.  I'm not certain what all of the implications will be
if suddenly we ascribe meaning to it, so I'll let some folks with more
native client history than I chime in.

Thanks,

Blake


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

> Hi,
>
> I have created a ticket with some issues I have found related to
> TransactionIds managed by CacheTransactionManager in the C++ native client.
>
> https://issues.apache.org/jira/browse/GEODE-7573
>
> In it, I also propose some solutions to the issues found.
>
> I'd appreciate if someone could review the analysis to see if I am in the
> right track or if I am missing something.
>
> Thanks in advance,
>
> Alberto
>

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

[Alberto Gomez]

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

Issues with TransactionIds managed by CacheTransactionManager in C++ native client

[Alberto Gomez]

Hi,

I have created a ticket with some issues I have found related to TransactionIds 
managed by CacheTransactionManager in the C++ native client.

https://issues.apache.org/jira/browse/GEODE-7573

In it, I also propose some solutions to the issues found.

I'd appreciate if someone could review the analysis to see if I am in the right 
track or if I am missing something.

Thanks in advance,

Alberto