DonalEvans commented on a change in pull request #6351:
URL: https://github.com/apache/geode/pull/6351#discussion_r624039673
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
##########
@@ -1124,6 +1125,14 @@ public void setQueryConfigurationServiceCreation(
this.queryConfigurationServiceCreation = queryConfigurationServiceCreation;
}
+ @Override
+ public boolean hasMemberOlderThan(KnownVersion version) {
+ return getMembers().stream()
Review comment:
`CacheCreation.getMembers()` always returns an empty set, so the rest of
this method seems unnecessary.
##########
File path:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/LuceneServiceImpl.java
##########
@@ -238,7 +238,7 @@ public void createIndex(final String indexName, String
regionPath, final Analyze
}
}
- protected void validateAllMembersAreTheSameVersion(PartitionedRegion region)
{
+ public void validateAllMembersAreTheSameVersion(PartitionedRegion region) {
Review comment:
Why was the visibility of this method changed? It appears to only be
called from within this class, so it could in fact be private. It is overridden
in `TestLuceneServiceImpl` in `LuceneServiceImplJUnitTest`, but with a little
additional mocking, the only test that uses that class can be made to pass
without needing to override this method.
##########
File path:
geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneQueryWithDifferentVersions.java
##########
@@ -0,0 +1,101 @@
+/*
+ * 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 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
Review comment:
Can this class be renamed to indicate that it's a rolling upgrade test
please.
##########
File path:
geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneQueryWithDifferentVersions.java
##########
@@ -0,0 +1,101 @@
+/*
+ * 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 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().untilAsserted(() -> await().untilAsserted(() -> {
+
assertThat(InternalLocator.getLocator().isSharedConfigurationRunning()).isTrue();
+ })));
+ locator2.invoke(
+ () -> await()
+ .untilAsserted(() -> await().untilAsserted(() -> {
+
assertThat(InternalLocator.getLocator().isSharedConfigurationRunning()).isTrue();
+ })));
+
+ server1.invoke(() -> createLuceneIndex(cache, regionName, INDEX_NAME));
+ server2.invoke(() -> createLuceneIndex(cache, regionName, INDEX_NAME));
+
+ invokeRunnableInVMs(invokeCreateRegion(regionName, shortcut.name()),
server1, server2);
+
+ putSerializableObjectAndVerifyLuceneQueryResult(server1, regionName, 10,
0,
+ 10, server1, server2);
+ locator1 = rollLocatorToCurrent(locator1, hostName, locatorPorts[0],
getTestMethodName(),
+ locatorString);
+
+ locator2 = rollLocatorToCurrent(locator2, hostName, locatorPorts[1],
getTestMethodName(),
+ locatorString);
+
+ server1 = rollServerToCurrentCreateLuceneIndexAndCreateRegion(server1,
regionType, null,
+ shortcut.name(), regionName, locatorPorts, reindex);
+
+ Throwable thrown = catchThrowable(() -> {
+ putSerializableObjectAndVerifyLuceneQueryResult(server2, regionName,
20, 15,
+ 25, server2);
+ });
+
+ assertThat(thrown.getCause()).isInstanceOf(AssertionError.class);
Review comment:
This is the only assertion that this test makes, and it's very vague.
Would it be possible to be more specific in terms of what exactly the test
expects to fail, and where? Instead of asserting that some other assertion from
`putSerializableObjectAndVerifyLuceneQueryResult()` fails, it would be better
to have a specific assertion for this test showing exactly where and how we
expect the query verification to fail.
##########
File path:
geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneQueryWithDifferentVersions.java
##########
@@ -0,0 +1,101 @@
+/*
+ * 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 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().untilAsserted(() -> await().untilAsserted(() -> {
Review comment:
Why are two `await()` calls being chained here?
##########
File path:
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/repository/IndexRepositoryImpl.java
##########
@@ -227,4 +230,31 @@ public int getAsInt() {
}
}
}
+
+ private static class Similarity extends TFIDFSimilarity {
Review comment:
To avoid confusion with the base Similarity class, might it be better to
name this one something different? Also, it's not clear to me what the reason
for using the specific implementations of the overridden methods here is. They
appear to be very similar to the implementations in `ClassicSimilarity`, so I
wonder if that class could just be used instead of defining a new
implementation.
##########
File path:
geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneQueryWithDifferentVersions.java
##########
@@ -0,0 +1,101 @@
+/*
+ * 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 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()
Review comment:
This test name could be more descriptive. Indicating specifically when a
Lucene query cannot be executed would make it a lot clearer what the test is
doing.
##########
File path:
geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneQueryWithDifferentVersions.java
##########
@@ -0,0 +1,101 @@
+/*
+ * 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 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);
Review comment:
`getServerHostName(final Host host)` is deprecated. Please use
`getServerHostName()` instead.
##########
File path:
geode-lucene/src/upgradeTest/java/org/apache/geode/cache/lucene/LuceneQueryWithDifferentVersions.java
##########
@@ -0,0 +1,101 @@
+/*
+ * 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 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().untilAsserted(() -> await().untilAsserted(() -> {
+
assertThat(InternalLocator.getLocator().isSharedConfigurationRunning()).isTrue();
+ })));
+ locator2.invoke(
+ () -> await()
+ .untilAsserted(() -> await().untilAsserted(() -> {
Review comment:
Another chained `await()` call.
--
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:
[email protected]
