You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/02 07:26:07 UTC
[GitHub] [geode] mkevo opened a new pull request #6076: uplift lucene to 7.1.0
mkevo opened a new pull request #6076:
URL: https://github.com/apache/geode/pull/6076
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, check Concourse 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a change in pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on a change in pull request #6076:
URL: https://github.com/apache/geode/pull/6076#discussion_r585337063
##########
File path: geode-lucene/build.gradle
##########
@@ -71,6 +71,7 @@ dependencies {
upgradeTestImplementation(project(':geode-dunit'))
upgradeTestImplementation('commons-io:commons-io')
+ upgradeTestImplementation('org.apache.lucene:lucene-backward-codecs')
Review comment:
just checking, is backward-codecs really only required by tests, not end-users too? I would naively assume it needs to be shipped with Geode as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] karensmolermiller commented on a change in pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
karensmolermiller commented on a change in pull request #6076:
URL: https://github.com/apache/geode/pull/6076#discussion_r586647844
##########
File path: geode-docs/tools_modules/lucene_integration.html.md.erb
##########
@@ -334,6 +334,7 @@ Lucene indexes created for all members.
# <a id="LuceneRandC" class="no-quick-link"></a>Requirements and Caveats
- Join queries between regions are not supported.
+- Executing queries is possible after all members are at the same version.
Review comment:
This statement is not quite specific enough. I'm guessing that all members must be at the same minor version number (not necessarily the same patch version within the minor). I'm also guessing that this is just for Lucene queries.
If I'm correct in my guesses, an attempt at better wording might be
> All cluster members must be running the same minor version in order to execute Lucene queries.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] karensmolermiller commented on pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
karensmolermiller commented on pull request #6076:
URL: https://github.com/apache/geode/pull/6076#issuecomment-793062536
Docs change looks good to me now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on a change in pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
mkevo commented on a change in pull request #6076:
URL: https://github.com/apache/geode/pull/6076#discussion_r585509589
##########
File path: geode-lucene/build.gradle
##########
@@ -71,6 +71,7 @@ dependencies {
upgradeTestImplementation(project(':geode-dunit'))
upgradeTestImplementation('commons-io:commons-io')
+ upgradeTestImplementation('org.apache.lucene:lucene-backward-codecs')
Review comment:
I read some articles about Lucene and its similarities and only find that this backward-codecs is needed for test. So I'm not sure taht we need to ship it with Geode.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on a change in pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
mkevo commented on a change in pull request #6076:
URL: https://github.com/apache/geode/pull/6076#discussion_r585509589
##########
File path: geode-lucene/build.gradle
##########
@@ -71,6 +71,7 @@ dependencies {
upgradeTestImplementation(project(':geode-dunit'))
upgradeTestImplementation('commons-io:commons-io')
+ upgradeTestImplementation('org.apache.lucene:lucene-backward-codecs')
Review comment:
I read some articles about Lucene and its similarities and only find that this backward-codecs is needed for test. So I'm not sure that we need to ship it with Geode.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #6076:
URL: https://github.com/apache/geode/pull/6076#issuecomment-823917771
The new PR is created, rebased on the latest develop branch, and removed **_1** from the branch name.
#6351
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on a change in pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
mkevo commented on a change in pull request #6076:
URL: https://github.com/apache/geode/pull/6076#discussion_r587439661
##########
File path: geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneQueryWithDifferentVersions.java
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.awaitility.GeodeAwaitility.await;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.distributed.internal.InternalLocator;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.test.dunit.DistributedTestUtils;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.NetworkUtils;
+import org.apache.geode.test.dunit.VM;
+
+public class LuceneQueryWithDifferentVersions
+ extends LuceneSearchWithRollingUpgradeDUnit {
+
+ // 2 locator, 2 servers
+ @Test
+ public void luceneQueryCannotBeExecuted()
+ throws Exception {
+ final Host host = Host.getHost(0);
+ VM locator1 = host.getVM(oldVersion, 0);
+ VM locator2 = host.getVM(oldVersion, 1);
+ VM server1 = host.getVM(oldVersion, 2);
+ VM server2 = host.getVM(oldVersion, 3);
+
+ final String regionName = "aRegion";
+ RegionShortcut shortcut = RegionShortcut.PARTITION_REDUNDANT;
+ String regionType = "partitionedRedundant";
+
+ int[] locatorPorts = AvailablePortHelper.getRandomAvailableTCPPorts(2);
+ locator1.invoke(() -> DistributedTestUtils.deleteLocatorStateFile(locatorPorts));
+ locator2.invoke(() -> DistributedTestUtils.deleteLocatorStateFile(locatorPorts));
+
+ String hostName = NetworkUtils.getServerHostName(host);
+ String locatorString = getLocatorString(locatorPorts);
+ try {
+ locator1.invoke(
+ invokeStartLocator(hostName, locatorPorts[0], getLocatorPropertiesPre91(locatorString)));
+ locator2.invoke(
+ invokeStartLocator(hostName, locatorPorts[1], getLocatorPropertiesPre91(locatorString)));
+ invokeRunnableInVMs(invokeCreateCache(getSystemProperties(locatorPorts)), server1, server2);
+
+ // Locators before 1.4 handled configuration asynchronously.
+ // We must wait for configuration configuration to be ready, or confirm that it is disabled.
+ locator1.invoke(
+ () -> await()
Review comment:
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo closed pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
mkevo closed pull request #6076:
URL: https://github.com/apache/geode/pull/6076
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on a change in pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
mkevo commented on a change in pull request #6076:
URL: https://github.com/apache/geode/pull/6076#discussion_r587433351
##########
File path: geode-docs/tools_modules/lucene_integration.html.md.erb
##########
@@ -334,6 +334,7 @@ Lucene indexes created for all members.
# <a id="LuceneRandC" class="no-quick-link"></a>Requirements and Caveats
- Join queries between regions are not supported.
+- Executing queries is possible after all members are at the same version.
Review comment:
Yes, you are right! Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6076:
URL: https://github.com/apache/geode/pull/6076#discussion_r585984716
##########
File path: geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneQueryWithDifferentVersions.java
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.awaitility.GeodeAwaitility.await;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.distributed.internal.InternalLocator;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.test.dunit.DistributedTestUtils;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.NetworkUtils;
+import org.apache.geode.test.dunit.VM;
+
+public class LuceneQueryWithDifferentVersions
+ extends LuceneSearchWithRollingUpgradeDUnit {
+
+ // 2 locator, 2 servers
+ @Test
+ public void luceneQueryCannotBeExecuted()
+ throws Exception {
+ final Host host = Host.getHost(0);
+ VM locator1 = host.getVM(oldVersion, 0);
+ VM locator2 = host.getVM(oldVersion, 1);
+ VM server1 = host.getVM(oldVersion, 2);
+ VM server2 = host.getVM(oldVersion, 3);
+
+ final String regionName = "aRegion";
+ RegionShortcut shortcut = RegionShortcut.PARTITION_REDUNDANT;
+ String regionType = "partitionedRedundant";
+
+ int[] locatorPorts = AvailablePortHelper.getRandomAvailableTCPPorts(2);
+ locator1.invoke(() -> DistributedTestUtils.deleteLocatorStateFile(locatorPorts));
+ locator2.invoke(() -> DistributedTestUtils.deleteLocatorStateFile(locatorPorts));
+
+ String hostName = NetworkUtils.getServerHostName(host);
+ String locatorString = getLocatorString(locatorPorts);
+ try {
+ locator1.invoke(
+ invokeStartLocator(hostName, locatorPorts[0], getLocatorPropertiesPre91(locatorString)));
+ locator2.invoke(
+ invokeStartLocator(hostName, locatorPorts[1], getLocatorPropertiesPre91(locatorString)));
+ invokeRunnableInVMs(invokeCreateCache(getSystemProperties(locatorPorts)), server1, server2);
+
+ // Locators before 1.4 handled configuration asynchronously.
+ // We must wait for configuration configuration to be ready, or confirm that it is disabled.
+ locator1.invoke(
+ () -> await()
Review comment:
I would get rid of the `!InternalLocator.getLocator().getConfig().getEnableClusterConfiguration()` unless you're parameterizing it to run both with and without cluster config. Then you can simplify it to:
```
await().untilAsserted(() -> {
assertThat(InternalLocator.getLocator().isSharedConfigurationRunning()).isTrue();
}
```
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/InternalCacheForClientAccess.java
##########
@@ -1225,6 +1226,14 @@ public void unlockDiskStore(String diskStoreName) {
}
+ @Override
+ public boolean hasMemberOlderThan(KnownVersion version) {
+ return getMembers().stream()
Review comment:
Can you just have this method delegate to `delegate` like all the other methods are doing?
```
@Override
public boolean hasMemberOlderThan(KnownVersion version) {
return delegate.hasMemberOlderThan(version);
}
```
You should not have to add the same method body to both `GemFireCacheImpl` and `InternalCacheForClientAccess`.
##########
File path: geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/repository/IndexRepositoryImpl.java
##########
@@ -143,8 +145,35 @@ public void delete(Object key) throws IOException {
@Override
public void query(Query query, int limit, IndexResultCollector collector) throws IOException {
long start = stats.startRepositoryQuery();
- int totalHits = 0;
+ long totalHits = 0;
IndexSearcher searcher = searcherManager.acquire();
+ searcher.setSimilarity(new TFIDFSimilarity() {
Review comment:
I would recommend making TFIDFSimilarity a private static inner-class instead of an anonymous class in the constructor. If it's a functional interface with just one method then this approach is fairly clean but more than that and it tends to get messy.
##########
File path: geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneSearchWithRollingUpgradeTestBase.java
##########
@@ -295,22 +295,26 @@ void verifyLuceneQueryResults(String regionName, int expectedRegionSize)
protected Collection executeLuceneQuery(Object luceneQuery)
throws IllegalAccessException, InvocationTargetException, NoSuchMethodException {
Collection results = null;
- int retryCount = 10;
- while (true) {
- try {
- results = (Collection) luceneQuery.getClass().getMethod("findKeys").invoke(luceneQuery);
- break;
- } catch (Exception ex) {
- if (!ex.getCause().getMessage().contains("currently indexing")) {
- throw ex;
- }
- if (--retryCount == 0) {
- throw ex;
- }
- }
- }
+ await().untilAsserted(() -> assertThat(isIndexingFinished(luceneQuery)).isTrue());
Review comment:
I would delete the method `isIndexingFinished` and simplify this down to:
```
await().ignoreExceptions().untilAsserted(() -> {
luceneQuery.getClass().getMethod("findKeys").invoke(luceneQuery);
}
```
When I'm digging around in Awaitility to see what its capabilities are, I find it most useful to look at the class `ConditionFactory` which is returned by `await()`.
Returning a boolean provides so little information about what went wrong, but the version above will print out the full stack trace of what went wrong, ie. why the query didn't complete without throwing.
##########
File path: geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneSearchWithRollingUpgradeTestBase.java
##########
@@ -295,22 +295,26 @@ void verifyLuceneQueryResults(String regionName, int expectedRegionSize)
protected Collection executeLuceneQuery(Object luceneQuery)
throws IllegalAccessException, InvocationTargetException, NoSuchMethodException {
Collection results = null;
- int retryCount = 10;
- while (true) {
- try {
- results = (Collection) luceneQuery.getClass().getMethod("findKeys").invoke(luceneQuery);
- break;
- } catch (Exception ex) {
- if (!ex.getCause().getMessage().contains("currently indexing")) {
- throw ex;
- }
- if (--retryCount == 0) {
- throw ex;
- }
- }
- }
+ await().untilAsserted(() -> assertThat(isIndexingFinished(luceneQuery)).isTrue());
Review comment:
You can do some experimenting to see the failure messages with something like this:
```
@Test
public void foo() {
await().atMost(2, TimeUnit.SECONDS).ignoreExceptions().untilAsserted(() -> {
throw new IllegalStateException("foo");
});
}
```
##########
File path: geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/RollingUpgradeQueryReturnsCorrectResultsAfterClientAndServersAreRolledOverAllBucketsCreated.java
##########
@@ -31,6 +32,7 @@
public class RollingUpgradeQueryReturnsCorrectResultsAfterClientAndServersAreRolledOverAllBucketsCreated
extends LuceneSearchWithRollingUpgradeDUnit {
+ @Ignore
Review comment:
Don't commit this with `@Ignore` on a test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on a change in pull request #6076: GEODE-7309: uplift lucene to 7.1.0
Posted by GitBox <gi...@apache.org>.
mkevo commented on a change in pull request #6076:
URL: https://github.com/apache/geode/pull/6076#discussion_r587841824
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/InternalCacheForClientAccess.java
##########
@@ -1225,6 +1226,14 @@ public void unlockDiskStore(String diskStoreName) {
}
+ @Override
+ public boolean hasMemberOlderThan(KnownVersion version) {
+ return getMembers().stream()
Review comment:
Thanks for the suggestion!
As both `GemFireCacheImpl` and `InternalCacheForClientAccess` implements `InternalCache`, I think that we need to add method body in both classes.
##########
File path: geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/RollingUpgradeQueryReturnsCorrectResultsAfterClientAndServersAreRolledOverAllBucketsCreated.java
##########
@@ -31,6 +32,7 @@
public class RollingUpgradeQueryReturnsCorrectResultsAfterClientAndServersAreRolledOverAllBucketsCreated
extends LuceneSearchWithRollingUpgradeDUnit {
+ @Ignore
Review comment:
Upgrading to the Lucene 7.1.0 this test is not supported as formatting is changed between versions. In that case we add note to the documentation so all cluster members must be on the same version in order to execute Lucene queries.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org