[GitHub] incubator-geode issue #285: GEODE-1617: Regions can be created with a variet...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/285 Pushed to develop. Thanks! --- 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 #285: GEODE-1617: Regions can be created with a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/285#discussion_r88075502
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
---
@@ -0,0 +1,75 @@
+package org.apache.geode.cache;
+
+import static org.junit.Assert.fail;
+
+import org.apache.geode.internal.cache.InternalRegionArguments;
+import org.apache.geode.internal.cache.LocalRegion;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+@Category(UnitTest.class)
+public class RegionNameValidationJUnitTest {
+ private static final Pattern NAME_PATTERN =
Pattern.compile("[aA-zZ0-9-_.]+");
+ private static final String REGION_NAME = "MyRegion";
+
+
+ @Test
+ public void testInvalidNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+try {
+ LocalRegion.validateRegionName(null, ira);
+ fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+ LocalRegion.validateRegionName("", ira);
+ fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+ LocalRegion.validateRegionName("FOO" + Region.SEPARATOR, ira);
+ fail();
+} catch (IllegalArgumentException ignore) {
+}
+
+ }
+
+ @Test
+ public void testExternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+validateCharacters(ira);
+try {
+ LocalRegion.validateRegionName("__InvalidInternalRegionName", ira);
+} catch (IllegalArgumentException ignore) {
+}
+ }
+
+ @Test
+ public void testInternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(true);
+LocalRegion.validateRegionName("__ValidInternalRegionName", ira);
+ }
+
+ private void validateCharacters(InternalRegionArguments ira) {
+for (int x = 0; x < Character.MAX_VALUE; x++) {
+ String name = (char) x + REGION_NAME;
+ Matcher matcher = NAME_PATTERN.matcher(name);
+ if (matcher.matches()) {
+LocalRegion.validateRegionName(name, ira);
+ } else {
+try {
+ LocalRegion.validateRegionName(name, ira);
+ fail("Should have received an IllegalArgumentException");
--- End diff --
Recommend adding which character to the message so we get instant info
about which character should have been illegal but was allowed.
---
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 #285: GEODE-1617: Regions can be created with a...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/285#discussion_r88075036
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
---
@@ -0,0 +1,75 @@
+package org.apache.geode.cache;
+
+import static org.junit.Assert.fail;
+
+import org.apache.geode.internal.cache.InternalRegionArguments;
+import org.apache.geode.internal.cache.LocalRegion;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+@Category(UnitTest.class)
+public class RegionNameValidationJUnitTest {
+ private static final Pattern NAME_PATTERN =
Pattern.compile("[aA-zZ0-9-_.]+");
+ private static final String REGION_NAME = "MyRegion";
+
+
+ @Test
+ public void testInvalidNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+try {
+ LocalRegion.validateRegionName(null, ira);
+ fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+ LocalRegion.validateRegionName("", ira);
+ fail();
+} catch (IllegalArgumentException ignore) {
+}
+try {
+ LocalRegion.validateRegionName("FOO" + Region.SEPARATOR, ira);
+ fail();
+} catch (IllegalArgumentException ignore) {
+}
+
+ }
+
+ @Test
+ public void testExternalRegionNames() {
+InternalRegionArguments ira = new InternalRegionArguments();
+ira.setInternalRegion(false);
+validateCharacters(ira);
+try {
+ LocalRegion.validateRegionName("__InvalidInternalRegionName", ira);
--- End diff --
Aren't you missing a fail() stmt here?
---
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 issue #283: GEODE-2098: Moved gfsh history file location fro...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/283 +1 looks ready to push to develop. Once it's committed on develop, then mark the Jira ticket Resolved (not Closed) and Fix Version(s) should say 1.1.0-incubating. When 1.1.0 is released, all Resolved tickets are moved to Closed (all such tickets will be part of release notes automatically I think). --- 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 issue #285: GEODE-1617: Regions can be created with a variet...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/285 I reworded your commit message: ``` GEODE-1617: add test for region names Added comprehensive test to validate region names This closes #285 ``` --- 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 issue #285: GEODE-1617: Regions can be created with a variet...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/285 +1 --- 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 issue #283: GEODE-2098: Moved gfsh history file location fro...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/283 I recommend discussing backwards compatibility of .gemfire vs .geode directory (which contains gfsh history file) on the dev list. I don't really feel strongly one way or the other (supporting .gemfire vs just a hard switch to .geode). Dev list is good place to discuss backwards compatibility though. --- 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 issue #283: GEODE-2098: Moved gfsh history file location fro...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/283 Since Geode 1.0 released using .gemfire history file, we should probably read and use .gemfire if .geode does not exist. If both files exist then .geode should take precedence. What OS does DiskSpaceLimitIntegrationTest fail on for you? --- 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...
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...
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...
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<K, V>)
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...
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...
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...
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...
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...
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 issue #273: Feature/geode 1983
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/273 Travis failed with: * What went wrong: Execution failed for task ':geode-core:spotlessJavaCheck'. > Format violations were found. Run 'gradlew spotlessApply' to fix them. geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessage.java geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java geode-core/src/test/java/org/apache/geode/internal/cache/BucketRegionQueueHelper.java Have you run precheckin yet? --- 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 issue #249: GEODE-1907: QueryDataFunction now adds LIMIT cla...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/249 +1 pulling in now --- 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 #247: Feature/geode 1902
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/247#discussion_r81405967
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/logging/log4j/LogMarkerJUnitTest.java
---
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.geode.internal.logging.log4j;
+
+
+import static
org.apache.geode.internal.logging.log4j.custom.CustomConfiguration.*;
+import static org.assertj.core.api.Assertions.*;
+
+import java.io.File;
+
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.core.config.ConfigurationFactory;
+import org.apache.logging.log4j.status.StatusLogger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.SystemErrRule;
+import org.junit.contrib.java.lang.system.SystemOutRule;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TemporaryFolder;
+
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.internal.logging.log4j.custom.BasicAppender;
+import org.apache.geode.test.junit.categories.UnitTest;
+
+/**
+ * Integration tests with custom log4j2 configuration.
+ */
+@Category(UnitTest.class)
+public class LogMarkerJUnitTest {
+
+ private String beforeConfigFileProp;
+ private Level beforeLevel;
+
+ private File customConfigFile;
+
+ @Rule
+ public SystemErrRule systemErrRule = new SystemErrRule().enableLog();
+
+ @Rule
+ public SystemOutRule systemOutRule = new SystemOutRule().enableLog();
+
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ @Before
+ public void setUp() throws Exception {
+Configurator.shutdown();
+BasicAppender.clearInstance();
+
+this.beforeConfigFileProp =
System.getProperty(ConfigurationFactory.CONFIGURATION_FILE_PROPERTY);
+this.beforeLevel = StatusLogger.getLogger().getLevel();
+
+this.customConfigFile =
createConfigFileIn(this.temporaryFolder.getRoot());
+
+System.setProperty(ConfigurationFactory.CONFIGURATION_FILE_PROPERTY,
this.customConfigFile.getAbsolutePath());
+LogService.reconfigure();
+
assertThat(LogService.isUsingGemFireDefaultConfig()).as(LogService.getConfigInformation()).isFalse();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+Configurator.shutdown();
+
+System.clearProperty(ConfigurationFactory.CONFIGURATION_FILE_PROPERTY);
+if (this.beforeConfigFileProp != null) {
+ System.setProperty(ConfigurationFactory.CONFIGURATION_FILE_PROPERTY,
this.beforeConfigFileProp);
+}
+StatusLogger.getLogger().setLevel(this.beforeLevel);
+
+LogService.reconfigure();
+
assertThat(LogService.isUsingGemFireDefaultConfig()).as(LogService.getConfigInformation()).isTrue();
+
+BasicAppender.clearInstance();
+
+assertThat(this.systemErrRule.getLog()).isEmpty();
+ }
+
+ /**
+ * Test to see that log messages for GEODE_VERBOSE are filtered, based
on the log4j2-custom.xml configuration file
+ */
+ @Test
+ public void testGeodeFilter()
+ {
--- End diff --
Please update, reload /etc/intellijIdeaCodeStyle.xml and reformat.
---
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 #247: Feature/geode 1902
Github user kirklund commented on a diff in the pull request: https://github.com/apache/incubator-geode/pull/247#discussion_r81405863 --- Diff: geode-core/src/test/java/org/apache/geode/internal/logging/log4j/LogMarkerJUnitTest.java --- @@ -0,0 +1,118 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.geode.internal.logging.log4j; + + +import static org.apache.geode.internal.logging.log4j.custom.CustomConfiguration.*; +import static org.assertj.core.api.Assertions.*; + +import java.io.File; + +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.config.ConfigurationFactory; +import org.apache.logging.log4j.status.StatusLogger; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.contrib.java.lang.system.SystemErrRule; +import org.junit.contrib.java.lang.system.SystemOutRule; +import org.junit.experimental.categories.Category; +import org.junit.rules.TemporaryFolder; + +import org.apache.geode.internal.logging.LogService; +import org.apache.geode.internal.logging.log4j.custom.BasicAppender; +import org.apache.geode.test.junit.categories.UnitTest; + +/** + * Integration tests with custom log4j2 configuration. + */ +@Category(UnitTest.class) --- End diff -- Please change to IntegrationTest.class since this exercises entire LogService and touches the file system. --- 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 #249: GEODE-1907: QueryDataFunction now adds LI...
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/249#discussion_r81404596
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/beans/QueryDataFunction.java
---
@@ -360,31 +373,36 @@ public static Object queryData(final String query,
for (String regionPath : regionsInQuery) {
DistributedRegionMXBean regionMBean =
service.getDistributedRegionMXBean(regionPath);
if (regionMBean == null) {
-return new
JsonisedErroMessage(ManagementStrings.QUERY__MSG__REGIONS_NOT_FOUND.toLocalizedString(regionPath)).toString();
+return new
JsonisedErroMessage(ManagementStrings.QUERY__MSG__REGIONS_NOT_FOUND.toLocalizedString(regionPath))
+ .toString();
} else {
Set associatedMembers =
DataCommands.getRegionAssociatedMembers(regionPath, cache, true);
if (inputMembers != null && inputMembers.size() > 0) {
if (!associatedMembers.containsAll(inputMembers)) {
-return new
JsonisedErroMessage(ManagementStrings.QUERY__MSG__REGIONS_NOT_FOUND_ON_MEMBERS.toLocalizedString(regionPath))
- .toString();
+return new
JsonisedErroMessage(ManagementStrings.QUERY__MSG__REGIONS_NOT_FOUND_ON_MEMBERS
+ .toLocalizedString(regionPath)).toString();
}
}
}
}
} else {
-return new
JsonisedErroMessage(ManagementStrings.QUERY__MSG__INVALID_QUERY.toLocalizedString("Region
mentioned in query probably missing /")).toString();
+return new
JsonisedErroMessage(ManagementStrings.QUERY__MSG__INVALID_QUERY.toLocalizedString("Region
mentioned in query probably missing /"))
+ .toString();
}
// Validate
if (regionsInQuery.size() > 1 && inputMembers == null) {
for (String regionPath : regionsInQuery) {
DistributedRegionMXBean regionMBean =
service.getDistributedRegionMXBean(regionPath);
- if
(regionMBean.getRegionType().equals(DataPolicy.PARTITION.toString()) ||
-
regionMBean.getRegionType().equals(DataPolicy.PERSISTENT_PARTITION.toString()))
{
-return new
JsonisedErroMessage(ManagementStrings.QUERY__MSG__JOIN_OP_EX.toLocalizedString()).toString();
+ if (regionMBean.getRegionType()
--- End diff --
I updated the formatters in etc/ including etc/intellijIdeaCodeStyle.xml to
prevent these whacky linefeeds. Please update and rerun the formatter. The
changes look good and I'll pull it in as soon as you've reformatted. Thanks!
---
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 #248: GEODE-1548: Specifying --J=-Dgemfire.jmx-...
Github user kirklund commented on a diff in the pull request: https://github.com/apache/incubator-geode/pull/248#discussion_r81402893 --- Diff: geode-core/src/test/java/org/apache/geode/management/internal/security/JavaRmiServerNameJunitTest.java --- @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.geode.management.internal.security; + +import static org.apache.geode.distributed.ConfigurationProperties.*; +import static org.junit.Assert.*; + +import java.util.Properties; + +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.cache.CacheFactory; +import org.apache.geode.internal.AvailablePort; +import org.apache.geode.test.junit.categories.UnitTest; + +@Category(UnitTest.class) --- End diff -- The test name is ok but I'd probably make it JavaRmiServerNameIntegrationTest.java --- 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 #248: GEODE-1548: Specifying --J=-Dgemfire.jmx-...
Github user kirklund commented on a diff in the pull request: https://github.com/apache/incubator-geode/pull/248#discussion_r81402616 --- Diff: geode-core/src/test/java/org/apache/geode/management/internal/security/JavaRmiServerNameJunitTest.java --- @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.geode.management.internal.security; + +import static org.apache.geode.distributed.ConfigurationProperties.*; +import static org.junit.Assert.*; + +import java.util.Properties; + +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.cache.CacheFactory; +import org.apache.geode.internal.AvailablePort; +import org.apache.geode.test.junit.categories.UnitTest; + +@Category(UnitTest.class) --- End diff -- Since this test is testing more than one class (it's creating a Cache and using networking resources), it should have @Category(IntegrationTest.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 issue #224: GEODE-1648: Introduce security-enabled-component...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/224 1) we need to fix the javadocs for security-enabled-components on DistributedSystem class 2) move SecurableComponent enum into internal.security pkg 3) consider collapsing SecurableComponents interface into SecurableComponent enum (we don't really need both --- 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 issue #218: GEODE-1682: Adding options for starting Geode RE...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/218 Hi @davinash, there was one test failure in precheckin that needs to be fixed: :geode-core:integrationTest com.gemstone.gemfire.management.internal.cli.commands.HelpCommandsIntegrationTest > testOfflineHelp FAILED org.junit.ComparisonFailure: expected:<...iguration(=value)?] [PARAMETERS <...snip...> use-cluster-configuration When set to true, the server requests the configuration from locator's cluster configuration service. Required: false Default (if the parameter is specified without value): true Default (if the parameter is not specified): true start-rest-api When set to true, will start the REST API service. Required: false Default (if the parameter is specified without value): true Default (if the parameter is not specified): false http-service-port Port on which HTTP Service will listen on Required: false Default (if the parameter is not specified): 8080 http-service-bind-address The IP address on which the HTTP Service will be bound. By default, the Server is bound to all local addresses. Required: fals]e> at org.junit.Assert.assertEquals(Assert.java:115) at org.junit.Assert.assertEquals(Assert.java:144) at com.gemstone.gemfire.management.internal.cli.commands.HelpCommandsIntegrationTest.testOfflineHelp(HelpCommandsIntegrationTest.java:100) 3229 tests completed, 1 failed, 175 skipped :geode-core:integrationTest FAILED --- 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 issue #218: GEODE-1682: Adding options for starting Geode RE...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/218 I'll merge it to develop, run tests and commit it if everything passes -- the commit will automatically close the PR and you'll receive a notification. Thanks Avinash! --- 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 issue #218: GEODE-1682: Adding options for starting Geode RE...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/218 Thank you for the pull request Avinash! Please look into writing some new tests that verify the changes. --- 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 issue #210: GEODE-1647: Add Integrated Security to Peer Auth...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/210 The changes in this PR are incompatible with the changes Jinmei checked in on develop. We'll have to rewrite GMSAuthenticator and its 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 issue #201: GEODE-1617: Regions can be created with a variet...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/201 I'm running this change through precheckin. --- 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 issue #193: GEODE-1646: repackage new Security classes in or...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/193 AnalyzeSerializablesJUnitTest.testSerializables fails but it's easy to fix. I'll include a change to the expected serializables so it passes with this change. --- 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 issue #195: GEODE-1598: fix auto-completion problems
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/195 JoptOptionParserTest.parseInputWithUndefinedOptionShouldThrow fails with this change but I have a fix for it and will commit it with this change. --- 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 issue #196: GEODE-1617: Regions can be created with a variet...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/196 Precheckin has lots of failures including in pivotal smoketests: https://jenkins.eng.pivotal.io/jenkins/job/GEM-patch-build-closed/342/#showFailuresLink --- 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 issue #193: GEODE-1646: repackage new Security classes in or...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/193 +1 Running tests on this PR. --- 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 issue #186: gfsh unable to destroy region that has an hyphen...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/186 +1 --- 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 issue #169: GEODE-11: Added findKeys, findValues and findRes...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/169 Changes look great! --- 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 issue #161: GEODE-1549: Correct "Help" hyperlink in pulse
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/161 @sboorlagadda Yes, I missed that. Thanks Sai! @smanvi-pivotal can you please change the links to point at corresponding pages under http://geode.docs.pivotal.io/docs/tools_modules/pulse/chapter_overview.html? Thanks! --- 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 issue #161: GEODE-1549: Correct "Help" hyperlink in pulse
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/161 Changes look good. I'll pull this in to commit! --- 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 issue #157: GFSH SYS_HOST_NAME variable should report hostna...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/157 Working on pulling this in now. --- 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 issue #155: GEODE-1521 - APP_FETCH_SIZE in GFSH should not b...
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/155 I've pulled this in and I'm running precheckin on it. --- 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 issue #154: GEODE-1470: Upgrade log4j to 2.6
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/154 2.6.1 was released on June 5th, so we should upgrade to 2.6.1 instead of 2.6. The following tests fail consistently with both 2.6 and 2.6.1: * AnalyzeSerializablesJUnitTest.testDataSerializables * LogWriterAppenderJUnitTest.testAppenderToConfigHandling * LogWriterAppenderJUnitTest.testLogWriterLevels --- 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 issue #154: GEODE-1470: Upgrade log4j to 2.6
Github user kirklund commented on the issue: https://github.com/apache/incubator-geode/pull/154 This change looks good. I'll handle this pull request and determine why Travis failed. --- 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: GEODE-866: Converting calls of vm.in...
Github user kirklund commented on the pull request: https://github.com/apache/incubator-geode/pull/98#issuecomment-185976314 +1 changes look great --- 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: GEODE-769 Update 3 issues in README....
Github user kirklund commented on the pull request: https://github.com/apache/incubator-geode/pull/69#issuecomment-169850963 I'll pull this in to commit it. --- 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: GEODE-401: upgrade log4j from 2.1 to...
Github user kirklund commented on the pull request: https://github.com/apache/incubator-geode/pull/67#issuecomment-169412679 I'll go ahead and pull in these changes to test and commit. --- 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. ---
