> On July 29, 2015, 6:39 p.m., Jason Huynh wrote:
> > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheCreation.java,
> >  line 201
> > <https://reviews.apache.org/r/36906/diff/1/?file=1024360#file1024360line201>
> >
> >     add a space.  so "udaMgr =" instead of "udaMgr="
> >     
> >     Possibly rename to udaMgrCreation to avoid confusion as we end up just 
> > using this to create locally?  I see we do a udaMgr.create but that is on 
> > the creation message and would not be broadcast to others whereas had it 
> > been a "real" udaMgr" it would have been broadcast?

UDAManagerCreate.createUDA ,  just stores the key/val in a Map.
Later when the actual cache is being initialized in the CacheCreation, the data 
is obtained from UDAManagerCreate and added to the actual UDAManagerImpl. This 
creates the UDAs locally ( not distributed). At the end of intialization, it 
collects UDA profile from one of the members. Those UDAs which it did not have 
get created through this. It then checks which UDAs were present only with 
itself and sends those UDAs to all the other peers.


> On July 29, 2015, 6:39 p.m., Jason Huynh wrote:
> > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheCreation.java,
> >  line 311
> > <https://reviews.apache.org/r/36906/diff/1/?file=1024360#file1024360line311>
> >
> >     If multiple instances are coming up at the same time, will they all 
> > exchange messages for the same udaNames/classes and then stop each other 
> > from starting up properly?

I will see if this causes an issue in a test. I doubt there will be any hang or 
something because this is happening outside the synch block.


> On July 29, 2015, 6:39 p.m., Jason Huynh wrote:
> > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheCreation.java,
> >  line 312
> > <https://reviews.apache.org/r/36906/diff/1/?file=1024360#file1024360line312>
> >
> >     are these exceptions possible?  It looks like udaMgrCreation is just a 
> > map?

Not here. But in the actual UDAManagerImpl it can . Since the UDAManager 
interface is implemented by both & its signature throws the exception, it is 
being caught here. I will fix it by using actual implementing class, rather 
than interface.


> On July 29, 2015, 6:39 p.m., Jason Huynh wrote:
> > gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/internal/aggregate/AggregatorJUnitTest.java,
> >  line 60
> > <https://reviews.apache.org/r/36906/diff/1/?file=1024376#file1024376line60>
> >
> >     Since we are testing distinct, try to add the same number twice to make 
> > sure it doesn't get counted?
> >     
> >     So cd1.accumulate(1); cd1.accumulate(1)
> >     
> >     and the scenario where
> >     cd1.accumulate(1); cd2.accumulate(1)

That is already tested at the begining 
CountDistinct count = new CountDistinct();
    count.accumulate(new Integer(5));
    count.accumulate(new Integer(6));
    count.accumulate(new Integer(5));
    count.accumulate(new Integer(6));
    count.accumulate(null);
    count.accumulate(null);
    
    The below ( which you mentioned) is testing when CDs from two different 
nodes ( they contain 3 as common), containing similar item are merged , it 
should not cause problem.


> On July 29, 2015, 6:39 p.m., Jason Huynh wrote:
> > gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/internal/aggregate/AggregatorJUnitTest.java,
> >  line 117
> > <https://reviews.apache.org/r/36906/diff/1/?file=1024376#file1024376line117>
> >
> >     maybe also try sd1.accumulate(5) again?

That is already tested at the begining 
SumDistinct sum = new SumDistinct();
    sum.accumulate(new Integer(5));
    sum.accumulate(new Integer(6));
    sum.accumulate(null);
    sum.accumulate(new Integer(5));
    sum.accumulate(new Integer(6));

The below ( which you mentioned) is testing when CDs from two different nodes ( 
they contain 3 as common), containing similar item are merged , it should not 
cause problem.


> On July 29, 2015, 6:39 p.m., Jason Huynh wrote:
> > gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/internal/aggregate/AggregatorJUnitTest.java,
> >  line 208
> > <https://reviews.apache.org/r/36906/diff/1/?file=1024376#file1024376line208>
> >
> >     try ad1.accumulate(4)?

That is already tested at the begining 
 AvgDistinct avg = new AvgDistinct();
    avg.accumulate(new Integer(1));
    avg.accumulate(new Integer(2));
    avg.accumulate(new Integer(2));
    avg.accumulate(new Integer(3));
    avg.accumulate(new Integer(3));
    avg.accumulate(new Integer(4));
    avg.accumulate(new Integer(5));
    avg.accumulate(new Integer(6));
    avg.accumulate(new Integer(7));
    avg.accumulate(new Integer(7));
    avg.accumulate(new Integer(6));
    avg.accumulate(null);
    avg.accumulate(null);
    float expected = (1 + 2+ 3 + 4 + 5 + 6 + 7)/7.0f ;    
    assertEquals(expected, ((Number)avg.terminate()).floatValue());

The below ( which you mentioned) is testing when CDs from two different nodes ( 
they contain 3 as common), containing similar item are merged , it should not 
cause problem.


- Asif


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


On July 29, 2015, 9:17 a.m., Asif Shahid wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36906/
> -----------------------------------------------------------
> 
> (Updated July 29, 2015, 9:17 a.m.)
> 
> 
> Review request for geode.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Support for UserDefinedAggregates in OQL. Support for allowing creation of 
> UDAs to be used in OQL
> 
> 
> Implemented changes for uda creation through cache xml ( declarative syntax)
> 
> 
> Fixed the bug in profile exchange of UDA. Added test for the same
> 
> 
> Diffs
> -----
> 
>   gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/Aggregator.java 
> 9f54eba48072466b139151173216238bc180d289 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/QueryService.java 
> 4a3cb46f4de4f263e8a7c8f26684392346f42e4a 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/UDAExistsException.java
>  PRE-CREATION 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/CompiledAggregateFunction.java
>  ba7731026f096cb0bd70d0cc04b391f46492f33a 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/CompiledGroupBySelect.java
>  ad41c309a906267c3383a4d444a62ad83dbd7ca6 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/CompiledUDAFunction.java
>  PRE-CREATION 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/DefaultQueryService.java
>  71d2e86095e51ead86b24c098dbc8db6de8ffa84 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/ProxyQueryService.java
>  8a20f10347aee8f3e29b953b5f39baa49dda017a 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/QCompiler.java
>  4777dc1b9a3b7576180fec5e4dc159ed2b30d882 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/AbstractAggregator.java
>  5809df8fe85d691807809898f1753b665f355cc1 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/Avg.java
>  e1da36d2f57a96829f7f1dc9a71e4ac607d34777 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/AvgBucketNode.java
>  96113218cc04c4a98610a1183680fb0da9918c4d 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/AvgDistinct.java
>  3863b72a2df3b2e7f27727e409aafcbdae31aa4e 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/AvgDistinctPRQueryNode.java
>  2bbc67d2a4b44c60da9882bd95e7b7c289c8fc20 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/AvgPRQueryNode.java
>  23654742130dbb1d20f3fb2e45c29eb0dbea46db 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/Count.java
>  3026b63e5a3a8f6a3bf1b4553b8e611a111dbd7a 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/CountDistinct.java
>  03af70bf6cee6ce0fd37227efbd33b5419a0e6f3 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/CountDistinctPRQueryNode.java
>  4e56f303d227ba2f746b8d86b56134c3c3182182 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/CountPRQueryNode.java
>  34d34ac17b16bd9b9eb2ca06f022099b798ae191 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/DistinctAggregator.java
>  0328d9a4c06a832daf9ca2aabcb2ec8bb22403e4 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/MaxMin.java
>  443e62ddeff304824504923cf4e3ddbbc2b6555d 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/Sum.java
>  c41d0a520bdb7636f9d7c23776149b58679b94e3 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/SumDistinct.java
>  e4741dc78f03fda61f031cca1668b696466a719e 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/SumDistinctPRQueryNode.java
>  99c6fe7cd819ddb9e94bbc85937a79acfccb7f4d 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/uda/UDADistributionAdvisor.java
>  PRE-CREATION 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/uda/UDAManager.java
>  PRE-CREATION 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/uda/UDAManagerImpl.java
>  PRE-CREATION 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/aggregate/uda/UDAMessage.java
>  PRE-CREATION 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/parse/ASTAggregateFunc.java
>  1bfe010a2109e32a2004ab1e8b12e0971f982254 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/parse/OQLLexer.java
>  2ada98da64bff6a504fa921529392dd4bcfd4a9d 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/parse/OQLLexerTokenTypes.java
>  f30f3c09e5e8fd30211f69dc9a9dce12c87e9c7b 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/parse/OQLLexerTokenTypes.txt
>  8c8fda57fc2d40133b88846254f328b0ab6350bb 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/parse/OQLParser.java
>  bb70b056222e935b98a9c71b155fe6d08738e573 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/cache/query/internal/parse/oql.g
>  e18e480785c6260bcbb00661d2eee1b449e80ae2 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/internal/DistributionAdvisor.java
>  75cadc9232ffdbf35197b4b4a6cf6378ce703581 
>   gemfire-core/src/main/java/com/gemstone/gemfire/internal/DSFIDFactory.java 
> 3c33553320276ca843f6f80a6ad24043d36293e4 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/DataSerializableFixedID.java
>  a8a17153b473143fe73ce965bde68ce81a2e0114 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java
>  54870002be6d2aadd5232e317661b668fc92be7b 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/InternalCache.java
>  7c1fa8eea232baafd7d40c15437ee4f09b20fccf 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/UpdateAttributesProcessor.java
>  2ff6f6f20fdc8a590a98701e24ab5597296e9147 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheCreation.java
>  b9fcfe7d215c9ff935df52a0d2fa587348e51922 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXml.java
>  e39663014c9bb4f9d7001c2c9216e24c2e90c490 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXmlGenerator.java
>  06eb091a80b09da7718288e67f8622da9387858c 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/CacheXmlParser.java
>  9f2cbc098a1bf581c9d523603e072ffaf9138abb 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/xmlcache/UDAManagerCreation.java
>  PRE-CREATION 
>   
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/i18n/ParentLocalizedStrings.java
>  21fc52ee132f3d72a60587e460be5ea807d92bfa 
>   
> gemfire-core/src/main/resources/META-INF/schemas/schema.pivotal.io/gemfire/cache/cache-9.0.xsd
>  35af50ccbdbd294282891f1631dbe57b137522c4 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/dunit/GroupByPartitionedQueryDUnitTest.java
>  d3e1aca82d78ed10bf252687bc8cb511a522d89f 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/dunit/UDACreationDUnitTest.java
>  PRE-CREATION 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/dunit/UDADUnitImpl.java
>  PRE-CREATION 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/dunit/UDAPartitionedQueryDUnitTest.java
>  PRE-CREATION 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/functional/UDAPartitionedJUnitTest.java
>  PRE-CREATION 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/functional/UDAReplicatedJUnitTest.java
>  PRE-CREATION 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/functional/UDATestImpl.java
>  PRE-CREATION 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/functional/UDATestInterface.java
>  PRE-CREATION 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/internal/CompiledAggregateFunctionJUnitTest.java
>  036fbd2370d8edf27202b4c80c53ca8df93c0f91 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/query/internal/aggregate/AggregatorJUnitTest.java
>  47243ad4ce42fa3513ac36adadaef5366f14b103 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml90DUnitTest.java
>  b268ada17b2710f69849fca68185d5475521818d 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXmlTestCase.java 
> 8d6bf19ca7954688761353c2e11eb061d2267982 
> 
> Diff: https://reviews.apache.org/r/36906/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Asif Shahid
> 
>

Reply via email to