[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/incubator-geode/pull/278


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[davinash]

Github user davinash commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86684707
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
 ---
@@ -907,6 +903,31 @@ private void validateRegionFunctionArgs(Cache cache, 
RegionFunctionArgs regionFu
 new Object[] {regionFunctionArgs.getCompressor()}));
   }
 }
+
+if (regionFunctionArgs.hasPartitionAttributes()) {
+  boolean partitionResolverFailure = false;
+  if (regionFunctionArgs.isPartitionResolverSet()) {
+String partitionResolverClassName = 
regionFunctionArgs.getPartitionResolver();
+Object partitionResolver = null;
+
+try {
+  Class compressorClass =
+  (Class) 
ClassPathLoader.getLatest().forName(partitionResolverClassName);
+  partitionResolver = compressorClass.newInstance();
+} catch (InstantiationException e) {
+  partitionResolverFailure = true;
+} catch (IllegalAccessException e) {
+  partitionResolverFailure = true;
+} catch (ClassNotFoundException e) {
+  partitionResolverFailure = true;
+}
+if (partitionResolverFailure || !(partitionResolver instanceof 
PartitionResolver)) {
+  throw new IllegalArgumentException(
--- End diff --

Fixed.


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[davinash]

Github user davinash commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86684704
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
 ---
@@ -907,6 +903,31 @@ private void validateRegionFunctionArgs(Cache cache, 
RegionFunctionArgs regionFu
 new Object[] {regionFunctionArgs.getCompressor()}));
   }
 }
+
+if (regionFunctionArgs.hasPartitionAttributes()) {
+  boolean partitionResolverFailure = false;
+  if (regionFunctionArgs.isPartitionResolverSet()) {
+String partitionResolverClassName = 
regionFunctionArgs.getPartitionResolver();
+Object partitionResolver = null;
+
+try {
+  Class compressorClass =
+  (Class) 
ClassPathLoader.getLatest().forName(partitionResolverClassName);
+  partitionResolver = compressorClass.newInstance();
+} catch (InstantiationException e) {
--- End diff --

Fixed.


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[davinash]

Github user davinash commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86684713
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
 ---
@@ -907,6 +903,31 @@ private void validateRegionFunctionArgs(Cache cache, 
RegionFunctionArgs regionFu
 new Object[] {regionFunctionArgs.getCompressor()}));
   }
 }
+
+if (regionFunctionArgs.hasPartitionAttributes()) {
+  boolean partitionResolverFailure = false;
+  if (regionFunctionArgs.isPartitionResolverSet()) {
+String partitionResolverClassName = 
regionFunctionArgs.getPartitionResolver();
+Object partitionResolver = null;
+
+try {
+  Class compressorClass =
+  (Class) 
ClassPathLoader.getLatest().forName(partitionResolverClassName);
+  partitionResolver = compressorClass.newInstance();
+} catch (InstantiationException e) {
+  partitionResolverFailure = true;
+} catch (IllegalAccessException e) {
+  partitionResolverFailure = true;
+} catch (ClassNotFoundException e) {
+  partitionResolverFailure = true;
+}
+if (partitionResolverFailure || !(partitionResolver instanceof 
PartitionResolver)) {
--- End diff --

Fixed.


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[davinash]

Github user davinash commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86684743
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java
 ---
@@ -1091,6 +1086,126 @@ public void testDestroyRegionWithSharedConfig() {
 });
   }
 
+
+  final String PR_STRING = " package com.cadrdunit;"
--- End diff --

@kirklund Thanks for the details review. I did not understand this comment. 
Could you please help me to understand this. I followed other examples from 
deploy jar tests


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[davinash]

Github user davinash commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86684717
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/RegionCreateFunction.java
 ---
@@ -405,9 +408,48 @@ private CliFunctionResult handleException(final String 
memberNameOrId, final Str
   
prAttrFactory.setStartupRecoveryDelay(partitionArgs.getPrStartupRecoveryDelay());
 }
 
+if (regionCreateArgs.isPartitionResolverSet()) {
+  Class partitionResolverClass = 
forName(regionCreateArgs.getPartitionResolver(),
+  CliStrings.CREATE_REGION__PARTITION_RESOLVER);
+  prAttrFactory
+  .setPartitionResolver((PartitionResolver) 
newInstance(partitionResolverClass,
+  CliStrings.CREATE_REGION__PARTITION_RESOLVER));
+}
 return prAttrFactory.create();
   }
 
+
+  private static Class forName(String className, String neededFor) {
--- End diff --

Fixed.


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86040503
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java
 ---
@@ -1091,6 +1086,126 @@ public void testDestroyRegionWithSharedConfig() {
 });
   }
 
+
+  final String PR_STRING = " package com.cadrdunit;"
--- End diff --

You could add a real class to a sub-package of this test in the source tree 
and then copy that class out to the .jar file. I'm not sure why you'd be making 
this so complex other than to avoid checking in a .jar (which is a disallowed 
binary). 


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86041266
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java
 ---
@@ -1091,6 +1086,126 @@ public void testDestroyRegionWithSharedConfig() {
 });
   }
 
+
+  final String PR_STRING = " package com.cadrdunit;"
+  + " public class TestPartitionResolver implements 
org.apache.geode.cache.PartitionResolver { "
+  + "   @Override" + "   public void close() {" + "   }" + "   
@Override"
+  + "   public Object 
getRoutingObject(org.apache.geode.cache.EntryOperation opDetails) { "
+  + "return null; " + "   }" + "   @Override" + "   public String 
getName() { "
+  + "return \"TestPartitionResolver\";" + "   }" + " }";
+
+  /**
+   * Test Description 1. Deploy a JAR with Custom Partition Resolver 2. 
Create Region with Partition
+   * Resolver 3. Region should get created with no Errors 4. Verify Region 
Partition Attributes for
+   * Partition Resolver
+   * 
+   * @throws IOException
+   */
+  @Test
+  public void testCreateRegionWithPartitionResolver() throws IOException {
+setUpJmxManagerOnVm0ThenConnect(null);
+VM vm = Host.getHost(0).getVM(1);
+// Create a cache in vm 1
+vm.invoke(() -> {
+  assertNotNull(getCache());
+});
+
+ClassBuilder classBuilder = new ClassBuilder();
+// classBuilder.addToClassPath(".");
+final File prJarFile = new File(new File(".").getAbsolutePath(), 
"myPartitionResolver.jar");
--- End diff --

You should start using TemporaryFolder for stuff like this so the test 
cleans up better after it runs.

```
  @Rule
  public TemporaryFolder temporaryFolder = new 
SerializableTemporaryFolder();
...
File prJarFile = new File(temporaryFolder.getRoot(), 
"myPartitionResolver.jar");

```
PS: SerializableTemporaryFolder is a a DUnit friendly version in 
geode-junit.


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86039620
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/RegionCreateFunction.java
 ---
@@ -405,9 +408,48 @@ private CliFunctionResult handleException(final String 
memberNameOrId, final Str
   
prAttrFactory.setStartupRecoveryDelay(partitionArgs.getPrStartupRecoveryDelay());
 }
 
+if (regionCreateArgs.isPartitionResolverSet()) {
+  Class partitionResolverClass = 
forName(regionCreateArgs.getPartitionResolver(),
+  CliStrings.CREATE_REGION__PARTITION_RESOLVER);
+  prAttrFactory
+  .setPartitionResolver((PartitionResolver) 
newInstance(partitionResolverClass,
+  CliStrings.CREATE_REGION__PARTITION_RESOLVER));
+}
 return prAttrFactory.create();
   }
 
+
+  private static Class forName(String className, String neededFor) {
--- End diff --

Class could change to Class everywhere in this class.


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86039193
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
 ---
@@ -907,6 +903,31 @@ private void validateRegionFunctionArgs(Cache cache, 
RegionFunctionArgs regionFu
 new Object[] {regionFunctionArgs.getCompressor()}));
   }
 }
+
+if (regionFunctionArgs.hasPartitionAttributes()) {
+  boolean partitionResolverFailure = false;
+  if (regionFunctionArgs.isPartitionResolverSet()) {
+String partitionResolverClassName = 
regionFunctionArgs.getPartitionResolver();
+Object partitionResolver = null;
+
+try {
+  Class compressorClass =
+  (Class) 
ClassPathLoader.getLatest().forName(partitionResolverClassName);
+  partitionResolver = compressorClass.newInstance();
+} catch (InstantiationException e) {
+  partitionResolverFailure = true;
+} catch (IllegalAccessException e) {
+  partitionResolverFailure = true;
+} catch (ClassNotFoundException e) {
+  partitionResolverFailure = true;
+}
+if (partitionResolverFailure || !(partitionResolver instanceof 
PartitionResolver)) {
--- End diff --

By using Class above, you can eliminate the instanceof 
check here.

Combine all of these changes and you end up with:

```
try {
...
} catch (InstantiationException| IllegalAccessException| 
ClassNotFoundException e) {
  throw new IllegalArgumentException(

CliStrings.format(CliStrings.CREATE_REGION__MSG__INVALID_PARTITION_RESOLVER,
new Object[] {regionFunctionArgs.getCompressor()}), e);
}

```



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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86038578
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
 ---
@@ -907,6 +903,31 @@ private void validateRegionFunctionArgs(Cache cache, 
RegionFunctionArgs regionFu
 new Object[] {regionFunctionArgs.getCompressor()}));
   }
 }
+
+if (regionFunctionArgs.hasPartitionAttributes()) {
+  boolean partitionResolverFailure = false;
+  if (regionFunctionArgs.isPartitionResolverSet()) {
+String partitionResolverClassName = 
regionFunctionArgs.getPartitionResolver();
+Object partitionResolver = null;
+
+try {
+  Class compressorClass =
--- End diff --

Variable name is probably wrong(?) or copied from other code.

Since the class is of a known type, we should specify that here instead of 
using '?':

String partitionResolverClassName = PRResolver.class.getName();
Class resolverClass = 

(Class)ClassPathLoader.getLatest().forName(partitionResolverClassName);
PartitionResolver partitionResolver = resolverClass.newInstance();


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86033727
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
 ---
@@ -907,6 +903,31 @@ private void validateRegionFunctionArgs(Cache cache, 
RegionFunctionArgs regionFu
 new Object[] {regionFunctionArgs.getCompressor()}));
   }
 }
+
+if (regionFunctionArgs.hasPartitionAttributes()) {
+  boolean partitionResolverFailure = false;
+  if (regionFunctionArgs.isPartitionResolverSet()) {
+String partitionResolverClassName = 
regionFunctionArgs.getPartitionResolver();
+Object partitionResolver = null;
+
+try {
+  Class compressorClass =
+  (Class) 
ClassPathLoader.getLatest().forName(partitionResolverClassName);
+  partitionResolver = compressorClass.newInstance();
+} catch (InstantiationException e) {
+  partitionResolverFailure = true;
+} catch (IllegalAccessException e) {
+  partitionResolverFailure = true;
+} catch (ClassNotFoundException e) {
+  partitionResolverFailure = true;
+}
+if (partitionResolverFailure || !(partitionResolver instanceof 
PartitionResolver)) {
+  throw new IllegalArgumentException(
--- End diff --

Use cause from above (you don't need to check it for null because null 
value is same as not having a cause):

 +  throw new IllegalArgumentException(
 +  
CliStrings.format(CliStrings.CREATE_REGION__MSG__INVALID_PARTITION_RESOLVER,
 +  new Object[] {regionFunctionArgs.getCompressor()}), 
cause);
 +}


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86033253
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
 ---
@@ -907,6 +903,31 @@ private void validateRegionFunctionArgs(Cache cache, 
RegionFunctionArgs regionFu
 new Object[] {regionFunctionArgs.getCompressor()}));
   }
 }
+
+if (regionFunctionArgs.hasPartitionAttributes()) {
+  boolean partitionResolverFailure = false;
+  if (regionFunctionArgs.isPartitionResolverSet()) {
+String partitionResolverClassName = 
regionFunctionArgs.getPartitionResolver();
+Object partitionResolver = null;
+
+try {
+  Class compressorClass =
+  (Class) 
ClassPathLoader.getLatest().forName(partitionResolverClassName);
+  partitionResolver = compressorClass.newInstance();
+} catch (InstantiationException e) {
--- End diff --

I'd really like to see us keep the original cause instead of discarding it. 
That will facilitate debugging. You can also you the multiple-catch syntax here:

Exception cause = null;
try {
...
} catch (InstantiationException| IllegalAccessException| 
ClassNotFoundException e) {
  cause = e;
}



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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[kirklund]

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/incubator-geode/pull/278#discussion_r86031904
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
 ---
@@ -31,19 +31,13 @@
 import javax.management.MalformedObjectNameException;
 import javax.management.ObjectName;
 
+import joptsimple.internal.Strings;
--- End diff --

Let's avoid using internal APIs of 3rd party libraries. Looking down, I see 
that you're using Strings.isNullOrEmpty. We have a comparable method in 
org.apache.geode.internal.lang.StringUtils. Please change code from using 
Strings.isNullOrEmpty to StringUtils.isBlank.


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

[GitHub] incubator-geode pull request #278: Feature/geode 1896 unable to specify a Pa...

[davinash]

GitHub user davinash opened a pull request:

https://github.com/apache/incubator-geode/pull/278

Feature/geode 1896 unable to specify a PartitionResolver in gfsh

GEODE-1896: Added a argument in Create Region to specify Partition Resolver.
 


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/davinash/incubator-geode feature/GEODE-1896

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-geode/pull/278.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #278


commit 0541cff3445a1f57fb48e642faa955829f6f7872
Author: adongre 
Date:   2016-10-28T10:34:15Z

Adding option partition-resolver in gfsh while creating a region.

commit 00f7f5faf92dd44aa174648557d2ce4ac64ce17b
Author: adongre 
Date:   2016-10-29T03:05:39Z

Fixing test org.apache.geode.codeAnalysis.AnalyzeSerializablesJUnitTest 
testSerializables




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