[GitHub] geode pull request #732: GEODE-3276: Managing race conditions while the send...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/732#discussion_r134622619
--- Diff:
geode-wan/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderImpl.java
---
@@ -107,6 +107,9 @@ public void stop() {
if (ev != null && !ev.isStopped()) {
ev.stopProcessing();
}
+ if (ev != null && ev.getDispatcher() != null) {
--- End diff --
Is it possible to pull this check and shutdown into a method? Looks like
it's used a few times throughout the code
---
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] geode pull request #718: GEODE-3434: Allow old tomcat server sessions to be ...
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/718 --- 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] geode pull request #718: GEODE-3434: Allow old tomcat server sessions to be ...
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/718 GEODE-3434: Allow old tomcat server sessions to be compatible with new tomcat session modules Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to dev@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-3434 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/718.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 #718 commit 08c29c8df14b8cc315551297477636237e855b48 Author: Jason Huynh Date: 2017-08-14T16:02:11Z GEODE-3434: Allow the modules to be interoperable with current and older versions of tomcat 7 Modified DeltaSessions to use reflection to handle attributes fields incase an earlier tomcat 7 is used Modified DeltaSession7 and DeltaSession8 to extend from DeltaSession Added session backward compatibility tests Modified aseembly build to download old product installs commit dfce284c2634ea72e797ad4a9d8c956b8d86211c Author: Jason Huynh Date: 2017-08-14T19:07:08Z GEODE-3434: Modifications based on review comments commit be305896685e1b7d4d1f37724f673701277e5a76 Author: Jason Huynh Date: 2017-08-15T18:46:16Z Removed unused import commit d282ec4470a3993f7b401675bf7eb4161e1fb66e Author: Jason Huynh Date: 2017-08-17T22:04:42Z updated --- 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] geode pull request #712: GEODE-3434: Allow the modules to be interoperable w...
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/712 --- 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] geode pull request #712: GEODE-3434: Allow the modules to be interoperable w...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/712#discussion_r133843663
--- Diff: geode-old-versions/build.gradle ---
@@ -65,6 +92,21 @@ task createGeodeClasspathsFile {
new FileOutputStream(classpathsFile).withStream { fos ->
versions.store(fos, '')
}
+
+installsFile.getParentFile().mkdirs();
+
+new FileOutputStream(installsFile).withStream { fos ->
+ project.ext.installs.store(fos, '')
+}
}
+
+ // Add sourceSets for backwards compatibility, rolling upgrade, and
+ // pdx testing.
+ addOldVersion('test100', '1.0.0-incubating')
+ addOldVersion('test110', '1.1.0')
+ addOldVersion('test111', '1.1.1')
--- End diff --
Good idea, I'll add a boolean to decide whether to do the product install
or not
---
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] geode pull request #712: GEODE-3434: Allow the modules to be interoperable w...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/712#discussion_r133843603
--- Diff:
extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
---
@@ -553,8 +555,30 @@ public void fromData(DataInput in) throws IOException,
ClassNotFoundException {
}
}
- protected Map readInAttributes(final DataInput in) throws IOException,
ClassNotFoundException {
-return DataSerializer.readObject(in);
+ private void readInAttributes(DataInput in) throws IOException,
ClassNotFoundException {
+Map map = DataSerializer.readObject(in);
+ConcurrentMap newMap = new ConcurrentHashMap();
+newMap.putAll(map);
+try {
+ Field field = getAttributesFieldObject();
+ field.setAccessible(true);
+ field.set(this, newMap);
+} catch (NoSuchFieldException e) {
+ logError(e);
--- End diff --
I'll throw NoSuchElementException and IllegalStateException if any of these
occur
---
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] geode pull request #712: GEODE-3434: Allow the modules to be interoperable w...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/712#discussion_r133843558
--- Diff:
extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
---
@@ -586,6 +610,20 @@ public int getSizeInBytes() {
return serializedAttributes;
}
+ protected ConcurrentMap getAttributes() {
+try {
+ Field field = getAttributesFieldObject();
+ field.setAccessible(true);
+ Map oldMap = (Map) field.get(this);
+ ConcurrentMap newMap = new ConcurrentHashMap();
--- End diff --
I think you are correct, I believe we wrapped it incase somehow it was not
a concurrent hash map but I guess that was being overly cautious. I'll change
them all to map
---
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] geode pull request #712: GEODE-3434: Allow the modules to be interoperable w...
Github user jhuynh1 commented on a diff in the pull request: https://github.com/apache/geode/pull/712#discussion_r133270585 --- Diff: geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneEventListener.java --- @@ -34,7 +34,6 @@ import org.apache.geode.cache.query.internal.DefaultQuery; import org.apache.geode.internal.cache.BucketNotFoundException; import org.apache.geode.internal.cache.PrimaryBucketException; -import org.apache.geode.internal.cache.tier.sockets.command.Default; --- End diff -- I reverted the first set of changes to this file as it didn't have anything to do with this change set. It shouldn't be showing up in this diff anymore but if it is I'll make sure to revert 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] geode pull request #712: GEODE-3434: Allow the modules to be interoperable w...
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/712 GEODE-3434: Allow the modules to be interoperable with current and ol⦠â¦der versions of tomcat 7 Modified DeltaSessions to use reflection to handle attributes fields incase an earlier tomcat 7 is used Modified DeltaSession7 and DeltaSession8 to extend from DeltaSession Added session backward compatibility tests Modified aseembly build to download old product installs Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to dev@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-3434 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/712.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 #712 commit e9ed8a12f6f8cb9dfeedcec5922069bce2e7b7e3 Author: Jason Huynh Date: 2017-08-14T16:02:11Z GEODE-3434: Allow the modules to be interoperable with current and older versions of tomcat 7 Modified DeltaSessions to use reflection to handle attributes fields incase an earlier tomcat 7 is used Modified DeltaSession7 and DeltaSession8 to extend from DeltaSession Added session backward compatibility tests Modified aseembly build to download old product installs --- 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] geode issue #688: GEODE-3397: Fixed issue with Tomcat locators in cache-clie...
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/688 Looks ok to me...I'll merge this in. --- 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] geode pull request #609: GEODE-2886 : sent IllegalStateException instead of ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/609#discussion_r132497282
--- Diff:
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneQueriesIntegrationTest.java
---
@@ -331,6 +331,29 @@ public void queryJsonObject() throws Exception {
}
@Test()
+ public void waitUntilFlushThrowsIllegalStateExceptionWhenAEQNotFound()
throws Exception {
+Map fields = new HashMap<>();
+fields.put("name", null);
+fields.put("lastName", null);
+fields.put("address", null);
+
luceneService.createIndexFactory().setFields(fields).create(INDEX_NAME,
REGION_NAME);
+Region region =
cache.createRegionFactory(RegionShortcut.PARTITION).create(REGION_NAME);
+final LuceneIndex index = luceneService.getIndex(INDEX_NAME,
REGION_NAME);
+
+// This is to send IllegalStateException from WaitUntilFlushedFunction
+String nonCreatedIndex = "index2";
+boolean result = false;
+try {
+ result = luceneService.waitUntilFlushed(nonCreatedIndex,
REGION_NAME, 6,
+ TimeUnit.MILLISECONDS);
--- End diff --
I think there should be a fail() here,after the all to waitUntilFlushed, if
we are expecting an exception to be thrown and it doesn't get thrown.
---
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] geode pull request #609: GEODE-2886 : sent IllegalStateException instead of ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/609#discussion_r130952081
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/distributed/WaitUntilFlushedFunction.java
---
@@ -85,8 +72,10 @@ public void execute(FunctionContext context) {
}
} else {
- throw new IllegalArgumentException(
+ IllegalStateException illegalStateException = new
IllegalStateException(
"The AEQ does not exist for the index " + indexName + " region "
+ region.getFullPath());
+ logger.error(illegalStateException.getMessage());
--- End diff --
I still think we should just throw the exception here. The executing side
will either get a true or false anyways and the exception never actually gets
propagated to the user.
---
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] geode pull request #659: GEODE-3308: Lucene rolling upgrade and backwards co...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/659#discussion_r130765375
--- Diff:
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneSearchWithRollingUpgradeDUnit.java
---
@@ -0,0 +1,1044 @@
+/*
+ * 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.cache.lucene;
+
+import static org.apache.geode.test.dunit.Assert.fail;
+import static org.junit.Assert.assertEquals;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Properties;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.awaitility.Awaitility;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.geode.cache.GemFireCache;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.ClientRegionFactory;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.cache30.CacheSerializableRunnable;
+import org.apache.geode.distributed.Locator;
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.internal.Version;
+import org.apache.geode.internal.cache.GemFireCacheImpl;
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.test.dunit.DistributedTestUtils;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.IgnoredException;
+import org.apache.geode.test.dunit.Invoke;
+import org.apache.geode.test.dunit.NetworkUtils;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.internal.JUnit4DistributedTestCase;
+import org.apache.geode.test.dunit.standalone.DUnitLauncher;
+import org.apache.geode.test.dunit.standalone.VersionManager;
+import org.apache.geode.test.junit.categories.BackwardCompatibilityTest;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import
org.apache.geode.test.junit.runners.CategoryWithParameterizedRunnerFactory;
+
+@Category({DistributedTest.class, BackwardCompatibilityTest.class})
+@RunWith(Parameterized.class)
+@Parameterized.UseParametersRunnerFactory(CategoryWithParameterizedRunnerFactory.class)
+public class LuceneSearchWithRollingUpgradeDUnit extends
JUnit4DistributedTestCase {
+
+
+ @Parameterized.Parameters
+ public static Collection data() {
+List result =
VersionManager.getInstance().getVersionsWithoutCurrent();
+// Lucene Compatibility checks start with Apache Geode v1.2.0
+// Removing the versions older than v1.2.0
+result.removeIf(s -> Integer.parseInt(s) < 120);
+if (result.size() < 1) {
+ throw new RuntimeException("No older versions of Geode were found to
test against");
+} else {
+ System.out.println("running against these versions: " + result);
+}
+return result;
+ }
+
+ private File[] testingDirs = new File[3];
+
+ private static String INDEX_NAME = "index";
+
+ private static String diskDir = "LuceneSearchWithRollingUpgradeDUnit";
+
+ // Each vm will have a cache object
+ private static Object cache;
+
+ // the old version of Geode we're testing against
+ private String oldVersion;
+
+ private void deleteVMFiles() throws Exception {
+System.out.println(&q
[GitHub] geode pull request #659: GEODE-3308: Lucene rolling upgrade and backwards co...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/659#discussion_r130765402
--- Diff:
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneSearchWithRollingUpgradeDUnit.java
---
@@ -0,0 +1,1044 @@
+/*
+ * 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.cache.lucene;
+
+import static org.apache.geode.test.dunit.Assert.fail;
+import static org.junit.Assert.assertEquals;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Properties;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.awaitility.Awaitility;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.geode.cache.GemFireCache;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.ClientRegionFactory;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.cache30.CacheSerializableRunnable;
+import org.apache.geode.distributed.Locator;
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.internal.Version;
+import org.apache.geode.internal.cache.GemFireCacheImpl;
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.test.dunit.DistributedTestUtils;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.IgnoredException;
+import org.apache.geode.test.dunit.Invoke;
+import org.apache.geode.test.dunit.NetworkUtils;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.internal.JUnit4DistributedTestCase;
+import org.apache.geode.test.dunit.standalone.DUnitLauncher;
+import org.apache.geode.test.dunit.standalone.VersionManager;
+import org.apache.geode.test.junit.categories.BackwardCompatibilityTest;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import
org.apache.geode.test.junit.runners.CategoryWithParameterizedRunnerFactory;
+
+@Category({DistributedTest.class, BackwardCompatibilityTest.class})
+@RunWith(Parameterized.class)
+@Parameterized.UseParametersRunnerFactory(CategoryWithParameterizedRunnerFactory.class)
+public class LuceneSearchWithRollingUpgradeDUnit extends
JUnit4DistributedTestCase {
+
+
+ @Parameterized.Parameters
+ public static Collection data() {
+List result =
VersionManager.getInstance().getVersionsWithoutCurrent();
+// Lucene Compatibility checks start with Apache Geode v1.2.0
+// Removing the versions older than v1.2.0
+result.removeIf(s -> Integer.parseInt(s) < 120);
+if (result.size() < 1) {
+ throw new RuntimeException("No older versions of Geode were found to
test against");
+} else {
+ System.out.println("running against these versions: " + result);
+}
+return result;
+ }
+
+ private File[] testingDirs = new File[3];
+
+ private static String INDEX_NAME = "index";
+
+ private static String diskDir = "LuceneSearchWithRollingUpgradeDUnit";
+
+ // Each vm will have a cache object
+ private static Object cache;
+
+ // the old version of Geode we're testing against
+ private String oldVersion;
+
+ private void deleteVMFiles() throws Exception {
+System.out.println(&q
[GitHub] geode pull request #609: GEODE-2886 : sent IllegalStateException instead of ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/609#discussion_r129624935
--- Diff:
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneQueriesIntegrationTest.java
---
@@ -290,6 +295,24 @@ public void queryJsonObject() throws Exception {
}
@Test()
+ public void testWaitUntilFlushedForException() throws Exception {
+Map fields = new HashMap();
+fields.put("name", null);
+fields.put("lastName", null);
+fields.put("address", null);
+
luceneService.createIndexFactory().setFields(fields).create(INDEX_NAME,
REGION_NAME);
+Region region =
cache.createRegionFactory(RegionShortcut.PARTITION).create(REGION_NAME);
+final LuceneIndex index = luceneService.getIndex(INDEX_NAME,
REGION_NAME);
+
+// This is to send IllegalStateException from WaitUntilFlushedFunction
+String nonCreatedIndex = "index2";
+
+boolean b =
+luceneService.waitUntilFlushed(nonCreatedIndex, REGION_NAME,
6, TimeUnit.MILLISECONDS);
+assertFalse(b);
--- End diff --
So the end result would be the wait for flush function would return false
(it did not flush) and a message would be logged on the server side. Based on
the proposed fix of returning the exception, we would have been doing something
similar anyways. The client would have seen a false (because the function
handled the exception and returned false)
So I guess having the test check for false would be just fine after all :-)
---
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] geode pull request #609: GEODE-2886 : sent IllegalStateException instead of ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/609#discussion_r129418949
--- Diff:
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneQueriesIntegrationTest.java
---
@@ -290,6 +295,24 @@ public void queryJsonObject() throws Exception {
}
@Test()
+ public void testWaitUntilFlushedForException() throws Exception {
+Map fields = new HashMap();
+fields.put("name", null);
+fields.put("lastName", null);
+fields.put("address", null);
+
luceneService.createIndexFactory().setFields(fields).create(INDEX_NAME,
REGION_NAME);
+Region region =
cache.createRegionFactory(RegionShortcut.PARTITION).create(REGION_NAME);
+final LuceneIndex index = luceneService.getIndex(INDEX_NAME,
REGION_NAME);
+
+// This is to send IllegalStateException from WaitUntilFlushedFunction
+String nonCreatedIndex = "index2";
+
+boolean b =
+luceneService.waitUntilFlushed(nonCreatedIndex, REGION_NAME,
6, TimeUnit.MILLISECONDS);
+assertFalse(b);
--- End diff --
@ameybarve15
I was under the assumption that the client currently is being passed the
IllegalArgumentException and instead we would just throw the
IllegalStateException and piggy back off of how FunctionExecution handles
exceptions. When a function fails during function execution, I would think an
exception is passed back to the user?
---
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] geode pull request #609: GEODE-2886 : sent IllegalStateException instead of ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/609#discussion_r128584677
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneServiceImpl.java
---
@@ -472,12 +478,20 @@ public boolean waitUntilFlushed(String indexName,
String regionPath, long timeou
new WaitUntilFlushedFunctionContext(indexName, timeout, unit);
Execution execution = FunctionService.onRegion(dataRegion);
ResultCollector rs =
execution.setArguments(context).execute(WaitUntilFlushedFunction.ID);
-List results = (List) rs.getResult();
-for (Boolean oneResult : results) {
- if (oneResult == false) {
+List results = (List) rs.getResult();
+if (results != null) {
+ if (results.get(0) instanceof IllegalStateException) {
--- End diff --
Instead of checking types and silently handling this exception, it's
probably better to just throw it and not have to modify this portion of code.
---
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] geode pull request #609: GEODE-2886 : sent IllegalStateException instead of ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/609#discussion_r128585366
--- Diff:
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneQueriesIntegrationTest.java
---
@@ -290,6 +295,24 @@ public void queryJsonObject() throws Exception {
}
@Test()
+ public void testWaitUntilFlushedForException() throws Exception {
--- End diff --
Perhaps rename this be a bit more descriptive so a test maintainer can know
what the test was expected to do when pass/failing.
Something like: waitUntilFlushThrowsIllegalStateExceptionWhenAEQNotFound
---
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] geode pull request #609: GEODE-2886 : sent IllegalStateException instead of ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/609#discussion_r128586252
--- Diff:
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneQueriesIntegrationTest.java
---
@@ -290,6 +295,24 @@ public void queryJsonObject() throws Exception {
}
@Test()
+ public void testWaitUntilFlushedForException() throws Exception {
+Map fields = new HashMap();
+fields.put("name", null);
+fields.put("lastName", null);
+fields.put("address", null);
+
luceneService.createIndexFactory().setFields(fields).create(INDEX_NAME,
REGION_NAME);
+Region region =
cache.createRegionFactory(RegionShortcut.PARTITION).create(REGION_NAME);
+final LuceneIndex index = luceneService.getIndex(INDEX_NAME,
REGION_NAME);
+
+// This is to send IllegalStateException from WaitUntilFlushedFunction
+String nonCreatedIndex = "index2";
+
+boolean b =
+luceneService.waitUntilFlushed(nonCreatedIndex, REGION_NAME,
6, TimeUnit.MILLISECONDS);
+assertFalse(b);
--- End diff --
We should check to make sure the exception is thrown instead of wait for
flush returning false.
---
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] geode pull request #609: GEODE-2886 : sent IllegalStateException instead of ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/609#discussion_r128584345
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/distributed/WaitUntilFlushedFunction.java
---
@@ -85,8 +72,8 @@ public void execute(FunctionContext context) {
}
} else {
- throw new IllegalArgumentException(
- "The AEQ does not exist for the index " + indexName + " region "
+ region.getFullPath());
+ resultSender.sendException(new IllegalStateException(
--- End diff --
I think we should just throw the IllegalStateException, that way it will
propagate to the user and let them know that an AEQ does not exist for the
index.
---
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] geode pull request #589: GEODE-393: Providing cache for FunctionContext
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/589#discussion_r123830138
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
---
@@ -37,20 +38,25 @@
private String functionId = null;
+ private Cache cache = null;
+
private ResultSender resultSender = null;
private final boolean isPossDup;
public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+ }
+
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
}
- public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender,
- boolean isPossibleDuplicate) {
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --
Sure we can't remove GemFireCacheImpl.getInstance now and we might not be
able to ever... but in order for it to be possible, this change would need to
be made anyways if I am not mistaken... not making these constructor changes
will hinder the effort later. Since the work is already done, was there a
reason why we didn't want to make these signature changes? Is it too
cumbersome with the number of variables being passed in?
---
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] geode pull request #589: GEODE-393: Providing cache for FunctionContext
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/589#discussion_r123553740
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
---
@@ -37,20 +38,25 @@
private String functionId = null;
+ private Cache cache = null;
+
private ResultSender resultSender = null;
private final boolean isPossDup;
public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+ }
+
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
}
- public FunctionContextImpl(final String functionId, final Object args,
ResultSender resultSender,
- boolean isPossibleDuplicate) {
+ public FunctionContextImpl(final Cache cache, final String functionId,
final Object args,
+ ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --
I think the goal is to remove static calls altogether at some point. I
think some benefits would be it makes it easier to write test code for
functions (for customers) and our own functions. Client code could already use
GemFireCacheImpl.getInstance() but I think the goal was to remove this call one
day in the far future.
---
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] geode pull request #591: GEODE-3073: Renamed OrderByComparatorUnmapped to Or...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/591#discussion_r123388223
--- Diff:
geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSelect.java
---
@@ -925,79 +917,89 @@ private SelectResults
applyProjectionOnCollection(SelectResults resultSet,
private SelectResults prepareEmptyResultSet(ExecutionContext context,
boolean ignoreOrderBy)
throws TypeMismatchException, AmbiguousNameException {
-// if no projection attributes or '*'as projection attribute
-// & more than one/RunTimeIterator then create a StrcutSet.
-// If attribute is null or '*' & only one RuntimeIterator then create a
-// ResultSet.
-// If single attribute is present without alias name , then create
-// ResultSet
-// Else if more than on attribute or single attribute with alias is
-// present then return a StrcutSet
-// create StructSet which will contain root objects of all iterators in
-// from clause
+// If no projection attributes or '*' as projection attribute & more
than one/RunTimeIterator
+// then create a StructSet.
+// If attribute is null or '*' & only one RuntimeIterator then create
a ResultSet.
+// If single attribute is present without alias name, then create
ResultSet.
+// Else if more than on attribute or single attribute with alias is
present then return a
+// StructSet.
+// Create StructSet which will contain root objects of all iterators
in from clause.
ObjectType elementType = this.cachedElementTypeForOrderBy != null
? this.cachedElementTypeForOrderBy : prepareResultType(context);
SelectResults results;
-if (this.distinct || !this.count) {
- if (this.orderByAttrs != null) {
-boolean nullValuesAtStart = !((CompiledSortCriterion)
orderByAttrs.get(0)).getCriterion();
-if (elementType.isStructType()) {
- if (ignoreOrderBy) {
-results = this.distinct ? new LinkedStructSet((StructTypeImpl)
elementType)
-: new SortedResultsBag(elementType, nullValuesAtStart);
- } else {
-OrderByComparator comparator = this.hasUnmappedOrderByCols
-? new OrderByComparatorUnmapped(this.orderByAttrs,
(StructTypeImpl) elementType,
-context)
-: new OrderByComparator(this.orderByAttrs,
(StructTypeImpl) elementType, context);
-results = this.distinct ? new SortedStructSet(comparator,
(StructTypeImpl) elementType)
-: new SortedStructBag(comparator, (StructType)
elementType, nullValuesAtStart);
+if (!this.distinct && this.count) {
+ // Shobhit: If it's a 'COUNT' query and no End processing required
Like for 'DISTINCT'
+ // we can directly keep count in ResultSet and ResultBag is good
enough for that.
+ countStartQueryResult = 0;
+ return new ResultsBag(new ObjectTypeImpl(Integer.class), 1,
context.getCachePerfStats());
+}
- }
-} else {
- if (ignoreOrderBy) {
-results =
-this.distinct ? new LinkedResultSet() : new
SortedResultsBag(nullValuesAtStart);
+// Potential edge-case: Could this be non-null but empty?
+boolean nullValuesAtStart = orderByAttrs != null &&
!orderByAttrs.get(0).getCriterion();
+OrderByComparator comparator;
+switch (convertToStringCase(elementType, ignoreOrderBy)) {
+ case "UNORDERED DISTINCT STRUCT":
+return new StructSet((StructType) elementType);
+ case "UNORDERED DISTINCT RESULTS":
+return new ResultsSet(elementType);
+ case "UNORDERED INDISTINCT STRUCT":
+return new StructBag((StructType) elementType,
context.getCachePerfStats());
+ case "UNORDERED INDISTINCT RESULTS":
+return new ResultsBag(elementType, context.getCachePerfStats());
+
+ case "ORDERED DISTINCT STRUCT IGNORED":
+return new LinkedStructSet((StructTypeImpl) elementType);
+ case "ORDERED INDISTINCT STRUCT IGNORED":
+return new SortedResultsBag(elementType, nullValuesAtStart);
+ case "ORDERED DISTINCT STRUCT UNIGNORED":
--- End diff --
I think the ignoreOrderBy flag is a bit misleading, otherwise UNORDERED
DISTINCT STRUCT should technically be the same as ORDERED DISTINCT STRUCT
IGNORED (since ignoreOrderby is true). It's almost like a "sort at data
structure" level or delay the sort until later flag.
If you decide to keep the naming, would it
[GitHub] geode pull request #588: GEODE-2820: Added awaitlity clause to wait for the ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/588#discussion_r123060011
--- Diff:
geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryIndexUsingXMLDUnitTest.java
---
@@ -510,6 +512,16 @@ public void
testCreateAsyncIndexWhileDoingGIIAndCompareQueryResults() throws Exc
vm1.invoke(prIndexCreationCheck(PERSISTENT_REG_NAME, "secIndex", 50));
vm1.invoke(indexCreationCheck(REP_REG_NAME, "secIndex"));
+vm1.invoke(() -> Awaitility.await().atMost(60,
TimeUnit.SECONDS).until(() -> {
+ boolean regionSizeCheck = getCache().getRegion(NAME).size() == 500
+ && getCache().getRegion(REP_REG_NAME).size() == 500
--- End diff --
Shouldn't this clause check the index size and not the region size? I
would think the timing issue was because the indexes were async. Because of
the async, the indexes may not have processes all of the updates prior to
validating the contents of the index...
---
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] geode pull request #565: GEODE-3021: Any call after the first to setPdxStrin...
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/565 --- 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] geode pull request #565: GEODE-3021: Any call after the first to setPdxStrin...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/565#discussion_r121196596
--- Diff:
geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java
---
@@ -2002,7 +2002,8 @@ void populateListForEquiJoin(List list, Object
outerEntries, Object innerEntries
*/
synchronized void setPdxStringFlag(Object key) {
// For Null and Undefined keys do not set the isIndexedPdxKeysFlagSet
flag
-if (key == null || key == IndexManager.NULL || key ==
QueryService.UNDEFINED) {
+if (isIndexedPdxKeysFlagSet || key == null || key == IndexManager.NULL
--- End diff --
They are slightly different. The isIndexedPdxKeys is represents whether
the index is storing pdx as keys. The isIndexedPdxKeysFlagSet, is a boolean
that is used as a short circuit to only call the method once. I guess it was a
performance "enhancement" to not call the method over and over for every value
and just call it only for the first call.
---
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] geode issue #565: GEODE-3021: Any call after the first to setPdxStringFlag s...
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/565 @boglesby @upthewaterspout @gesterzhou Adding additional reviewers --- 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] geode pull request #565: GEODE-3021: Any call after the first to setPdxStrin...
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/565 GEODE-3021: Any call after the first to setPdxStringFlag should no-op * The flag isIndexedPdxKeysFlagSet is now checked before setting pdx string flag @nabarunnag @ladyVader Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [X] Is your initial contribution a single, squashed commit? - [X] Does `gradlew build` run cleanly? - [X] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to dev@geode.apache.org. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-3021 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/565.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 #565 commit c2943a4acb9c9325662a4cbee14823e0a1c05061 Author: Jason Huynh Date: 2017-06-01T20:52:41Z GEODE-3021: Any call after the first to setPdxStringFlag should no-op * The flag isIndexedPdxKeysFlagSet is now checked before setting pdx string flag --- 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] geode issue #558: GEODE-194: Remove spark connector
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/558 Looks good to me --- 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] geode issue #511: Feature/geode 269
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/511 Hi Deepak, Sorry, I've tried merging this into develop but had issues with the conflicts and then was unable to credit you with the checkin. Will you be able to recreate this pull request without the conflict? I can always apply the origin patch otherwise, but I wanted to make sure you got attributed with the 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] geode issue #509: GEODE-265: Removing deprecated API from Execution interfac...
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/509 @metatype Will do for both this and PR#511 --- 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] geode issue #509: GEODE-265: Removing deprecated API from Execution interfac...
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/509 I think these changes are ok (I didn't check when these were deprecated but I assume @metatype already did the grunt work on it. I can merge these changes in if no one has any other reviews --- 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] geode issue #511: Feature/geode 269
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/511 I still get a conflicting file but it was simple enough to change. It looks good to me. I'll merge this in when I get a chance (tomorrow?) --- 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] geode pull request #519: GEODE-2937: Restore removeFromQueueOnException
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/519 --- 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] geode pull request #519: GEODE-2937: Restore removeFromQueueOnException
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/519 GEODE-2937: Restore removeFromQueueOnException Restoring the system property REMOVE_FROM_QUEUE_ON_EXCEPTION. This was no-op'd in some refactoring code last year and should not have been. Review request for: @boglesby You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-2937 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/519.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 #519 --- 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] geode issue #517: GEODE-2587: Refactored the OrderByComparator's compare met...
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/517 We definitely have order by in the tests, whether or not they are adequate I am not sure. doing a find on "order by" will show a log of queries in the tests... The changes look good to me, if in the future we could collapse common code or extract some into smaller methods that would be 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] geode pull request #502: GEODE-2900: push shadow key to the front of eventSe...
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/502 --- 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] geode pull request #513: GEODE-2914: Tidy up LuceneService javadoc
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/513 --- 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] geode issue #505: [GEODE-2890]: Corrected debug log location in AbstractGate...
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/505 Yeah I think so. 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] geode pull request #513: GEODE-2914: Tidy up LuceneService javadoc
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/513 GEODE-2914: Tidy up LuceneService javadoc Potential reviewers: @karensmolermiller @nabarunnag @ladyVader Fixing typos and javadoc issues You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-2914 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/513.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 #513 commit 433e84e7a364228a1adbee1fdd7c0f7a7975ea8b Author: Jason Huynh Date: 2017-05-12T17:42:01Z GEODE-2914: Tidy up LuceneService javadoc --- 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] geode pull request #502: GEODE-2900: push shadow key to the front of eventSe...
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/502 GEODE-2900: push shadow key to the front of eventSeqNumQueue Testing still needs to be done on this change, but wanted to get some eyeballs on the change in parallel @upthewaterspout @boglesby You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-2900 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/502.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 #502 commit 240b469ff217fbfba39381f196ae3e0e832a69a6 Author: Jason Huynh Date: 2017-05-09T17:11:39Z GEODE-2900: push shadow key back into the front of the eventSeqNumber "Queue" --- 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] geode pull request #491: GEODE-2870: Local node function execution failure c...
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/491 --- 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] geode pull request #491: GEODE-2870: Local node function execution failure c...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/491#discussion_r114674904
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/execute/PartitionedRegionFunctionResultSender.java
---
@@ -214,16 +214,15 @@ public void lastResult(Object oneResult) {
private synchronized void lastResult(Object oneResult, ResultCollector
collector,
boolean lastRemoteResult, boolean lastLocalResult, DistributedMember
memberID) {
+
+boolean completedLocal = lastLocalResult |
this.localLastResultRecieved;
--- End diff --
Whoops good catch, I'll fix that one
---
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] geode pull request #491: GEODE-2870: Local node function execution failure c...
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/491 GEODE-2870: Local node function execution failure correctly returns e⦠â¦xception * Race condition and escaping synchronized block led to function possibly missing results * It was possible a remote node would enter the synchronized block after local node threw exception * Certain side effects would allow processing of remote node results to be considered last result * Local processing thread would be paused/non active and miss opportunity to write exception * This would manifest as incomplete results instead of a retry Reviewers: @upthewaterspout @nabarunnag @gesterzhou @boglesby @ladyVader You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-2870 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/491.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 #491 commit d7671df8788fa926967ef35a71b95b21ffd41726 Author: Jason Huynh Date: 2017-05-02T23:30:46Z GEODE-2870: Local node function execution failure correctly returns exception * Race condition and escaping synchronized block led to function possibly missing results * It was possible a remote node would enter the synchronized block after local node threw exception * Certain side effects would allow processing of remote node results to be considered last result * Local processing thread would be paused/non active and miss opportunity to write exception * This would manifest as incomplete results instead of a retry --- 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] geode pull request #480: GEODE-2825: Lucene query waits for defined index to...
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/480 --- 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] geode issue #480: GEODE-2825: Lucene query waits for defined index to be cre...
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/480 Sure, I can re-add the one second sleep. --- 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] geode pull request #481: GEODE-2828: AEQ being created before the user regio...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/481#discussion_r114195735
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/AbstractPartitionedRepositoryManager.java
---
@@ -111,6 +121,10 @@ protected IndexRepository computeRepository(Integer
bucketId) {
return repo;
}
+ protected void allowRepositoryComputation() {
--- End diff --
ah ok, makes more sense, I think my mind mistook this method name with
allowing the repo to do computation rather than allowing computing of repository
---
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] geode pull request #481: GEODE-2828: AEQ being created before the user regio...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/481#discussion_r114161620
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/AbstractPartitionedRepositoryManager.java
---
@@ -111,6 +121,10 @@ protected IndexRepository computeRepository(Integer
bucketId) {
return repo;
}
+ protected void allowRepositoryComputation() {
--- End diff --
Nitpick isn't this representing that the repository is ready for use?
Should rename the latch and method to reflect this, such as readyForUse and
enableRepositoryForUse?
---
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] geode pull request #481: GEODE-2828: AEQ being created before the user regio...
Github user jhuynh1 commented on a diff in the pull request: https://github.com/apache/geode/pull/481#discussion_r114160633 --- Diff: geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/AbstractPartitionedRepositoryManager.java --- @@ -47,18 +48,22 @@ new ConcurrentHashMap(); /** The user region for this index */ - protected final PartitionedRegion userRegion; + protected PartitionedRegion userRegion = null; protected final LuceneSerializer serializer; protected final LuceneIndexImpl index; protected volatile boolean closed; + private CountDownLatch isDataRegionReady = new CountDownLatch(1); --- End diff -- Should this be final? --- 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] geode pull request #479: GEODE-2828: AEQ created before the user region
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/479#discussion_r113565230
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneServiceImpl.java
---
@@ -166,28 +166,28 @@ public void createIndex(final String indexName,
String regionPath, final Analyze
*
* Public because this is called by the Xml parsing code
*/
- public void afterDataRegionCreated(final String indexName, final
Analyzer analyzer,
- final String dataRegionPath, final Map
fieldAnalyzers,
- final String... fields) {
-LuceneIndexImpl index = createIndexRegions(indexName, dataRegionPath);
-index.setSearchableFields(fields);
-index.setAnalyzer(analyzer);
-index.setFieldAnalyzers(fieldAnalyzers);
+ public void afterDataRegionCreated(LuceneIndexImpl index) {
index.initialize();
registerIndex(index);
if (this.managementListener != null) {
this.managementListener.afterIndexCreated(index);
}
+
+ }
+
+ public LuceneIndexImpl beforeDataRegionCreated(final String indexName,
final String regionPath,
+ RegionAttributes attributes, final Analyzer analyzer,
+ final Map fieldAnalyzers, String aeqId, final
String... fields) {
+LuceneIndexImpl index = createIndexRegions(indexName, regionPath);
--- End diff --
This wasn't introduced with this diff, but we probably want to rename this
method. I don't think the method is creating any index regions. I think it's
just creating the index object but none of the file/chunk or aeq region or
buckets
---
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] geode pull request #480: GEODE-2825: Lucene query waits for defined index to...
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/480 GEODE-2825: Lucene query waits for defined index to be created * If an index is in a defined state but not yet created, the query will now wait until the index is created or no longer defined. Instead of throwing an exception and possibly getting a stack overflow Review request list: @upthewaterspout @nabarunnag @boglesby @ladyVader @gesterzhou You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-2825 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/480.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 #480 commit fb01b37ce8b7acda184daabc40de969a15c933c4 Author: Jason Huynh Date: 2017-04-26T18:13:53Z GEODE-2825: Lucene query waits for defined index to be created * If an index is in a defined state but not yet created, the query will now wait until the index is created or no longer defined. Instead of throwing an exception and possibly getting a stack overflow --- 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] geode pull request #460: Feature/geode 2703
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/460 --- 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] geode pull request #460: Feature/geode 2703
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/460 Feature/geode 2703 Modified transaction exception messaging when executing a lucene query within a geode transaction. The client transactions behave differently in geode depending on whether single hop is enabled or not. If single hop is enabled, the expected transaction exception actually never is thrown. Possible reviewers : @nabarunnag @upthewaterspout @boglesby You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-2703 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/460.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 #460 commit e7b57b679b1be837f943c34e1d7022b5aa8de583 Author: Jason Huynh Date: 2017-04-12T20:54:39Z GEODE-2703: Improve exception message when executing lucene within a transaction * Throw a LuceneQueryException instead of a TransactionException commit 9a93b7acd31d07eeb97af93dc12c817e64f210c2 Author: Jason Huynh Date: 2017-04-12T21:42:52Z Modified Test commit f408ccad86418626009bf1d0b306b98a84c20b03 Author: Jason Huynh Date: 2017-04-14T16:16:21Z Added catch when executing from client without singlehop commit 464263967d4a4352760e4d88dddfa734e3cf Author: Jason Huynh Date: 2017-04-14T22:50:15Z Changes commit 1bd2b86eecbedfcddb56e2298e49fb14a2cceaf3 Author: Jason Huynh Date: 2017-04-18T16:22:03Z Modified test for client transactions with Lucene queries --- 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] geode pull request #451: GEODE-2768: Lucene Queries executed before index is...
Github user jhuynh1 closed the pull request at: https://github.com/apache/geode/pull/451 --- 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] geode pull request #449: GEODE-2764: Added checks on the region name
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/449#discussion_r111591894
--- Diff:
geode-wan/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigurationIndexWithFromClauseDUnitTest.java
---
@@ -0,0 +1,120 @@
+/*
+ * 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.configuration;
+
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.CommandResult;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.test.dunit.rules.GfshShellConnectionRule;
+import org.apache.geode.test.dunit.rules.LocatorServerStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import java.util.Properties;
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+
+@Category(DistributedTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class ClusterConfigurationIndexWithFromClauseDUnitTest {
+
+ final String REGION_NAME = "region";
+ final String INDEX_NAME = "index";
+
+ protected RegionShortcut[] getPartitionRegionTypes() {
+return new RegionShortcut[] {RegionShortcut.PARTITION,
RegionShortcut.PARTITION_PERSISTENT,
+RegionShortcut.PARTITION_REDUNDANT,
RegionShortcut.PARTITION_REDUNDANT_PERSISTENT,
+RegionShortcut.REPLICATE, RegionShortcut.REPLICATE_PERSISTENT};
+
+ }
+
+ @Rule
+ public LocatorServerStartupRule lsRule = new LocatorServerStartupRule();
+
+ @Rule
+ public GfshShellConnectionRule gfshShellConnectionRule = new
GfshShellConnectionRule();
+
+ private MemberVM locator = null;
+
+ @Before
+ public void before() throws Exception {
+locator = lsRule.startLocatorVM(0);
+ }
+
+ @Test
+ @Parameters(method = "getPartitionRegionTypes")
+ public void indexCreatedWithFromClauseMustPersist(RegionShortcut
regionShortcut)
--- End diff --
maybe change this name to describing the .entrySet() that this was trying
to solve?
indexCreatedWithEntrySetInFromClauseMustPersist?
---
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] geode pull request #451: GEODE-2768: Lucene Queries executed before index is...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/451#discussion_r111275346
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/distributed/LuceneQueryFunction.java
---
@@ -132,6 +133,25 @@ private LuceneIndexImpl getLuceneIndex(final Region
region,
try {
index =
(LuceneIndexImpl) service.getIndex(searchContext.getIndexName(),
region.getFullPath());
+ if (index == null && service instanceof LuceneServiceImpl) {
+if (((LuceneServiceImpl)
service).getDefinedIndex(searchContext.getIndexName(),
+region.getFullPath()) != null) {
+ // The node may be in the process of recovering, where we have
the index defined but yet
+ // to be recovered
+ // If we retry fast enough, we could get a stack overflow based
on the way function
+ // execution is currently written
+ // Instead we will add an artificial sleep to slow down the
retry at this point
+ // Hopefully in the future, the function execution would retry
without adding to the stack
+ // and this can be removed
+ try {
--- End diff --
This is really an artificial time that was to avoid the stack overflow from
the function execution retry. The smaller the time, the greater chance we hit
the stack overflow but also the quicker we return to the user if the index
really doesn't exist. I can understand maybe bumping this up to a second but
10 seems a bit long...
---
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] geode issue #451: GEODE-2768: Lucene Queries executed before index is fully ...
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/451 Tagging potential reviewers @nabarunnag @boglesby @ladyVader @gesterzhou --- 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] geode pull request #451: GEODE-2768: Lucene Queries executed before index is...
GitHub user jhuynh1 opened a pull request: https://github.com/apache/geode/pull/451 GEODE-2768: Lucene Queries executed before index is fully created sho⦠â¦uld be retried * Added a sleep to prevent rapid retries which lead to stack overflow * Sleep can be removed when Function Execution retry does not add to stack You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/geode feature/GEODE-2768 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/geode/pull/451.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 #451 commit 60f5f06f18f8ea6c1d176a43a25bfbb4086e2f21 Author: Jason Huynh Date: 2017-04-12T18:39:28Z GEODE-2768: Lucene Queries executed before index is fully created should be retried * Added a sleep to prevent rapid retries which lead to stack overflow * Sleep can be removed when Function Execution retry does not add to stack --- 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] geode issue #447: Feature/geode 2217
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/447 I pulled the changes and it built fine. I collapsed and edited the commit messages to match how geode commits generally look and made a minor tweak to the wording on one parameter. This is now in the develop branch of Geode. Thanks (again) for the contribution! --- 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] geode issue #321: [GEODE-1577] Unhelpful generic types on Execution.execute
Github user jhuynh1 commented on the issue: https://github.com/apache/geode/pull/321 I've pulled these changes in. Had to resolve some conflicts but these changes should be in geode 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] geode pull request #449: GEODE-2764: Added checks on the region name
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/449#discussion_r111036455
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/CreateIndexFunction.java
---
@@ -93,6 +91,31 @@ public void execute(FunctionContext context) {
}
}
+ private void setResultInSender(FunctionContext context, IndexInfo
indexInfo, String memberId,
+ Cache cache, String regionPath) {
+if (regionPath == null) {
+ String message =
CliStrings.format(CliStrings.CREATE_INDEX__INVALID__REGIONPATH,
+ indexInfo.getRegionPath());
+ context.getResultSender().lastResult(new CliFunctionResult(memberId,
false, message));
+} else {
+ XmlEntity xmlEntity =
+ new XmlEntity(CacheXml.REGION, "name",
cache.getRegion(regionPath).getName());
+ context.getResultSender().lastResult(new CliFunctionResult(memberId,
xmlEntity));
+}
+ }
+
+ private String getValidRegionName(Cache cache, String regionPath) {
+while (cache.getRegion(regionPath) == null && regionPath != null) {
--- End diff --
If regionPath is null, will cache.getRegion(regionPath) throw an exception?
Shouldn't the regionPath check for null be first?
---
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] geode pull request #448: GEODE-2745: WaitUntilBucketRegionQueueFlushedCallab...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/448#discussion_r111011735
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegionQueue.java
---
@@ -464,16 +464,21 @@ private void setLatestAcknowledgedKey(Long key) {
this.latestAcknowledgedKey.set(key);
}
- public boolean waitUntilFlushed(long timeout, TimeUnit unit) throws
InterruptedException {
+ public long getLatestQueuedKey() {
+return this.latestQueuedKey.get();
+ }
+
+ public boolean waitUntilFlushed(long latestQueuedKey, long timeout,
TimeUnit unit)
+ throws InterruptedException {
long then = System.currentTimeMillis();
if (logger.isDebugEnabled()) {
- logger.debug("BucketRegionQueue: waitUntilFlushed bucket=" + getId()
+ "; timeout=" + timeout
- + "; unit=" + unit);
+ logger.debug("BucketRegionQueue: waitUntilFlushed bucket=" + getId()
+ "; latestQueuedKey="
+ + latestQueuedKey + "; timeout=" + timeout + "; unit=" + unit);
}
boolean result = false;
// Wait until latestAcknowledgedKey > latestQueuedKey or the queue is
empty
if (this.initialized) {
- long latestQueuedKeyToCheck = this.latestQueuedKey.get();
+ long latestQueuedKeyToCheck = latestQueuedKey;
--- End diff --
Not an actual problem, but we probably could just replace this variable
altogether and only use latestQueueKey instead.
---
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] geode pull request #444: Feature/gem 1353
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110718931
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java ---
@@ -1261,7 +1333,14 @@ void basicUpdateEntryVersion(EntryEventImpl event)
throws EntryNotFoundException
}
protected void distributeUpdateEntryVersionOperation(EntryEventImpl
event) {
-new UpdateEntryVersionOperation(event).distribute();
+UpdateEntryVersionOperation op = new
UpdateEntryVersionOperation(event);
+long viewVersion = -1;
+try {
+ viewVersion = op.startOperation();
--- End diff --
Out of curiosity, what happens if the startOperation() method returns a -1
(I think that may be possible based on how it's currently written or if an
exception is thrown before token is assigned. What is the expected way to
handle endOperation with an invalid/-1 token?
---
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] geode pull request #444: Feature/gem 1353
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110719505
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
---
@@ -240,12 +240,37 @@ public boolean containsRegionContentChange() {
return true;
}
+ public long startOperation() {
+DistributedRegion region = getRegion();
+long viewVersion = -1;
+if (this.containsRegionContentChange()) {
+ viewVersion = region.getDistributionAdvisor().startOperation();
+}
+if (logger.isTraceEnabled(LogMarker.STATE_FLUSH_OP)) {
+ logger.trace(LogMarker.STATE_FLUSH_OP, "dispatching operation in
view version {}",
+ viewVersion);
+}
+return viewVersion;
+ }
+
+ public void endOperation(long viewVersion) {
+DistributedRegion region = getRegion();
+if (viewVersion != -1) {
--- End diff --
If somehow we get a -1, will the state flush ever "end?" or are we stuck?
---
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] geode pull request #444: Feature/gem 1353
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/444#discussion_r110702026
--- Diff:
geode-cq/src/test/java/org/apache/geode/cache/query/cq/dunit/CqQueryDUnitTest.java
---
@@ -1577,7 +1577,13 @@ public void run2() throws CacheException {
Region subregion = getCache().getRegion("root/" + regionName);
DistributedTombstoneOperation gc = DistributedTombstoneOperation
.gc((DistributedRegion) subregion, new
EventID(getCache().getDistributedSystem()));
-gc.distribute();
+long viewVersion = -1;
+try {
+ viewVersion = gc.startOperation();
+ gc.distribute();
+} finally {
+ gc.endOperation(viewVersion);
--- End diff --
Same as my other question earlier, if an error occurs during start
operation, viewVersion could potentially be -1 still. Does this handle that
behavior correctly? We do this try/finally pattern in a lot of places, as
Bruce stated, maybe we could combine into a common method (there are some cases
where this may not be possible)
---
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] geode-examples pull request #3: GEODE-2231 A partitioned region example
Github user jhuynh1 commented on a diff in the pull request: https://github.com/apache/geode-examples/pull/3#discussion_r108541629 --- Diff: partitioned/README.md --- @@ -0,0 +1,156 @@ + + +# Geode partitioned region example + +This example demonstrates the basic property of partitioning. +The basic property of partitioning is that data entries are distributed +across all servers that host a region. +The distribution is like database sharding, except that the distribution +occurs automatically. It is also similar to data striping on disks, +except that the distribution is not based on hardware. + +In this example, +two servers host a single partitioned region. +There is no redundancy, so that the basic property of partitioning +may be observed. +The Producer code puts the 10 entries into the region. +The Consumer gets and prints the entries from the region. +Because the region is partitioned, +its entries are distributed among the two servers hosting the region. +Since there is no redundancy of the data within the region, +when one of the servers goes away, +the entries hosted within that server are also gone. + +## Demonstration of Partitioning +1. Set directory ```geode-examples/partitioned``` to be the +current working directory. +Each step in this example specifies paths relative to that directory. + +1. Build the jar (with the ```EmployeeKey``` and ```EmployeeData``` classes): + +``` +$ ../gradlew build +``` +1. Run a script that starts a locator and two servers. +The built JAR will be placed onto the classpath when the script +starts the servers: + +``` +$ scripts/startAll.sh +``` +Each of the servers hosts the partitioned region. + +1. Run the producer to put 10 entries into the ```EmployeeRegion```. + +``` +$ ../gradlew run -Pmain=Producer +... +... +INFO: Inserted 10 entries in EmployeeRegion. +``` + +1. There are several ways to see the contents of the region. +Run the consumer to get and print all 10 entries in the `EmployeeRegion`. + +``` +$ ../gradlew run -Pmain=Consumer +... +INFO: 10 entries in EmployeeRegion on the server(s). +... +``` + +If desired, use a ```gfsh``` query to see contents of the region keys: + +``` +$ $GEODE_HOME/bin/gfsh +... +gfsh>connect +gfsh>query --query="select e.key from /EmployeeRegion.entries e" +... +``` + +Note that the quantity of entries may also be observed with `gfsh`: + +``` +gfsh>describe region --name=EmployeeRegion +.. +Name: EmployeeRegion +Data Policy : partition +Hosting Members : server2 + server1 + +Non-Default Attributes Shared By Hosting Members + + Type |Name | Value +-- | --- | - +Region | size| 10 + | data-policy | PARTITION +``` + +As an alternative, `gfsh` maybe used to identify how many entries +are in the region on each server by looking at statistics. + +``` +gfsh>show metrics --categories=partition --region=/EmployeeRegion --member=server1 +``` + +Within the output, the result for `totalBucketSize` identifies +the number of entries hosted on the specified server. +Vary the command to see statistics for both `server1` and `server2`. + +1. The region entries are distributed across both servers. +Kill one of the servers: + +``` +$ $GEODE_HOME/bin/gfsh +... +gfsh>connect +gfsh>stop server --name=server1 +gfsh>quit +``` + +1. Run the consumer a second time, and notice that approximately half of +the entries of the ```EmployeeRegion``` are still available on the +remaining server. +Those hosted by the server that was stopped were lost. --- End diff -- As someone who might not know Geode that well, it may alarm a user that their entries were "lost." Do we really need to show them/explain this step? The next step is shutting down the entire cluster anyways? --- 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] geode-examples pull request #3: GEODE-2231 A partitioned region example
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode-examples/pull/3#discussion_r108542421
--- Diff:
partitioned/src/test/java/org/apache/geode/examples/partitioned/PartitionedTest.java
---
@@ -0,0 +1,165 @@
+/*
+ * 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.examples.partitioned;
+
+import static org.hamcrest.core.Is.*;
+import static org.junit.Assert.*;
+import static org.junit.Assume.*;
+
+import java.io.IOException;
+import java.net.ServerSocket;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+import org.apache.commons.exec.CommandLine;
+import org.apache.commons.exec.DefaultExecuteResultHandler;
+import org.apache.commons.exec.ExecuteException;
+import org.apache.commons.exec.environment.EnvironmentUtils;
+import org.apache.geode.examples.utils.ShellUtil;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+/**
+ * Tests for the shell scripts of the partitioned example
+ */
+public class PartitionedTest {
+
+ // TODO: parameterize
--- End diff --
Is this TODO still relevant?
---
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] geode-examples pull request #3: GEODE-2231 A partitioned region example
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode-examples/pull/3#discussion_r108542208
--- Diff:
partitioned/src/main/java/org/apache/geode/examples/partitioned/Producer.java
---
@@ -0,0 +1,102 @@
+/*
+ * 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.examples.partitioned;
+
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+
+import java.util.logging.Logger;
+
+public class Producer {
+
+ static final Logger logger = Logger.getAnonymousLogger();
+ // protected ClientCache clientCache;
--- End diff --
Should this commented code be removed?
---
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] geode pull request #427: GEODE-2674: Changing the lucene listener to get fro...
Github user jhuynh1 commented on a diff in the pull request: https://github.com/apache/geode/pull/427#discussion_r106482198 --- Diff: geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneEventSubstitutionFilter.java --- @@ -0,0 +1,37 @@ +/* + * 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.cache.lucene.internal; + +import org.apache.geode.cache.EntryEvent; +import org.apache.geode.cache.wan.GatewayEventSubstitutionFilter; +import org.apache.geode.internal.cache.Token; + +/** + * A substitution filter which throws away the value of the entry and replaces it with an INVALID --- End diff -- incorrect comment? we are not returning an INVALID token but rather an empty string? Should we use a static string for this? --- 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] geode pull request #410: Overhauling the javadocs for the LuceneService
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/410#discussion_r103764691
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/PageableLuceneQueryResults.java
---
@@ -15,30 +15,34 @@
package org.apache.geode.cache.lucene;
+import org.apache.geode.annotations.Experimental;
+
import java.util.Iterator;
import java.util.List;
-import org.apache.geode.annotations.Experimental;
-
/**
*
- * Defines the interface for a container of lucene query result collected
from function
- * execution.
- *
- *
+ * This interface allows you to retrieve a page of query results at time,
using the {@link #hasNext()}
--- End diff --
at *a time
---
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] geode pull request #410: Overhauling the javadocs for the LuceneService
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/410#discussion_r103763560
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/LuceneResultStruct.java
---
@@ -17,34 +17,29 @@
import org.apache.geode.annotations.Experimental;
/**
- *
- * Abstract data structure for one item in query result.
+ * A single result of a lucene query.
*
*/
@Experimental
public interface LuceneResultStruct {
/**
- * Return key of the entry
+ * @return The region key of the entry matching the query
*
- * @return key
- * @throws IllegalArgumentException If this struct does not contain key
*/
public K getKey();
/**
- * Return value of the entry
+ * @return the region value of the entry matching the query.
*
- * @return value the whole domain object
- * @throws IllegalArgumentException If this struct does not contain value
*/
public V getValue();
/**
- * Return score of the query
+ * Return score of the score of the entry matching the query. Scores
--- End diff --
I think you meant "Return score of the entry..."?
---
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] geode pull request #410: Overhauling the javadocs for the LuceneService
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/410#discussion_r103763944
--- Diff:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/LuceneService.java ---
@@ -18,53 +18,77 @@
import java.util.Map;
import java.util.concurrent.TimeUnit;
+import org.apache.geode.cache.DataPolicy;
import org.apache.lucene.analysis.Analyzer;
import org.apache.geode.annotations.Experimental;
import org.apache.geode.cache.GemFireCache;
import org.apache.geode.cache.lucene.internal.LuceneIndexCreationProfile;
/**
- * LuceneService instance is a singleton for each cache.
- *
- * It provides handle for managing the {@link LuceneIndex} and create the
{@link LuceneQuery} via
- * {@link LuceneQueryFactory}
- *
+ *
+ * The LuceneService provides the capability to create lucene indexes and
execute lucene
+ * queries on data stored in geode regions. The lucene indexes are
automatically maintained
+ * by geode whenever entries are updated in the associated regions.
+ *
+ *
+ * To obtain an instance of LuceneService, use {@link
LuceneServiceProvider#get(GemFireCache)}.
+ *
+ *
+ * Lucene indexes can be created using gfsh, xml, or the java API. Below
is an example of
+ * creating a lucene index with the java API. The lucene index created on
each member that
+ * will host data for the region.
*
- * Example:
- *
*
- * At client and server JVM, initializing cache will create the
LuceneServiceImpl object,
- * which is a singleton at each JVM.
- *
- * At each server JVM, for data region to create index, create the index
on fields with default analyzer:
- * LuceneIndex index = luceneService.createIndex(indexName, regionName,
"field1", "field2", "field3");
- * or create index on fields with specified analyzer:
+ * {@code
+ * LuceneIndex index = luceneService.createIndex(indexName, regionName,
"field1", "field2", "field3");
+ * }
+ *
+ *
+ * You can also specify what {@link Analyzer} to use for each field.
+ *
+ *
+ * {@code
* LuceneIndex index = luceneService.createIndex(indexName, regionName,
analyzerPerField);
- *
- * We can also create index via cache.xml or gfsh.
- *
- * At client side, create query and run the search:
- *
- * LuceneQuery query =
luceneService.createLuceneQueryFactory().setLimit(200).setPageSize(20)
- * .setResultTypes(SCORE, VALUE, KEY).setFieldProjection("field1",
"field2")
- * .create(indexName, regionName, querystring, analyzer);
- *
- * The querystring is using lucene's queryparser syntax, such as
"field1:zhou* AND field2:gz...@pivotal.io"
- *
- * PageableLuceneQueryResults results = query.search();
- *
- * If pagination is not specified:
- * List list = results.getNextPage(); // return all results in one
getNextPage() call
- * or if paging is specified:
- * if (results.hasNextPage()) {
- * List page = results.nextPage(); // return resules page by page
* }
- *
- * The item of the list is either the domain object or instance of {@link
LuceneResultStruct}
*
- *
*
+ * Indexes should be created on all peers that host the region being
indexed. Clients do not need
+ * to define the index, they can directly execute queries using this
service.
+ *
+ *
+ * Queries on this service can return either the region keys, values, or
both that match
+ * a lucene query expression. To execute a query, start with the {@link
#createLuceneQueryFactory()} method.
+ *
+ *
+ *
+ * {@code
+ * LuceneQuery query = luceneService.createLuceneQueryFactory()
+ *.setLimit(200)
+ *.create(indexName, regionName, "name:John AND zipcode:97006",
defaultField);
+ * Collection results = query.findValues();
+ * }
+ *
+ *
+ *
+ * The lucene index data is colocated with the region that is indexed.
This means
+ * that the index data will partitioned, copied, or persisted using the
same configuration
--- End diff --
will *be partitioned
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92691796
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionQueryDUnitTest.java
---
@@ -149,6 +154,92 @@ public void run() {
});
}
+ @Test
+ public void testIndexDoesNotDeserializePdxObjects() {
+Host host = Host.getHost(0);
+VM vm0 = host.getVM(0);
+VM vm1 = host.getVM(1);
+
+SerializableRunnableIF createPR = () -> {
+ Cache cache = getCache();
+ PartitionAttributesFactory paf = new PartitionAttributesFactory();
+ paf.setTotalNumBuckets(10);
+
cache.createRegionFactory(RegionShortcut.PARTITION).setPartitionAttributes(paf.create())
+ .create("region");
+};
+vm0.invoke(createPR);
+vm1.invoke(createPR);
+
+// Do Puts. These objects can't be deserialized because they throw
+// and exception from the constructor
+vm0.invoke(() -> {
+ Cache cache = getCache();
+ Region region = cache.getRegion("region");
+ region.put(0, new PdxNotDeserializableAsset(0, "B"));
+ region.put(10, new PdxNotDeserializableAsset(1, "B"));
+ region.put(1, new PdxNotDeserializableAsset(1, "B"));
+ IntStream.range(11, 100)
+ .forEach(i -> region.put(i, new PdxNotDeserializableAsset(i,
Integer.toString(i;
+});
+
+// If this tries to deserialize the assets, it will fail
+vm0.invoke(() -> {
+ Cache cache = getCache();
+ cache.getQueryService().createHashIndex("ContractDocumentIndex",
"document", "/region");
--- End diff --
Yeah, that's the hardest part about the query tests, there are so many test
classes that it can get hard to figure out where the best place for it is. If
you feel ambitious/adventurous you can work on the hierarchy otherwise, I think
this spot would be ok. Hopefully one day we can revisit and organize them a
bit better.
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92677109
--- Diff:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
---
@@ -8751,18 +8751,26 @@ public Index createIndex(boolean
remotelyOriginated, IndexType indexType, String
// Second step is iterating over REs and populating all the created
indexes
if (unpopulatedIndexes != null && unpopulatedIndexes.size() > 0) {
- throwException = populateEmptyIndexes(unpopulatedIndexes,
exceptionsMap);
+ throwException |= populateEmptyIndexes(unpopulatedIndexes,
exceptionsMap);
}
// Third step is to send the message to remote nodes
// Locally originated create index request.
// Send create request to other PR nodes.
-throwException =
+throwException |=
sendCreateIndexesMessage(remotelyOriginated, indexDefinitions,
indexes, exceptionsMap);
// If exception is throw in any of the above steps
if (throwException) {
- throw new MultiIndexCreationException(exceptionsMap);
+ try {
+for (String indexName : exceptionsMap.keySet()) {
+ Index index = indexManager.getIndex(indexName);
+ indexManager.removeIndex(index);
+ removeIndex(index, remotelyOriginated);
--- End diff --
Just a question, was the calling method not cleaning up indexes if an
exception was thrown?
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92678100
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionQueryDUnitTest.java
---
@@ -149,6 +154,92 @@ public void run() {
});
}
+ @Test
+ public void testIndexDoesNotDeserializePdxObjects() {
+Host host = Host.getHost(0);
+VM vm0 = host.getVM(0);
+VM vm1 = host.getVM(1);
+
+SerializableRunnableIF createPR = () -> {
+ Cache cache = getCache();
+ PartitionAttributesFactory paf = new PartitionAttributesFactory();
+ paf.setTotalNumBuckets(10);
+
cache.createRegionFactory(RegionShortcut.PARTITION).setPartitionAttributes(paf.create())
+ .create("region");
+};
+vm0.invoke(createPR);
+vm1.invoke(createPR);
+
+// Do Puts. These objects can't be deserialized because they throw
+// and exception from the constructor
+vm0.invoke(() -> {
+ Cache cache = getCache();
+ Region region = cache.getRegion("region");
+ region.put(0, new PdxNotDeserializableAsset(0, "B"));
+ region.put(10, new PdxNotDeserializableAsset(1, "B"));
+ region.put(1, new PdxNotDeserializableAsset(1, "B"));
+ IntStream.range(11, 100)
+ .forEach(i -> region.put(i, new PdxNotDeserializableAsset(i,
Integer.toString(i;
+});
+
+// If this tries to deserialize the assets, it will fail
+vm0.invoke(() -> {
+ Cache cache = getCache();
+ cache.getQueryService().createHashIndex("ContractDocumentIndex",
"document", "/region");
+});
+
+vm0.invoke(() -> {
+ QueryService qs = getCache().getQueryService();
+ SelectResults results = (SelectResults) qs
+ .newQuery(" select assetId,document from /region where
document='B' limit 1000")
+ .execute();
+
+ assertEquals(3, results.size());
+ final Index index = qs.getIndex(getCache().getRegion("region"),
"ContractDocumentIndex");
+ assertEquals(1, index.getStatistics().getTotalUses());
+});
+ }
+
+ @Test
+ public void testFailureToCreateIndexOnRemoteNodeThrowsException() {
--- End diff --
Should there be a test for create index on local node fails but remotes are
ok?
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92677783
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionQueryDUnitTest.java
---
@@ -149,6 +154,92 @@ public void run() {
});
}
+ @Test
+ public void testIndexDoesNotDeserializePdxObjects() {
+Host host = Host.getHost(0);
+VM vm0 = host.getVM(0);
+VM vm1 = host.getVM(1);
+
+SerializableRunnableIF createPR = () -> {
+ Cache cache = getCache();
+ PartitionAttributesFactory paf = new PartitionAttributesFactory();
+ paf.setTotalNumBuckets(10);
+
cache.createRegionFactory(RegionShortcut.PARTITION).setPartitionAttributes(paf.create())
+ .create("region");
+};
+vm0.invoke(createPR);
+vm1.invoke(createPR);
+
+// Do Puts. These objects can't be deserialized because they throw
+// and exception from the constructor
+vm0.invoke(() -> {
+ Cache cache = getCache();
+ Region region = cache.getRegion("region");
+ region.put(0, new PdxNotDeserializableAsset(0, "B"));
+ region.put(10, new PdxNotDeserializableAsset(1, "B"));
+ region.put(1, new PdxNotDeserializableAsset(1, "B"));
+ IntStream.range(11, 100)
+ .forEach(i -> region.put(i, new PdxNotDeserializableAsset(i,
Integer.toString(i;
+});
+
+// If this tries to deserialize the assets, it will fail
+vm0.invoke(() -> {
+ Cache cache = getCache();
+ cache.getQueryService().createHashIndex("ContractDocumentIndex",
"document", "/region");
--- End diff --
Would you be able to test out a range index (not a compact range index or
hash)
This would require the index expression to contain multiple iterators such
as /region r, r.someCollection p (assuming the values in the region have
someCollection populated) The indexed expression itself could stay the same
but it would try to create tuples. In this case, I would think that we would
have to deserialize the value based on how that index maintains it's keys...
Just wanted to make sure that index worked correctly (the test would obviously
throw an error, but I just wanted to make sure that index still gets created
correctly...)
---
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] geode pull request #303: GEODE-1984: Fix Issue Make GatewaySender destroy a ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/303#discussion_r91423780
--- Diff:
geode-wan/src/test/java/org/apache/geode/internal/cache/wan/wancommand/WanCommandCreateDestroyGatewaySenderDUnitTest.java
---
@@ -282,13 +335,40 @@ public void
testCreateGatewaySenderWithGatewayEventFilters() {
1000, 5000, true, false, 1000, 100, 2, OrderPolicy.THREAD,
eventFilters, null));
vm5.invoke(() -> verifySenderAttributes("ln", 2, false, true, 1000,
socketReadTimeout, true,
1000, 5000, true, false, 1000, 100, 2, OrderPolicy.THREAD,
eventFilters, null));
+
+// Test Destroy Command.
+command =
+CliStrings.DESTROY_GATEWAYSENDER + " --" +
CliStrings.DESTROY_GATEWAYSENDER__ID + "=ln";
--- End diff --
This code looks copy pasted in other areas, any way we can pull it into a
separate method for reuse?
---
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] geode pull request #303: GEODE-1984: Fix Issue Make GatewaySender destroy a ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/303#discussion_r91390603
--- Diff:
geode-wan/src/test/java/org/apache/geode/internal/cache/wan/wancommand/WanCommandCreateDestroyGatewaySenderDUnitTest.java
---
@@ -55,7 +55,7 @@ private CommandResult
executeCommandWithIgnoredExceptions(String command) {
* GatewaySender with all default attributes
--- End diff --
Would it make sense to create different tests for destroy and keep them
separate from testing create? That way when a test fails we know exactly what
is failing (create vs destroy).
---
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] geode pull request #303: GEODE-1984: Fix Issue Make GatewaySender destroy a ...
Github user jhuynh1 commented on a diff in the pull request: https://github.com/apache/geode/pull/303#discussion_r91348684 --- Diff: geode-core/src/main/java/org/apache/geode/cache/wan/GatewaySender.java --- @@ -400,4 +400,19 @@ public int getMaxParallelismForReplicatedRegion(); + + /** + * Destroys the GatewaySender. Before destroying the sender, caller needs to to ensure that the --- End diff -- minor grammar: change the "to to" -> "to" --- 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] geode pull request #303: GEODE-1984: Fix Issue Make GatewaySender destroy a ...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/303#discussion_r91391868
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/GatewaySenderDestroyFunction.java
---
@@ -0,0 +1,90 @@
+/*
+ * 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.cli.functions;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.execute.FunctionAdapter;
+import org.apache.geode.cache.execute.FunctionContext;
+import org.apache.geode.cache.execute.ResultSender;
+import org.apache.geode.cache.wan.GatewaySender;
+import org.apache.geode.internal.InternalEntity;
+import org.apache.geode.internal.cache.wan.GatewaySenderException;
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.management.internal.cli.CliUtil;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.logging.log4j.Logger;
+
+public class GatewaySenderDestroyFunction extends FunctionAdapter
implements InternalEntity {
+ private static final long serialVersionUID = 1459761440357690134L;
+
+ private static final Logger logger = LogService.getLogger();
+ private static final String ID =
GatewaySenderDestroyFunction.class.getName();
+ public static GatewaySenderDestroyFunction INSTANCE = new
GatewaySenderDestroyFunction();
+
+ @Override
+ public void execute(FunctionContext context) {
+ResultSender resultSender = context.getResultSender();
+
+Cache cache = CacheFactory.getAnyInstance();
+String memberNameOrId =
+
CliUtil.getMemberNameOrId(cache.getDistributedSystem().getDistributedMember());
+
+GatewaySenderDestroyFunctionArgs gatewaySenderDestroyFunctionArgs =
+(GatewaySenderDestroyFunctionArgs) context.getArguments();
+
+try {
+ GatewaySender gatewaySender =
+ cache.getGatewaySender(gatewaySenderDestroyFunctionArgs.getId());
+ if (gatewaySender != null) {
+gatewaySender.stop();
--- End diff --
Is there a way to check to make sure the gateway is actually stopped
first(). I don't think the stop() method is a blocking call and the sender
itself could technically still be running. stop() probably should be changed
to actually stop the sender before returning but I don't think it's the case
today.
---
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] geode pull request #303: GEODE-1984: Fix Issue Make GatewaySender destroy a ...
Github user jhuynh1 commented on a diff in the pull request: https://github.com/apache/geode/pull/303#discussion_r91360319 --- Diff: geode-wan/src/test/java/org/apache/geode/internal/cache/wan/wancommand/WANCommandTestBase.java --- @@ -448,6 +452,37 @@ public void verifyReceiverCreationWithAttributes(boolean isRunning, int startPor } } + // Added for gateway destroy command + // Copied from WANTestBase.java --- End diff -- Not sure if we need to know this? Perhaps remove the comment --- 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 #293: GEODE-1653: Executing a fire-and-forget f...
Github user jhuynh1 commented on a diff in the pull request: https://github.com/apache/incubator-geode/pull/293#discussion_r90116715 --- Diff: geode-core/src/test/java/org/apache/geode/internal/cache/FireAndForgetFunctionOnAllServersDUnitTest.java --- @@ -0,0 +1,164 @@ +/* + * 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.cache; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.client.*; +import org.apache.geode.cache.client.internal.LocatorTestBase; +import org.apache.geode.cache.execute.Execution; +import org.apache.geode.cache.execute.Function; +import org.apache.geode.cache.execute.FunctionService; +import org.apache.geode.internal.AvailablePort; +import org.apache.geode.internal.cache.functions.TestFunction; +import org.apache.geode.test.dunit.Assert; +import org.apache.geode.test.dunit.Host; +import org.apache.geode.test.dunit.NetworkUtils; +import org.apache.geode.test.dunit.VM; +import org.apache.geode.test.junit.categories.DistributedTest; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import static com.jayway.awaitility.Awaitility.*; +import static java.util.concurrent.TimeUnit.*; + +/** + * Created by amey on 22/11/16. --- End diff -- Minor nitpick, would you be able to remove the author tags? --- 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 #300: [GEODE-224] Geode Spark connector parser is not ...
Github user jhuynh1 commented on the issue: https://github.com/apache/incubator-geode/pull/300 The changes look good, I've merged them in. Thanks for the contribution! --- 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 #293: GEODE-1653: Executing a fire-and-forget f...
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/293#discussion_r89148312
--- Diff:
geode-core/src/test/java/org/apache/geode/internal/cache/BugGeode_1653DUnitTest.java
---
@@ -0,0 +1,145 @@
+/*
+ * 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.cache;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.*;
+import org.apache.geode.cache.client.internal.LocatorTestBase;
+import org.apache.geode.cache.execute.Execution;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.FunctionService;
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.internal.cache.functions.TestFunction;
+import org.apache.geode.test.dunit.Assert;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.NetworkUtils;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+/**
+ * Created by amey on 22/11/16.
+ */
+@Category(DistributedTest.class)
+public class BugGeode_1653DUnitTest extends LocatorTestBase {
+
+ public BugGeode_1653DUnitTest() {
+super();
+ }
+
+ @Override
+ public final void postSetUp() throws Exception {
+disconnectAllFromDS();
+ }
+
+ @Override
+ protected final void postTearDownLocatorTestBase() throws Exception {
+disconnectAllFromDS();
+ }
+
+ @Test
+ public void testBugGeode_1653() {
+
+// Test case for Executing a fire-and-forget function on all servers
as opposed to only
+// executing on the ones the
+// client is currently connected to.
+
+Host host = Host.getHost(0);
+VM locator = host.getVM(0);
+VM server1 = host.getVM(1);
+VM server2 = host.getVM(2);
+VM client = host.getVM(3);
+
+final int locatorPort =
AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
+final String locatorHost = NetworkUtils.getServerHostName(host);
+
+// Step 1. Start a locator and one cache server.
+locator.invoke("Start Locator", () -> startLocator(locatorHost,
locatorPort, ""));
+
+String locString = getLocatorString(host, locatorPort);
+
+// Step 2. Start a server and create a replicated region "R1".
+server1.invoke("Start BridgeServer",
+() -> startBridgeServer(new String[] {"R1"}, locString, new
String[] {"R1"}));
+
+// Step 3. Create a client cache with pool mentioning locator.
+client.invoke("create client cache and pool mentioning locator", () ->
{
+ ClientCacheFactory ccf = new ClientCacheFactory();
+ ccf.addPoolLocator(locatorHost, locatorPort);
+ ClientCache cache = ccf.create();
+ Pool pool1 = PoolManager.createFactory().addLocator(locatorHost,
locatorPort)
+ .setServerGroup("R1").create("R1");
+
+ Region region1 =
cache.createClientRegionFactory(ClientRegionShortcut.PROXY).setPoolName("R1")
+ .create("R1");
+
+ // Step 4. Execute the function to put DistributedMemberID into
above created replicated
+ // region.
+ Function function = new TestFunction(false,
TestFunction.TEST_FUNCTION_1653);
+ FunctionService.registerFunction(function);
+
+ String regionName = "R1";
+ Execution dataSet = FunctionService.onServers(pool1);
+ dataSet.withArgs(regionName).execute(function);
+ Thread.sleep(1000);
--- End diff --
I think the name of the test and class should also be more explicit and not
reference bug numbers
---
If your project is set up for it, you can reply to this email and have your
reply app
