Re: Review Request 51010: GEODE-11 Test for Cluster Configuration changes for Lucene index.

[anilkumar gingade]

> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> >

Jason, Thanks for the review...Good points...


> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java,
> >  line 82
> > 
> >
> > Is this method needed?

I like to keep it so that, if future there is need for any intial settings that 
can be done here...One could add it in the future, but it makes it easier 
having the method there...


> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java,
> >  line 115
> > 
> >
> > Should we limit the index here or rely on the host.getVM for which 
> > index values are valid?
> > 
> > If we do want to limit it in this rule, maybe break out 1 and 3 into 
> > constants?

We can rely on host.getVM...just need to check on vm_0 used for Locator...Will 
make that change...


> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java,
> >  line 125
> > 
> >
> > If mcast port is set... what will happen?  This checks to see if it's 
> > not set.

If set it will use the MCAST port set by the test...I need to do the same 
change with getNode() will add that...


> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java,
> >  line 103
> > 
> >
> > Should we seperate this test into two tests?  One for when the new node 
> > is created in the group and one for when it is out of the group?  This test 
> > is named in a way that made me think every new node was in the group and 
> > should have the index, but vm3 is outside the group and verifies it does 
> > not get an index

Good idea...I will split this...


> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java,
> >  line 154
> > 
> >
> > What currently happens if we have RegionShortcut.REPLICATE?  The index 
> > itself is just not created but everything else keeps on running?

I believe so..Its upto the gfsh command how we have added...


> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java,
> >  line 189
> > 
> >
> > Will this dir and clusterConfigDir get cleaned up after the test is 
> > run?  Should this use a TemporaryFolder rule or something else?

It uses the TemporaryFolder...yes it clears the config dir...Without this i was 
having problem with the second test, where it was loading the cluster config 
from previous test...


- anilkumar


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


On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> ---
> 
> (Updated Aug. 11, 2016, 10:34 p.m.)
> 
> 
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk 
> Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Added test to validate Cluster configuration support for Lucene indexes.
> 
> Added a new Rule to create Distributed System with test/custom 
> configuration...The "LocatorServerConfigurationRule" makes it easier to 
> create a Locator or cluster nodes with required configuration.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java
>  PRE-CREATION 
>   
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51010/diff/
> 
> 
> Testing
> ---
> 
> Run the newly added test.
> 
> 
> Thanks,
> 
> anilkumar gingade
> 
>

Review Request 51018: fix diskStats and prStats related to entry count and data size

[Darrel Schneider]

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

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


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


Repository: geode


Description
---

entriesInVM no longer counts entries that have no value in the VM.
Before it was counting invalid entries as being inVM.
Also fixed some code paths that were not updating the PR stats which led to 
dataStoreEntriesInUse not being consistent with CachePerfStats entries.


Diffs
-

  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractLRURegionMap.java
 c9b3038f79ae917b04316e89017c84daa4a8d76a 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java
 f3cb3d654087c797de430f448348c3ba66846ac4 
  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DiskEntry.java 
98ee7292a3ae75d8d5bc26a7af3d64f34cea55e8 
  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 
46ccd47e96e75a8c4c6f3075b65a2d2447043213 
  
geode-core/src/test/java/com/gemstone/gemfire/cache30/DiskRegionDUnitTest.java 
a43950edde101c8c28c7773571f6e74b5dfad5fd 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/DiskRegRecoveryJUnitTest.java
 792f07eb1a8af4a93eb67ad15378fdbf350d218e 

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


Testing
---

precheckin
Also run a bunch of hydra tests that had been failing


Thanks,

Darrel Schneider

[Spring CI] Spring Data GemFire > Nightly-ApacheGeode > #398 was SUCCESSFUL (with 1423 tests)

[Spring CI]

---
Spring Data GemFire > Nightly-ApacheGeode > #398 was successful.
---
Scheduled
1425 tests in total.

https://build.spring.io/browse/SGF-NAG-398/





--
This message is automatically generated by Atlassian Bamboo

Re: Geode M3 docs: Review opportunity

[Dave Barnes]

This version of the Geode User Manual has been posted to the public site:
http://geode.docs.pivotal.io/



On Wed, Aug 10, 2016 at 1:15 PM, William Markito 
wrote:

> LGTM.  +1
>
> Thanks Dave!
>
> On Mon, Aug 8, 2016 at 11:40 AM, Dave Barnes  wrote:
>
> > I've posted the Geode User docs for the upcoming M3 release here:
> >
> > http://geode-review.cfapps.io/
> >
> > Please have a look at the section(s) describing the features you've
> > implemented and send me any suggestions or corrections.
> > Deadline is Code Release or this Friday, Aug 12, whichever comes first.
> > Thanks,
> > -Dave
> >
>
>
>
> --
>
> ~/William
>

Re: Revised SSL properties failure scenario advice.

[Darrel Schneider]

how does ssl-default-alias interact with this one in the spec:
   ssl-certificate-alias=[empty,string] (default - use first cert in
keystore)
I thought ssl-certificate-alias was the one to use by default. It could be
overridden by a component one.
And the default for it is to use the first certificate in the keystore. Are
you suggesting that the default is only the first cert when the keystore
only has one?

I think having both ssl-default-alias and ssl-certificate-alias would be
confusing (I'm currently confused by it).

I also don't understand the failure scenario you describe. Here is an
example of what I hear you saying:
if the keystore how more than one cert and you do not configure a default
but you do configure a specific cert alias for each component then fail.
If all the certs they configure are in the key store then what is the
reason for failure? Is some other component is using the default one and
the default is either undefined or non-existent then I would vote for
failure.


On Wed, Aug 10, 2016 at 4:59 PM, Kirk Lund  wrote:

> +1 to introduce "ssl-default-alias" and fail if it's not set for multi-key
> keystore
>
>
> On Wed, Aug 10, 2016 at 4:34 PM, Bruce Schuchardt 
> wrote:
>
> > +1 for ssl-default-alias and failing
> >
> >
> > Le 8/10/2016 à 3:36 PM, Udo Kohlmeyer a écrit :
> >
> >> Hi there guys,
> >>
> >> As per the proposal for the revision of the SSL configuration <
> >> https://cwiki.apache.org/confluence/display/GEODE/
> Revised+SSL+properties>,
> >> I'm nearing the completion of this feature.
> >>
> >> What I have come across is some scenarios where a system is configured
> >> with a multi-key keystore and all Geode components are marked to use
> SSL.
> >> As the ssl configuration factory would not know what key to use, it
> might
> >> fail to correctly configure SSL comms.
> >>
> >> In this scenario, would it make sense to introduce another property
> >> "ssl-default-alias" which specifies the default certificate alias to be
> >> used in a multi-key keystore?
> >>
> >> Also, in the scenario where a single component specifies a different
> >> alias to be used, should we fail if the "ssl-default-alias" has not been
> >> set for a multi-key keystore?
> >>
> >> Any advice or opinions would be appreciated.
> >>
> >> --Udo
> >>
> >>
> >>
> >
>

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




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


This is the grandchild case right?
I don't see it adding anything to the ColocationLogger so how does that 
work?



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


I don't see a need to for this for loop.
It actually does more work than needed because once it finds a match it 
should have just returned.

Logically I think you just wanted to do this:
String fullPath = region.getFullPath();
if (missingChildren.contains(fullPath) {
  removeMissingChildRegion(fullPath);
}

But the other thing you are doing is a double check. You test if it is in 
the collection here and then if it is you call removeMissingChildRegion; it 
syncs; and then you basically do this check again.
This does avoid the sync on a miss but do you really need this?
Couldn't this method just call:
removeMissingChildRegion(region.getFullPath());



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


This was the old code style for catch ( "} LF catch"). I think you got this 
code from and old class.
In this new class I think you should convert this to the new style "} 
catch".



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


!isEmpty() better than size() > 0
same goes for line 139



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


This name (warningLogged) is a bit misleading since we might not actually 
log a warning even when it is set to true.
I change to something like "alreadySlept".
Setting it to true should happen inside the if that tests for it to be 
false instead of everytime around the loop.



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


sleepMillis /= 2;
is a bit clearer



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


This does not seem like the correct handling of InterruptedException. I 
think in this thread if wait is interrupted you just want the thread to 
terminate. The way you currently have it coded up it can't be interrupted.
I would move this catch to your outer try and get rid of this inner try 
completely.



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


Why is catch Exception needed? Seems like anything it would catch should 
just be thrown and your code up in run will handle it.



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


Also is it seems like you might as well check the result of remove and only 
do the isEmpty check if remove returned non-null.



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


I noticed that here you have "this.missingChildren" but in other places you 
drop the "this.". We used to require that instance var references included 
"this.". It was part of our coding guidelines.
But given IDEs I'd vote for not adding "this." unless it is required 
because of a local var having the same name as an inst var.
Since you have a mix in this class I'd recommend that you can this new 
class for "this." that you can remove and do so.



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


Your "getMissingChildRegions" clones the "missingChildren" collection. No 
need for that here since you are synced on the loggerLock.
Also it is better to call isEmpty() instead of size() == 0.
So I'd change this line to:
if (missingChildren.isEmpty())



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


I see no reason for you doing this clone.
The whole point of CopyOnWriteArrayList is to provide safe concurrent 
access. It looks like this method is for afterCreate and I don't see a need for 
it to get a clone.



geode-c

Review Request 50967: GEODE-599: CI Failure: DistributedAckPersistentRegionCCEDUnitTest.testClearWithConcurrentEventsAsync

[Scott Jewell]

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

Review request for geode, Darrel Schneider, Eric Shu, and Ken Howe.


Repository: geode


Description
---

**I'LL SQUASH THIS WHEN I PUSH IT UP FOR THE PULL REQUEST**

Cache modification lock was being released before operation distribution to 
other members.
This provided a small window in which an operation from another thread could 
update the
region prior to the other members receiving notification (i.e. down leveled)

GEODE-599:  Flush current operations if region scope is NO_ACK

GEODE-599: Cleanup commented code

GEODE-599: General code cleanup of ClearRvvLockingDUnitTest

GEODE-599:  Remove unnecessary region reference from RegionVersionVector

commit


Merge remote-tracking branch 'origin/develop' into feature/GEODE-599


Merge remote-tracking branch 'origin/develop' into feature/GEODE-599


GEODE-599: Resolve issues with region operations and RVV locking


Merge remote-tracking branch 'origin/develop' into feature/GEODE-599


GEODE-599: Add license to ARMLockTestHookAdapter


GEODE-599: Remove reference to test class VM from AbstractRegionMap


GEODE-599: Remove test class VM from ARM


GEODE-599: Refactor ARMLockTestHook interface


GEODE-599: Ensure test hook reset after test via finally clause


Merge remote-tracking branch 'origin/develop' into feature/GEODE-599


Diffs
-

  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java
 226e1948412a178c2a18a1e0a7d701b331c57e72 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/BucketRegion.java 
f1627d38b9d84c673a15c812712ebad297ce87ce 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedClearOperation.java
 5526ef01639869b10b0f8e475a6c23f62bdf9a86 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedRegion.java
 b42a617fe681700ca0c58f76c88b52535b7692b2 
  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 
46ccd47e96e75a8c4c6f3075b65a2d2447043213 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ProxyRegionMap.java
 3ad2cc1ff4fb352571bf6073d535fefb5837b12f 
  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionMap.java 
14a2d2f09e2ff83b84080b2df558290a7a47710f 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/StateFlushOperation.java
 88666898574dfde3a2f7a148144f41f18f56687e 
  
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/versions/RegionVersionVector.java
 2f024229d72c6259a360b1742fe33dbd8d6a2ab3 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ARMLockTestHookAdapter.java
 PRE-CREATION 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ClearRvvLockingDUnitTest.java
 PRE-CREATION 

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


Testing
---


Thanks,

Scott Jewell

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. 11, 2016, 5:51 p.m.)


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


Changes
---

Updated per review comments


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/i18n/LocalizedStrings.java
 2254a89 
  
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 
  
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/partitioned/PersistentPartitionedRegionTestBase.java
 692378c 

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


Testing
---

precheckin in progress


Thanks,

Ken Howe

Re: Experimental features for 1.0

[William Markito]

Cool! I'll take a stab looking current stuff in develop and already opened
tickets.

On Thu, Aug 11, 2016 at 9:39 AM, Anthony Baker  wrote:

> I think that is the relevant point.  We are making releases from develop,
> not from WIP branches.
>
> @John, HDFS was moved to a WIP branch.
>
> Anthony
>
> > On Aug 11, 2016, at 9:31 AM, Kirk Lund  wrote:
> >
> > JVSD is not on develop, so maybe not. We probably only need to annotate
> and
> > file tickets for incomplete features on develop.
> >
> > -Kirk
> >
>
>


-- 

~/William

Re: Experimental features for 1.0

[Anthony Baker]

I think that is the relevant point.  We are making releases from develop, not 
from WIP branches.

@John, HDFS was moved to a WIP branch.

Anthony

> On Aug 11, 2016, at 9:31 AM, Kirk Lund  wrote:
> 
> JVSD is not on develop, so maybe not. We probably only need to annotate and
> file tickets for incomplete features on develop.
> 
> -Kirk
> 



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: Experimental features for 1.0

[Kirk Lund]

JVSD is not on develop, so maybe not. We probably only need to annotate and
file tickets for incomplete features on develop.

-Kirk


On Wed, Aug 10, 2016 at 10:35 PM, Dave Barnes  wrote:

> JVSD is not fully-featured, yet. Does it belong on the list?
>
> > On Aug 10, 2016, at 6:26 PM, Kirk Lund  wrote:
> >
> > +1 for Jira tickets with "experimental" label
> >
> > Also, don't forget we have the @Experimental annotation defined in
> > geode-common that should be applied to any experimental user APIs before
> > final 1.0.0 release.
> >
> > -Kirk
> >
> >> On Wednesday, August 10, 2016, William Markito 
> wrote:
> >>
> >> As we move forward to 1.0 I'd like to propose creating JIRAS with the
> >> "experimental" label to capture everything we have that is classified as
> >> such.
> >>
> >> This would help users and projects consume Geode to consider those
> >> features properly and decide to add support (or not) to them as well.
> >>
> >> AFAIK this is the current list:
> >>
> >> Redis Adapter
> >> Spark Connector
> >> OQL Aggregates
> >> Lucene integration
> >> Auto Rebalancer
> >>
> >> Is there anything else ? Are any of these not considered experimental
> >> anymore ?
> >>
> >> If by the time we get to 1.0 they're complete we'd remove be label
> >> obviously.
> >>
> >> Thoughts ?
> >>
> >>
> >>
> >>
> >>
>

[GitHub] incubator-geode issue #230: GEODE-1744: Probable bugs from == use fixed

[kjduling]

Github user kjduling commented on the issue:

https://github.com/apache/incubator-geode/pull/230
  
No existing test exposed this.  This is in reaction to a code analysis.  
Case by case, the code may or may not be doing what the developer intended 
because of the nature of == vs .equals().  If the == test is true, then the 
.equals() test must also be true.  However, just because == is false does not 
mean .equals() is not true.

Existing testing did not reveal this.  I'll work with Grace to come up with 
tests to verify.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---