[
https://issues.apache.org/jira/browse/GEODE-7309?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17294042#comment-17294042
]
ASF GitHub Bot commented on GEODE-7309:
---------------------------------------
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:
us...@infra.apache.org
> Upgrade Lucene from 6.6.2 to 8.2.0
> ----------------------------------
>
> Key: GEODE-7309
> URL: https://issues.apache.org/jira/browse/GEODE-7309
> Project: Geode
> Issue Type: Sub-task
> Reporter: Mario Kevo
> Assignee: Mario Kevo
> Priority: Major
> Labels: pull-request-available
> Time Spent: 5.5h
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
