Re: Review Request 50715: GEODE-1128: Fixes for colocation configuration problems

[Darrel Schneider]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50715/#review146054
---




geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java
 (line 37)


make sure and remove the commented out RegionListener



geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java
 (line 116)


Collections.emptyList() is better



geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java
 (line 118)


couldn't you change lines 113..122 to be:
missingChildren.removeAll(coloHierarchy.keySet());



geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java
 (line 129)


I don't think you want to log an error because the thread was interrupted.
In this thread's case I think you should just return if interrupted.
I think the only thing you need to do in this particular catch is:
  Thread.currentThread().interrupt();
that will reset the interrupt bit that was cleared when you caught the 
exception. You really don't even need that but it is always a good idea in case 
something else after the return ends up blocking and should have also been 
interrupted.



geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java
 (line 151)


I'd like to see you reuse "addMissingChildRegion" here instead of 
duplicating that code.
So this method could be:
for (String name: childRegion.getMissingColocatedChildren()) {
  addMissingChildRegion(name);
}



geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java
 (line 7490)


I think you need to deal with missingColoctedRegionLogger being null.
And it can become null async if stop is called.
So this method should read it into a local var;
test the local var; if not null call getMissingChildRegions; otherwise 
return Collections.emptyList().


- Darrel Schneider


On Aug. 17, 2016, 4:12 p.m., Ken Howe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50715/
> ---
> 
> (Updated Aug. 17, 2016, 4:12 p.m.)
> 
> 
> Review request for geode, Darrel Schneider, Eric Shu, Scott Jewell, Kirk 
> Lund, and Dan Smith.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Test for missing parent regions on child region creates and throw 
> IllegalStateException
> 
> Log warnings about missing colocated child regions.
> 
> Create Unit and DUnit tests for new exceptions and warnings
> 
> There are two cases of missing colocated regions,
> 1) The 'parent' region hasn't been created when a region specifies it in the 
> 'colocated-with' attribute
> 2) A persistent child region does not exist.
> 
> For (1), this condition can be determined in 
> ColocationHelper.getColocatedRegion(). The core product currently does not 
> test for this which results in an NPE being thrown without any logging to 
> indicate the reason. There are two variations of this state.
> 
> 1a) When starting a region with non-null 'colocated'with', a reference to the 
> parent region configuration is obtained through the configuration root. When 
> the reference obtained is null (the region doesn't exist in the root 
> configuration) the NPE ends up getting thrown. The fix for this is to 
> immediately throw an IllegalStateException with a message to note the missing 
> colocated-with region.
> 
> 1b) The parent region configuration may exist in the root configuration (the 
> parent PR has been created on another member) but does not exist on the local 
> member. In this case the null comes about when obtaining the local region 
> (PartitionedRegion.getPRFromId()). The fix here is the same as (1a) - throw 
> an IllegalStateException.
> 
> For (2), missing child regions: This state will always exist for an 
> indeterminate period because the parent is always created before the child 
> region. There currently isn't any indication in the logs of this condition, 
> even it if persists indefinitely, other than a failure to recover the PR's 
> persistent data. The fix for this is starting a logging thread (similar to 
> the RedundancyLogger) when a child region is found missing. The condition 
> will be periodically logged (set initially to 30 secs) until the region is 
> created. There is a delay before the first log message to allow time for the 
> normal sequencing of region creations.
> 
> 
> 

Re: git commit messages

[Dan Smith]

Yeah, 50 chars seems a bit short. I think I've been aiming for 72
personally.

-Dan

On Wed, Aug 17, 2016 at 4:37 PM, Kirk Lund  wrote:

> Some examples from feature/GEODE-1781-02 which are my latest unmerged
> commits...
>
> 1) 1st line is 62 chars. To shorten to 50: "GEODE-1799: fix javadocs on
> CacheServerStats" or "GEODE-1799: change javadocs from Bridge to Cache"
>
> commit 0a07db189c8b928976ed6554499f15b6a64e1633
> Author: Kirk Lund 
> Date:   Wed Aug 17 16:27:52 2016 -0700
>
> GEODE-1799: change javadocs from Bridge Server to Cache Server
>
> 2) 1st line is 80 chars. To shorten to 50: "GEODE-1781: exclude class from
> AnaylzeSerializableJUnitTest"
>
> commit 55c5df5e4cd6228be1617fb1e92d8d955a703b08
> Author: Kirk Lund 
> Date:   Tue Aug 16 09:25:33 2016 -0700
>
> GEODE-1781: exclude LinuxProcFsStatistics$CPU from
> AnaylzeSerializableJUnitTest
>
> 3) 1st line is 59 chars. To shorten to 50: "GEODE-1782: fix stat resource
> equals"
> (the later lines meet our guidelines)
>
> commit 7dc1ce68a483f993adeb613893073d8a7c88a9b7
> Author: Kirk Lund 
> Date:   Mon Aug 15 18:35:45 2016 -0700
>
> GEODE-1782: include start timestamp in stat resource equals
>
> * stat resources with different time stamps should not be equal
>
> * StatArchiveWithConsecutiveResourceInstGenerator generates gfs with
> multiple stat resources of same name but different times
>
> * StatArchiveWithConsecutiveResourceInstIntegrationTest confirms
> existence of bug GEODE-1782: StatArchiveReader ignores later stats
> resource with same name as closed stats resource
>
> * ResourceInstTest verifies the underlying issue and fix in
> StatArchiveReader.ResourceInst.equals and the fix
>
> 4) I got this one down to 48 chars!
> (the later lines meet our guidelines)
>
> commit 115070123ec15638ca1189a7349938c35e0d51e0
> Author: Kirk Lund 
> Date:   Mon Aug 15 18:26:16 2016 -0700
>
> GEODE-1781: refactor internal statistics classes
>
> * move internal statistics classes into com.gemstone.gemfire.internal.
> statistics
>
> * move statistics tests into com.gemstone.gemfire.internal.statistics
>
> * modify tests to include integration and distributed in names
>
> * modify tests to use TemporaryFolder and TestName rules
>
> On Wed, Aug 17, 2016 at 4:22 PM, Kenneth Howe  wrote:
>
> > Agree with Kirk, 50 chars is really short by the time you use up the
> first
> > 12 characters for the Jira tag. If we’re going to have a guideline, I’d
> > rather be longer - somewhat arbitrarily I’d probably make it 20-30 chars
> > more. It’s been a long time since text listings were intended to fit on a
> > 80x24 dumb terminal, so I don’t see a need to restrict the commit message
> > headers so severely.
> >
> > I do use the —online option embedded in a local alias I use to look at a
> > history list of my local repo.
> >
> > Ken
> >
> > > On Aug 17, 2016, at 3:45 PM, Kevin Duling  wrote:
> > >
> > > The format is very similar to the one most other git shops I've worked
> in
> > > before use.  I don't believe we ever had formal length limits.
> > Typically,
> > > it was:
> > >
> > > -: 
> > >>
> > > blank line
> > >
> > >  > ticket>
> > >
> > >
> > > The Atlassian plugin for IDEA automates a lot of this.  There are
> limits
> > on
> > > the length of a jira ticket summary, but I'm not sure what that is.  I
> > ran
> > > in to it when I did my round of CI.
> > >
> > > I don't see a reason to change anything except maybe stress that he
> > lengths
> > > are a guideline, not a hard & fast rule.  If more room is needed to
> write
> > > good information, it shouldn't be truncated as it's not unknown to move
> > > away from a given ticket system.
> > >
> > > On Wed, Aug 17, 2016 at 3:38 PM, Kirk Lund  wrote:
> > >
> > >> 50 chars including "GEODE-: " is awfully short.
> > >> http://chris.beams.io/posts/git-commit/ does say that's just a
> general
> > >> rule
> > >> of thumb and not a hard limit. The author's reasoning seems to be
> > >> specifically for using "git log --oneline" -- does anyone use that
> > option
> > >> with git log? I don't.
> > >>
> > >> I guess another option is to not have to have a guideline if we don't
> > want
> > >> one... our current git log messages are reasonable and make sense.
> > >>
> > >> -Kirk
> > >>
> > >>
> > >> On Wed, Aug 17, 2016 at 3:21 PM, Kirk Lund  wrote:
> > >>
> > >>> Here's the git commit message guidelines we discussed and voted on
> last
> > >>> year. I just checked and my own git commit message line lengths have
> > >> grown
> > >>> beyond what we decided to use. Most other are also not following this
> > >>> guideline.
> > >>>
> > >>> Here's the list of folks who voted last year along with their vote:
> > >>>
> > >>> Anthony Baker +1
> > >>> Vincent Ford +1
> > >>> William Markito 

Re: git commit messages

[Kenneth Howe]

Agree with Kirk, 50 chars is really short by the time you use up the first 12 
characters for the Jira tag. If we’re going to have a guideline, I’d rather be 
longer - somewhat arbitrarily I’d probably make it 20-30 chars more. It’s been 
a long time since text listings were intended to fit on a 80x24 dumb terminal, 
so I don’t see a need to restrict the commit message headers so severely.

I do use the —online option embedded in a local alias I use to look at a 
history list of my local repo. 

Ken

> On Aug 17, 2016, at 3:45 PM, Kevin Duling  wrote:
> 
> The format is very similar to the one most other git shops I've worked in
> before use.  I don't believe we ever had formal length limits.  Typically,
> it was:
> 
> -: 
>> 
> blank line
> 
> 
> 
> 
> The Atlassian plugin for IDEA automates a lot of this.  There are limits on
> the length of a jira ticket summary, but I'm not sure what that is.  I ran
> in to it when I did my round of CI.
> 
> I don't see a reason to change anything except maybe stress that he lengths
> are a guideline, not a hard & fast rule.  If more room is needed to write
> good information, it shouldn't be truncated as it's not unknown to move
> away from a given ticket system.
> 
> On Wed, Aug 17, 2016 at 3:38 PM, Kirk Lund  wrote:
> 
>> 50 chars including "GEODE-: " is awfully short.
>> http://chris.beams.io/posts/git-commit/ does say that's just a general
>> rule
>> of thumb and not a hard limit. The author's reasoning seems to be
>> specifically for using "git log --oneline" -- does anyone use that option
>> with git log? I don't.
>> 
>> I guess another option is to not have to have a guideline if we don't want
>> one... our current git log messages are reasonable and make sense.
>> 
>> -Kirk
>> 
>> 
>> On Wed, Aug 17, 2016 at 3:21 PM, Kirk Lund  wrote:
>> 
>>> Here's the git commit message guidelines we discussed and voted on last
>>> year. I just checked and my own git commit message line lengths have
>> grown
>>> beyond what we decided to use. Most other are also not following this
>>> guideline.
>>> 
>>> Here's the list of folks who voted last year along with their vote:
>>> 
>>> Anthony Baker +1
>>> Vincent Ford +1
>>> William Markito +1
>>> arghya sadhu +1
>>> 
>>> Do we want to reaffirm this guideline or should it change?
>>> 
>>> -Kirk
>>> 
>>> -- Forwarded message --
>>> From: Kirk Lund 
>>> Date: Wed, Aug 5, 2015 at 3:18 PM
>>> Subject: git commit messages
>>> To: dev@geode.incubator.apache.org
>>> 
>>> 
>>> Several of us were discussing http://chris.beams.io/posts/git-commit/ --
>>> there are a couple other really good articles about git commit messages
>> and
>>> below is the message style I've been trying to follow.
>>> 
>>> http://chris.beams.io/posts/git-commit/
>>> http://www.laurencegellert.com/2013/07/how-to-write-a-proper-commit-
>>> message/
>>> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>>> 
>>> GEODE-nn: Begin capitalized and 50 chars or less
>>> 
>>> More detailed explanation with linefeeds to wrap at 72 characters after
>>> a blank line following the summary.
>>> 
>>> Further paragraphs come after blank lines.
>>> 
>>> - Bullet points are okay, too
>>> 
>>> - Typically a hyphen or asterisk is used for the bullet, followed by a
>>>  single space, with blank lines in between, but conventions vary here
>>> 
>>> - Use a hanging indent
>>> 
>>> 
>>> 
>>> 
>> 

Re: git commit messages

[Dan Smith]

To me, having a meaningful short summary line is still pretty useful. I use
git oneline and github and my ide also use that summary line.

-Dan



On Wed, Aug 17, 2016 at 3:45 PM, Kevin Duling  wrote:

> The format is very similar to the one most other git shops I've worked in
> before use.  I don't believe we ever had formal length limits.  Typically,
> it was:
>
> -: 
> >
> blank line
>
> 
>
>
> The Atlassian plugin for IDEA automates a lot of this.  There are limits on
> the length of a jira ticket summary, but I'm not sure what that is.  I ran
> in to it when I did my round of CI.
>
> I don't see a reason to change anything except maybe stress that he lengths
> are a guideline, not a hard & fast rule.  If more room is needed to write
> good information, it shouldn't be truncated as it's not unknown to move
> away from a given ticket system.
>
> On Wed, Aug 17, 2016 at 3:38 PM, Kirk Lund  wrote:
>
> > 50 chars including "GEODE-: " is awfully short.
> > http://chris.beams.io/posts/git-commit/ does say that's just a general
> > rule
> > of thumb and not a hard limit. The author's reasoning seems to be
> > specifically for using "git log --oneline" -- does anyone use that option
> > with git log? I don't.
> >
> > I guess another option is to not have to have a guideline if we don't
> want
> > one... our current git log messages are reasonable and make sense.
> >
> > -Kirk
> >
> >
> > On Wed, Aug 17, 2016 at 3:21 PM, Kirk Lund  wrote:
> >
> > > Here's the git commit message guidelines we discussed and voted on last
> > > year. I just checked and my own git commit message line lengths have
> > grown
> > > beyond what we decided to use. Most other are also not following this
> > > guideline.
> > >
> > > Here's the list of folks who voted last year along with their vote:
> > >
> > > Anthony Baker +1
> > > Vincent Ford +1
> > > William Markito +1
> > > arghya sadhu +1
> > >
> > > Do we want to reaffirm this guideline or should it change?
> > >
> > > -Kirk
> > >
> > > -- Forwarded message --
> > > From: Kirk Lund 
> > > Date: Wed, Aug 5, 2015 at 3:18 PM
> > > Subject: git commit messages
> > > To: dev@geode.incubator.apache.org
> > >
> > >
> > > Several of us were discussing http://chris.beams.io/posts/git-commit/
> --
> > > there are a couple other really good articles about git commit messages
> > and
> > > below is the message style I've been trying to follow.
> > >
> > > http://chris.beams.io/posts/git-commit/
> > > http://www.laurencegellert.com/2013/07/how-to-write-a-proper-commit-
> > > message/
> > > http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
> > >
> > > GEODE-nn: Begin capitalized and 50 chars or less
> > >
> > > More detailed explanation with linefeeds to wrap at 72 characters after
> > > a blank line following the summary.
> > >
> > > Further paragraphs come after blank lines.
> > >
> > > - Bullet points are okay, too
> > >
> > > - Typically a hyphen or asterisk is used for the bullet, followed by a
> > >   single space, with blank lines in between, but conventions vary here
> > >
> > > - Use a hanging indent
> > >
> > >
> > >
> > >
> >
>

Re: Review Request 50715: GEODE-1128: Fixes for colocation configuration problems

[Ken Howe]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50715/
---

(Updated Aug. 17, 2016, 11:12 p.m.)


Review request for geode, Darrel Schneider, Eric Shu, Scott Jewell, Kirk Lund, 
and Dan Smith.


Changes
---

Refactored the managment of the missingChildren list in the logger object and 
reformatted the warning messages on missing children. Changes to handling 
missingChildren removes the need for the logger to be a RegionListener. 

Each region that has missing child regions anywhere in the colocation hierarchy 
that it is the root of has an instance of the ColocationLogger that runs as a 
separate thread.  ColocationHelper.hasOfflineColocatedChildRegions determines 
if there are missing colocated regions (no change here). When it finds any new 
calls are made to add them to the missingChildren list for the region's logger 
instance. The logger will write warning messages peridocally as long as there 
are missing colocated regions. Each region in the missingChildren list is 
verified as still missing prior to including it in the warning message. If a 
region is no longer missing, then it's removed from the list. When persistent 
data recovery is complete for a region or the missingChildren list is empty, 
then the logger thread terminates.

A missing region will be included in the warning for each region above it in a 
colcation hierarchy. Message format shown in the following example taking from 
a DUnit test.

[vm_0][warn 2016/08/17 15:30:19.663 PDT  tid=88] 
Persistent data recovery for region /Parent is prevented by offline colocated 
regions
[vm_0]  /Gen1_C2
[vm_0]  /Gen2_C1_1
[vm_0]  /Gen2_C1_2

[vm_0][warn 2016/08/17 15:30:19.676 PDT  tid=91] 
Persistent data recovery for region /Gen1_C1 is prevented by offline colocated 
regions
[vm_0]  /Gen2_C1_1
[vm_0]  /Gen2_C1_2


Repository: geode


Description
---

Test for missing parent regions on child region creates and throw 
IllegalStateException

Log warnings about missing colocated child regions.

Create Unit and DUnit tests for new exceptions and warnings

There are two cases of missing colocated regions,
1) The 'parent' region hasn't been created when a region specifies it in the 
'colocated-with' attribute
2) A persistent child region does not exist.

For (1), this condition can be determined in 
ColocationHelper.getColocatedRegion(). The core product currently does not test 
for this which results in an NPE being thrown without any logging to indicate 
the reason. There are two variations of this state.

1a) When starting a region with non-null 'colocated'with', a reference to the 
parent region configuration is obtained through the configuration root. When 
the reference obtained is null (the region doesn't exist in the root 
configuration) the NPE ends up getting thrown. The fix for this is to 
immediately throw an IllegalStateException with a message to note the missing 
colocated-with region.

1b) The parent region configuration may exist in the root configuration (the 
parent PR has been created on another member) but does not exist on the local 
member. In this case the null comes about when obtaining the local region 
(PartitionedRegion.getPRFromId()). The fix here is the same as (1a) - throw an 
IllegalStateException.

For (2), missing child regions: This state will always exist for an 
indeterminate period because the parent is always created before the child 
region. There currently isn't any indication in the logs of this condition, 
even it if persists indefinitely, other than a failure to recover the PR's 
persistent data. The fix for this is starting a logging thread (similar to the 
RedundancyLogger) when a child region is found missing. The condition will be 
periodically logged (set initially to 30 secs) until the region is created. 
There is a delay before the first log message to allow time for the normal 
sequencing of region creations.


Diffs (updated)
-

  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationHelper.java
 012a77f 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ColocationLogger.java
 PRE-CREATION 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionRegionConfig.java
 6d7c1ca 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java
 9ac95a1 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/RedundancyLogger.java
 f7e8621 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/i18n/LocalizedStrings.java
 443fe78 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ColocationHelperTest.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java
 d8b3514 
  

Re: git commit messages

[Kevin Duling]

The format is very similar to the one most other git shops I've worked in
before use.  I don't believe we ever had formal length limits.  Typically,
it was:

-: 
>
blank line




The Atlassian plugin for IDEA automates a lot of this.  There are limits on
the length of a jira ticket summary, but I'm not sure what that is.  I ran
in to it when I did my round of CI.

I don't see a reason to change anything except maybe stress that he lengths
are a guideline, not a hard & fast rule.  If more room is needed to write
good information, it shouldn't be truncated as it's not unknown to move
away from a given ticket system.

On Wed, Aug 17, 2016 at 3:38 PM, Kirk Lund  wrote:

> 50 chars including "GEODE-: " is awfully short.
> http://chris.beams.io/posts/git-commit/ does say that's just a general
> rule
> of thumb and not a hard limit. The author's reasoning seems to be
> specifically for using "git log --oneline" -- does anyone use that option
> with git log? I don't.
>
> I guess another option is to not have to have a guideline if we don't want
> one... our current git log messages are reasonable and make sense.
>
> -Kirk
>
>
> On Wed, Aug 17, 2016 at 3:21 PM, Kirk Lund  wrote:
>
> > Here's the git commit message guidelines we discussed and voted on last
> > year. I just checked and my own git commit message line lengths have
> grown
> > beyond what we decided to use. Most other are also not following this
> > guideline.
> >
> > Here's the list of folks who voted last year along with their vote:
> >
> > Anthony Baker +1
> > Vincent Ford +1
> > William Markito +1
> > arghya sadhu +1
> >
> > Do we want to reaffirm this guideline or should it change?
> >
> > -Kirk
> >
> > -- Forwarded message --
> > From: Kirk Lund 
> > Date: Wed, Aug 5, 2015 at 3:18 PM
> > Subject: git commit messages
> > To: dev@geode.incubator.apache.org
> >
> >
> > Several of us were discussing http://chris.beams.io/posts/git-commit/ --
> > there are a couple other really good articles about git commit messages
> and
> > below is the message style I've been trying to follow.
> >
> > http://chris.beams.io/posts/git-commit/
> > http://www.laurencegellert.com/2013/07/how-to-write-a-proper-commit-
> > message/
> > http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
> >
> > GEODE-nn: Begin capitalized and 50 chars or less
> >
> > More detailed explanation with linefeeds to wrap at 72 characters after
> > a blank line following the summary.
> >
> > Further paragraphs come after blank lines.
> >
> > - Bullet points are okay, too
> >
> > - Typically a hyphen or asterisk is used for the bullet, followed by a
> >   single space, with blank lines in between, but conventions vary here
> >
> > - Use a hanging indent
> >
> >
> >
> >
>

Re: git commit messages

[Kirk Lund]

50 chars including "GEODE-: " is awfully short.
http://chris.beams.io/posts/git-commit/ does say that's just a general rule
of thumb and not a hard limit. The author's reasoning seems to be
specifically for using "git log --oneline" -- does anyone use that option
with git log? I don't.

I guess another option is to not have to have a guideline if we don't want
one... our current git log messages are reasonable and make sense.

-Kirk


On Wed, Aug 17, 2016 at 3:21 PM, Kirk Lund  wrote:

> Here's the git commit message guidelines we discussed and voted on last
> year. I just checked and my own git commit message line lengths have grown
> beyond what we decided to use. Most other are also not following this
> guideline.
>
> Here's the list of folks who voted last year along with their vote:
>
> Anthony Baker +1
> Vincent Ford +1
> William Markito +1
> arghya sadhu +1
>
> Do we want to reaffirm this guideline or should it change?
>
> -Kirk
>
> -- Forwarded message --
> From: Kirk Lund 
> Date: Wed, Aug 5, 2015 at 3:18 PM
> Subject: git commit messages
> To: dev@geode.incubator.apache.org
>
>
> Several of us were discussing http://chris.beams.io/posts/git-commit/ --
> there are a couple other really good articles about git commit messages and
> below is the message style I've been trying to follow.
>
> http://chris.beams.io/posts/git-commit/
> http://www.laurencegellert.com/2013/07/how-to-write-a-proper-commit-
> message/
> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>
> GEODE-nn: Begin capitalized and 50 chars or less
>
> More detailed explanation with linefeeds to wrap at 72 characters after
> a blank line following the summary.
>
> Further paragraphs come after blank lines.
>
> - Bullet points are okay, too
>
> - Typically a hyphen or asterisk is used for the bullet, followed by a
>   single space, with blank lines in between, but conventions vary here
>
> - Use a hanging indent
>
>
>
>

Fwd: git commit messages

[Kirk Lund]

Here's the git commit message guidelines we discussed and voted on last
year. I just checked and my own git commit message line lengths have grown
beyond what we decided to use. Most other are also not following this
guideline.

Here's the list of folks who voted last year along with their vote:

Anthony Baker +1
Vincent Ford +1
William Markito +1
arghya sadhu +1

Do we want to reaffirm this guideline or should it change?

-Kirk

-- Forwarded message --
From: Kirk Lund 
Date: Wed, Aug 5, 2015 at 3:18 PM
Subject: git commit messages
To: dev@geode.incubator.apache.org


Several of us were discussing http://chris.beams.io/posts/git-commit/ --
there are a couple other really good articles about git commit messages and
below is the message style I've been trying to follow.

http://chris.beams.io/posts/git-commit/
http://www.laurencegellert.com/2013/07/how-to-write-a-proper-commit-message/
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

GEODE-nn: Begin capitalized and 50 chars or less

More detailed explanation with linefeeds to wrap at 72 characters after
a blank line following the summary.

Further paragraphs come after blank lines.

- Bullet points are okay, too

- Typically a hyphen or asterisk is used for the bullet, followed by a
  single space, with blank lines in between, but conventions vary here

- Use a hanging indent

Re: Geode Clubhouse TOMORROW Thursday, Aug 18, 9AM Pacific: What's New with Spring Data Geode

[Gregory Chase]

Dear Geode Community,
Just reminding you about tomorrow's Geode Clubhouse at 9AM Pacific:

Join here  | Add
reminder to Calendar


-Greg

On Fri, Aug 12, 2016 at 10:12 AM, Gregory Chase  wrote:

> Dear Geode Community,
> John Blum will be our guest speaker at the next Geode Clubhouse on
> Thursday August 18, at 9AM Pacific. He'll give us an update on his latest
> work with the Spring Data Geode API.
>
> Join here  | Add
> reminder to Calendar
> 
>
> In John's own words:
>
> > Recently, I have made significant progress on new developments in*Spring
> > Data GemFire* as well as *Spring Data Geode's* new Java annotation-based
> > configuration meta-data approach.  My promise is this, this will greatly
> > simplify the user's getting started experience when developing *Spring*
> > Java applications using *Apache Geode*.
>
> See you next Thursday!
>
>
> --
> Greg Chase
>
> Global Head, Big Data Communities
> http://www.pivotal.io/big-data
>
> Pivotal Software
> http://www.pivotal.io/
>
> 650-215-0477
> @GregChase
> Blog: http://geekmarketing.biz/
>
>


-- 
Greg Chase

Global Head, Big Data Communities
http://www.pivotal.io/big-data

Pivotal Software
http://www.pivotal.io/

650-215-0477
@GregChase
Blog: http://geekmarketing.biz/

Re: Review Request 51188: GEODE-1776: Always read all of the results in ExecuteRegionFunctionOp

[Barry Oglesby]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51188/#review146033
---


Ship it!




Ship It!

- Barry Oglesby


On Aug. 17, 2016, 5:41 p.m., Dan Smith wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51188/
> ---
> 
> (Updated Aug. 17, 2016, 5:41 p.m.)
> 
> 
> Review request for geode and Barry Oglesby.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Read all of the results from the wire in
> ExecuteRegionFunctionOp.processResponse, even if we read an exception as
> one of the responses. This ensures that all data is read off the wire.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/ExecuteFunctionOp.java
>  278f1f79afa98823df27974da2907685c7e5e641 
>   
> geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/ExecuteRegionFunctionOp.java
>  d1540204546604ff7ed3d3d490ac40d079ac2953 
>   
> geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/ExecuteRegionFunctionSingleHopOp.java
>  ce7c3f84092f59051e5a148a42cc6f06adc787fe 
> 
> Diff: https://reviews.apache.org/r/51188/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Smith
> 
>

Re: Flaky tests failing with BindException

[Bruce Schuchardt]

The membership-port-range changes have been checked in for a while now.

Le 8/17/2016 à 10:51 AM, Kirk Lund a écrit :

Have we made all of the changes that we think will help prevent
BindException failures?

Last night's nightly build failed with one again:

:geode-core:flakyTest

com.gemstone.gemfire.security.ClientAuthenticationDUnitTest >
testCredentialsForNotifications FAILED
 com.gemstone.gemfire.test.dunit.RMIException: While invoking
com.gemstone.gemfire.security.ClientAuthenticationTestCase$$Lambda$26/1964608307.call
in VM 0 running on Host asf902.gq1.ygridcore.net with 4 VMs
 at com.gemstone.gemfire.test.dunit.VM.invoke(VM.java:389)
 at com.gemstone.gemfire.test.dunit.VM.invoke(VM.java:355)
 at com.gemstone.gemfire.test.dunit.VM.invoke(VM.java:320)
 at
com.gemstone.gemfire.security.ClientAuthenticationTestCase.doTestCredentialsForNotifications(ClientAuthenticationTestCase.java:456)
 at
com.gemstone.gemfire.security.ClientAuthenticationDUnitTest.testCredentialsForNotifications(ClientAuthenticationDUnitTest.java:82)

 Caused by:
 java.lang.AssertionError: Got unexpected exception when starting
server

 Caused by:
 java.net.BindException: Failed to create server socket on
  null[60,026]

 Caused by:
 java.net.BindException: Address already in use

193 tests completed, 1 failed, 6 skipped

On Thu, Aug 4, 2016 at 10:38 AM, Bruce Schuchardt 
wrote:


I've pushed the port-range changes that I described in my last email on
this subject.


Le 8/1/2016 à 5:33 PM, Kirk Lund a écrit :


I think that the changes mentioned by Jens and Bruce obviate the need to
do
what I was proposing.

-Kirk


On Fri, Jul 29, 2016 at 3:41 PM, Bruce Schuchardt 
wrote:

Why not create a JUnit rule that returns available ports and keep track
of


ports being used ?

I've cloned this gist from somewhere (don't remember now) but I've
planning
to send it for discussion...

https://gist.github.com/markito/b5be3fc570c7c7c84e6d09e064a6e898

Still talking about rules, I've played a bit with the TemporaryFolder
rule
and that's very useful as well, specially to clean up after test runs
and
to avoid conflicts.

http://junit.org/junit4/javadoc/4.12/org/junit/rules/Tempora
ryFolder.html

Just my 2c

On Fri, Jul 29, 2016 at 1:54 PM, Hitesh Khamesra <
hitesh...@yahoo.com.invalid> wrote:

Is there any possibility of running multiple test same time on that


machine?

-Hitesh


 From: Kirk Lund 
To: geode 
Sent: Friday, July 29, 2016 1:21 PM
Subject: Flaky tests failing with BindException

Many of our flaky tests are flaky because they use AvailablePort or
AvailablePortHelper to find randomly available ports. They then later

fail

with a BindException because the port is already in use by the time the

test uses it.

Here's a proposal for a temporary fix:

The module geode-junit contains a JUnit 4 rule called RetryRule. We
could
modify RetryRule to only retry if a BindException (or configurable
exception/s) is detected. This rule would then be dropped into every
test
that uses AvailablePort or AvailablePortHelper. Then if the test fails

with

a BindException, it would automatically retry (once or twice or

whatever

we

decide to configure RetryRule with). If the test fails without any

detected

BindException, then it would just fail without retrying.

Opinions on this?

Thanks,
Kirk





--

~/William

Re: Flaky tests failing with BindException

[Kirk Lund]

Have we made all of the changes that we think will help prevent
BindException failures?

Last night's nightly build failed with one again:

:geode-core:flakyTest

com.gemstone.gemfire.security.ClientAuthenticationDUnitTest >
testCredentialsForNotifications FAILED
com.gemstone.gemfire.test.dunit.RMIException: While invoking
com.gemstone.gemfire.security.ClientAuthenticationTestCase$$Lambda$26/1964608307.call
in VM 0 running on Host asf902.gq1.ygridcore.net with 4 VMs
at com.gemstone.gemfire.test.dunit.VM.invoke(VM.java:389)
at com.gemstone.gemfire.test.dunit.VM.invoke(VM.java:355)
at com.gemstone.gemfire.test.dunit.VM.invoke(VM.java:320)
at
com.gemstone.gemfire.security.ClientAuthenticationTestCase.doTestCredentialsForNotifications(ClientAuthenticationTestCase.java:456)
at
com.gemstone.gemfire.security.ClientAuthenticationDUnitTest.testCredentialsForNotifications(ClientAuthenticationDUnitTest.java:82)

Caused by:
java.lang.AssertionError: Got unexpected exception when starting
server

Caused by:
java.net.BindException: Failed to create server socket on
 null[60,026]

Caused by:
java.net.BindException: Address already in use

193 tests completed, 1 failed, 6 skipped

On Thu, Aug 4, 2016 at 10:38 AM, Bruce Schuchardt 
wrote:

> I've pushed the port-range changes that I described in my last email on
> this subject.
>
>
> Le 8/1/2016 à 5:33 PM, Kirk Lund a écrit :
>
>> I think that the changes mentioned by Jens and Bruce obviate the need to
>> do
>> what I was proposing.
>>
>> -Kirk
>>
>>
>> On Fri, Jul 29, 2016 at 3:41 PM, Bruce Schuchardt > >
>> wrote:
>>
>> I'm making another change that will help.
>>>
>>> One of the problems with these tests is that they will choose a random
>>> port for a Cache Server or some other component and only use the port
>>> after
>>> opening a cache.  Doing that allows the communications/membership
>>> component
>>> to grab two ports. AvailablePort restricts the ports it hands out to the
>>> range [2, 3], so if we restrict the communications/membership
>>> component to use ports outside of that range it will help avoid
>>> collisions.
>>>
>>>
>>> Le 7/29/2016 à 3:23 PM, Nabarun Nag a écrit :
>>>
>>> +1 for the retry.

 In my opinion, maintaining available port lists maybe hard as we move
 towards running test modules in parallel. Also maybe some non-geode
 entity
 may come up and pick up a port hence we will need to constantly
 refresh/update the list before/after each test run. (1 ports needs
 to
 be checked as per geode getRandomWildcardBindPortNumber)


 Also for GEODE-1600 fix, DUnitLauncher now passes 0 as the port number
 while creating a locator. The system assigns it an available port number
 while staring the server rather than getting a random available port
 number
 first then asking things to be started on that port. (race conditions
 ensues )

 On Fri, Jul 29, 2016 at 2:36 PM William Markito 
 wrote:

 Why not create a JUnit rule that returns available ports and keep track
 of

> ports being used ?
>
> I've cloned this gist from somewhere (don't remember now) but I've
> planning
> to send it for discussion...
>
> https://gist.github.com/markito/b5be3fc570c7c7c84e6d09e064a6e898
>
> Still talking about rules, I've played a bit with the TemporaryFolder
> rule
> and that's very useful as well, specially to clean up after test runs
> and
> to avoid conflicts.
>
> http://junit.org/junit4/javadoc/4.12/org/junit/rules/Tempora
> ryFolder.html
>
> Just my 2c
>
> On Fri, Jul 29, 2016 at 1:54 PM, Hitesh Khamesra <
> hitesh...@yahoo.com.invalid> wrote:
>
> Is there any possibility of running multiple test same time on that
>
>> machine?
>>
>> -Hitesh
>>
>>
>> From: Kirk Lund 
>>To: geode 
>>Sent: Friday, July 29, 2016 1:21 PM
>>Subject: Flaky tests failing with BindException
>>
>> Many of our flaky tests are flaky because they use AvailablePort or
>> AvailablePortHelper to find randomly available ports. They then later
>>
>> fail
>
> with a BindException because the port is already in use by the time the
>> test uses it.
>>
>> Here's a proposal for a temporary fix:
>>
>> The module geode-junit contains a JUnit 4 rule called RetryRule. We
>> could
>> modify RetryRule to only retry if a BindException (or configurable
>> exception/s) is detected. This rule would then be dropped into every
>> test
>> that uses AvailablePort or AvailablePortHelper. Then if the test fails
>>
>> with
>
> a BindException, it would 

Re: Review Request 51183: GEODE-67: fix nonTxnFindObject for partitioned region

[Ken Howe]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51183/#review146009
---


Ship it!




Ship It!

- Ken Howe


On Aug. 17, 2016, 4:56 p.m., Darrel Schneider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51183/
> ---
> 
> (Updated Aug. 17, 2016, 4:56 p.m.)
> 
> 
> Review request for geode, Eric Shu, Scott Jewell, and Ken Howe.
> 
> 
> Bugs: GEODE-67
> https://issues.apache.org/jira/browse/GEODE-67
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> the pr code path on nonTxnFindObject now propagates isCreate, 
> requestingClient, clientEvent, and returnTombstones
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 
> 304a5ade6d6647bf961345090bd3cc02bc04669f 
> 
> Diff: https://reviews.apache.org/r/51183/diff/
> 
> 
> Testing
> ---
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>

Review Request 51187: GEODE-1760 Sending partial results to user's ResultCollector

[Dan Smith]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51187/
---

Review request for geode and Barry Oglesby.


Repository: geode


Description
---

For non-HA functions on the client, results will now be passed to the
user's result collector, even if some nodes fail due to a bucket movement
or cache close.


Diffs
-

  
geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/ExecuteRegionFunctionOp.java
 d1540204546604ff7ed3d3d490ac40d079ac2953 
  
geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/ExecuteRegionFunctionSingleHopOp.java
 ce7c3f84092f59051e5a148a42cc6f06adc787fe 
  
geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/SingleHopClientExecutor.java
 30cac5bbf1f47652c3d951c26b76e1bb69b8a9b6 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/PartitionedRegion.java
 9ac95a1f9c0ab880d6cab8a350bd9c4842797d5d 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/execute/AbstractExecution.java
 2e981a85b91471a86538d113b5e16d2c67f8e9a7 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/execute/FunctionStreamingResultCollector.java
 7ed908ec8280318a9ad6a1c57780e23eabdaf896 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/execute/LocalResultCollectorImpl.java
 23eb70a787037d7cb5edefa8043aa07882b38677 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/execute/PartitionedRegionFunctionResultSender.java
 d16e9b58fa333f2ef8176759f7d6687e40bc11b6 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/execute/ServerToClientFunctionResultSender.java
 9a3b5d8cdc38847c56f61716594d5f60f63efdfc 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/execute/ServerToClientFunctionResultSender65.java
 4cc80a39738bd8c79e72c9d2120b062a1e4cf9b4 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/partitioned/PRFunctionStreamingResultCollector.java
 5fe869a36304f5f9d10029fdfef92bd8473c9dc3 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/ExecuteFunction70.java
 528e462953b038737880a3f025c2cbc741ea9535 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/functions/TestFunction.java
 d1b1aa519fa257fd65f9e420df33939f831d7dbe 

Diff: https://reviews.apache.org/r/51187/diff/


Testing
---


Thanks,

Dan Smith

Review Request 51188: GEODE-1776: Always read all of the results in ExecuteRegionFunctionOp

[Dan Smith]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51188/
---

Review request for geode and Barry Oglesby.


Repository: geode


Description
---

Read all of the results from the wire in
ExecuteRegionFunctionOp.processResponse, even if we read an exception as
one of the responses. This ensures that all data is read off the wire.


Diffs
-

  
geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/ExecuteFunctionOp.java
 278f1f79afa98823df27974da2907685c7e5e641 
  
geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/ExecuteRegionFunctionOp.java
 d1540204546604ff7ed3d3d490ac40d079ac2953 
  
geode-core/src/main/java/com/gemstone/gemfire/cache/client/internal/ExecuteRegionFunctionSingleHopOp.java
 ce7c3f84092f59051e5a148a42cc6f06adc787fe 

Diff: https://reviews.apache.org/r/51188/diff/


Testing
---


Thanks,

Dan Smith

Review Request 51186: GEODE-1760, GEODE-1762: Tests of function exceptions from clients

[Dan Smith]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51186/
---

Review request for geode and Barry Oglesby.


Repository: geode


Description
---

Extending FunctionServiceBase with tests that use a client server
topology. Added tests of onRegion with PRs as well as onServers.

Adding tests of P2P onRegion with multiple members and PRs.

Adding a temporary workaround for GEODE-1762 to let the function service
tests pass even if a Execution.execute throws an exception. This lets us
at least run the same tests against these different topologies even
though they are throwing different exceptions.

Added a test that result collectors receive partial results even after a
failure.


Diffs
-

  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceBase.java
 eeb7d8d973434bf89a9984e35f83f42d71a31df7 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceClientAccessorPRBase.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceClientAccessorPRDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceClientAccessorPRMultipleMembersDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceClientAccessorPRMultipleMembersMultihopDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceClientBase.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceClientMultipleOnServerDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceClientOnServerBase.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServiceClientOnServerDUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServicePeerAccessorPRBase.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServicePeerAccessorPRDUnitTest.java
 e5bf2d2e6d7069bcf7651d35ed08d0379198cc3c 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/execute/FunctionServicePeerAccessorPRMultipleMembersDUnitTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/51186/diff/


Testing
---


Thanks,

Dan Smith

Review Request 51183: GEODE-67: fix nonTxnFindObject for partitioned region

[Darrel Schneider]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51183/
---

Review request for geode, Eric Shu, Scott Jewell, and Ken Howe.


Bugs: GEODE-67
https://issues.apache.org/jira/browse/GEODE-67


Repository: geode


Description
---

the pr code path on nonTxnFindObject now propagates isCreate, requestingClient, 
clientEvent, and returnTombstones


Diffs
-

  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 
304a5ade6d6647bf961345090bd3cc02bc04669f 

Diff: https://reviews.apache.org/r/51183/diff/


Testing
---

precheckin


Thanks,

Darrel Schneider