You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by on...@apache.org on 2020/10/03 08:02:10 UTC
[geode] 01/01: Revert "GEODE-8536: Allow limited retries when
creating Lucene IndexWriter (#5553)"
This is an automated email from the ASF dual-hosted git repository.
onichols pushed a commit to branch revert-5553-feature/GEODE-8536
in repository https://gitbox.apache.org/repos/asf/geode.git
commit 4811704d027fae2be14591a0b0f65bf70d8a8cba
Author: Owen Nichols <34...@users.noreply.github.com>
AuthorDate: Sat Oct 3 01:01:27 2020 -0700
Revert "GEODE-8536: Allow limited retries when creating Lucene IndexWriter (#5553)"
This reverts commit eccd4f0d867579990a6a5de3dc070d45ef8bb8fb.
---
.../IndexRepositoryFactoryDistributedTest.java | 2 +
.../IndexRepositoryFactoryIntegrationTest.java | 107 ---------------------
.../lucene/internal/IndexRepositoryFactory.java | 41 ++------
.../internal/IndexRepositoryFactoryTest.java | 45 ++-------
4 files changed, 18 insertions(+), 177 deletions(-)
diff --git a/geode-lucene/src/distributedTest/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryDistributedTest.java b/geode-lucene/src/distributedTest/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryDistributedTest.java
index bfe921f..a3f245d 100644
--- a/geode-lucene/src/distributedTest/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryDistributedTest.java
+++ b/geode-lucene/src/distributedTest/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryDistributedTest.java
@@ -25,6 +25,7 @@ import java.io.Serializable;
import java.util.concurrent.TimeUnit;
import java.util.stream.IntStream;
+import junitparams.Parameters;
import org.apache.commons.lang3.RandomStringUtils;
import org.awaitility.core.ConditionTimeoutException;
import org.junit.Before;
@@ -110,6 +111,7 @@ public class IndexRepositoryFactoryDistributedTest implements Serializable {
}
@Test
+ @Parameters()
public void lockedBucketShouldPreventPrimaryFromMoving() {
dataStore1.invoke(this::initDataStoreAndLuceneIndex);
dataStore1.invoke(() -> LuceneTestUtilities.pauseSender(getCache()));
diff --git a/geode-lucene/src/integrationTest/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryIntegrationTest.java b/geode-lucene/src/integrationTest/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryIntegrationTest.java
deleted file mode 100644
index 0506b75..0000000
--- a/geode-lucene/src/integrationTest/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryIntegrationTest.java
+++ /dev/null
@@ -1,107 +0,0 @@
-/*
- * 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 static org.assertj.core.api.Assertions.assertThatThrownBy;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.doThrow;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.times;
-
-import java.io.IOException;
-
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import org.mockito.invocation.InvocationOnMock;
-import org.mockito.stubbing.Answer;
-
-import org.apache.geode.InternalGemFireError;
-import org.apache.geode.cache.Cache;
-import org.apache.geode.cache.CacheFactory;
-import org.apache.geode.cache.Region;
-import org.apache.geode.cache.RegionShortcut;
-import org.apache.geode.cache.execute.FunctionException;
-import org.apache.geode.cache.lucene.LuceneQuery;
-import org.apache.geode.cache.lucene.LuceneQueryException;
-import org.apache.geode.cache.lucene.LuceneServiceProvider;
-import org.apache.geode.cache.lucene.test.TestObject;
-import org.apache.geode.internal.cache.PartitionedRegion;
-
-public class IndexRepositoryFactoryIntegrationTest {
- public Cache cache;
- public static final String INDEX_NAME = "testIndex";
- public static final String REGION_NAME = "testRegion";
- private IndexRepositoryFactory spyFactory;
- private LuceneQuery<Object, Object> luceneQuery;
-
- @Before
- public void setUp() {
- cache = new CacheFactory().create();
- LuceneServiceProvider.get(cache).createIndexFactory().setFields("field1", "field2")
- .create(INDEX_NAME, REGION_NAME);
-
- Region<Object, Object> dataRegion =
- cache.createRegionFactory(RegionShortcut.PARTITION).create(REGION_NAME);
-
- dataRegion.put("A", new TestObject());
-
- spyFactory = spy(new IndexRepositoryFactory());
- PartitionedRepositoryManager.indexRepositoryFactory = spyFactory;
-
- luceneQuery = LuceneServiceProvider.get(cache).createLuceneQueryFactory()
- .create(INDEX_NAME, REGION_NAME, "hello", "field1");
- }
-
- @After
- public void tearDown() {
- if (cache != null) {
- cache.close();
- }
- }
-
- @Test
- public void shouldRetryWhenIOExceptionEncounteredOnceDuringComputingRepository()
- throws IOException, LuceneQueryException {
- // To ensure that the specific bucket used in the query throws the IOException to trigger the
- // retry, throw once for every bucket in the region
- int timesToThrow = ((PartitionedRegion) cache.getRegion(REGION_NAME)).getTotalNumberOfBuckets();
-
- doAnswer(new Answer<Object>() {
- private int times = 0;
-
- @Override
- public Object answer(InvocationOnMock invocation) throws Throwable {
- if (times < timesToThrow) {
- times++;
- throw new IOException();
- }
- return invocation.callRealMethod();
- }
- }).when(spyFactory).getIndexWriter(any(), any());
-
- luceneQuery.findKeys();
- }
-
- @Test
- public void shouldThrowInternalGemfireErrorWhenIOExceptionEncounteredConsistentlyDuringComputingRepository()
- throws IOException {
- doThrow(new IOException()).when(spyFactory).getIndexWriter(any(), any());
-
- assertThatThrownBy(luceneQuery::findKeys).isInstanceOf(FunctionException.class)
- .hasCauseInstanceOf(InternalGemFireError.class);
- }
-}
diff --git a/geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactory.java b/geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactory.java
index 7db8b96..7674a45 100644
--- a/geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactory.java
+++ b/geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactory.java
@@ -44,7 +44,6 @@ public class IndexRepositoryFactory {
private static final Logger logger = LogService.getLogger();
public static final String FILE_REGION_LOCK_FOR_BUCKET_ID = "FileRegionLockForBucketId:";
public static final String APACHE_GEODE_INDEX_COMPLETE = "APACHE_GEODE_INDEX_COMPLETE";
- protected static final int GET_INDEX_WRITER_MAX_ATTEMPTS = 200;
public IndexRepositoryFactory() {}
@@ -75,8 +74,7 @@ public class IndexRepositoryFactory {
* This is a util function just to not let computeIndexRepository be a huge chunk of code.
*/
protected IndexRepository finishComputingRepository(Integer bucketId, LuceneSerializer serializer,
- PartitionedRegion userRegion, IndexRepository oldRepository, InternalLuceneIndex index)
- throws IOException {
+ PartitionedRegion userRegion, IndexRepository oldRepository, InternalLuceneIndex index) {
LuceneIndexForPartitionedRegion indexForPR = (LuceneIndexForPartitionedRegion) index;
final PartitionedRegion fileRegion = indexForPR.getFileAndChunkRegion();
BucketRegion fileAndChunkBucket = getMatchingBucket(fileRegion, bucketId);
@@ -131,7 +129,7 @@ public class IndexRepositoryFactory {
} catch (IOException e) {
logger.warn("Exception thrown while constructing Lucene Index for bucket:" + bucketId
+ " for file region:" + fileAndChunkBucket.getFullPath(), e);
- throw e;
+ return null;
} catch (CacheClosedException e) {
logger.info("CacheClosedException thrown while constructing Lucene Index for bucket:"
+ bucketId + " for file region:" + fileAndChunkBucket.getFullPath());
@@ -146,34 +144,11 @@ public class IndexRepositoryFactory {
protected IndexWriter buildIndexWriter(int bucketId, BucketRegion fileAndChunkBucket,
LuceneIndexForPartitionedRegion indexForPR) throws IOException {
- int attempts = 0;
- // IOExceptions can occur if the fileAndChunk region is being modified while the IndexWriter is
- // being initialized, so allow limited retries here to account for that timing window
- while (true) {
- // bucketTargetingMap handles partition resolver (via bucketId as callbackArg)
- Map<Object, Object> bucketTargetingMap = getBucketTargetingMap(fileAndChunkBucket, bucketId);
- RegionDirectory dir =
- new RegionDirectory(bucketTargetingMap, indexForPR.getFileSystemStats());
- IndexWriterConfig config = new IndexWriterConfig(indexForPR.getAnalyzer());
- try {
- attempts++;
- return getIndexWriter(dir, config);
- } catch (IOException e) {
- if (attempts >= GET_INDEX_WRITER_MAX_ATTEMPTS) {
- throw e;
- }
- logger.info("Encountered {} while attempting to get IndexWriter for index {}. Retrying...",
- e, indexForPR.getName());
- try {
- Thread.sleep(5);
- } catch (InterruptedException ignore) {
- }
- }
- }
- }
+ // bucketTargetingMap handles partition resolver (via bucketId as callbackArg)
+ Map bucketTargetingMap = getBucketTargetingMap(fileAndChunkBucket, bucketId);
+ RegionDirectory dir = new RegionDirectory(bucketTargetingMap, indexForPR.getFileSystemStats());
+ IndexWriterConfig config = new IndexWriterConfig(indexForPR.getAnalyzer());
- protected IndexWriter getIndexWriter(RegionDirectory dir, IndexWriterConfig config)
- throws IOException {
return new IndexWriter(dir, config);
}
@@ -211,8 +186,8 @@ public class IndexRepositoryFactory {
return value;
}
- protected Map<Object, Object> getBucketTargetingMap(BucketRegion region, int bucketId) {
- return new BucketTargetingMap<>(region, bucketId);
+ protected Map getBucketTargetingMap(BucketRegion region, int bucketId) {
+ return new BucketTargetingMap(region, bucketId);
}
protected String getLockName(final BucketRegion fileAndChunkBucket) {
diff --git a/geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryTest.java b/geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryTest.java
index 38e6355..e301dcf 100644
--- a/geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryTest.java
+++ b/geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/IndexRepositoryFactoryTest.java
@@ -14,7 +14,6 @@
*/
package org.apache.geode.cache.lucene.internal;
-import static org.apache.geode.cache.lucene.internal.IndexRepositoryFactory.GET_INDEX_WRITER_MAX_ATTEMPTS;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
@@ -23,13 +22,11 @@ import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.io.IOException;
-import org.apache.lucene.index.IndexWriter;
import org.junit.Before;
import org.junit.Test;
@@ -80,8 +77,7 @@ public class IndexRepositoryFactoryTest {
}
@Test
- public void finishComputingRepositoryShouldReturnNullAndCleanOldRepositoryWhenFileAndChunkBucketIsNull()
- throws IOException {
+ public void finishComputingRepositoryShouldReturnNullAndCleanOldRepositoryWhenFileAndChunkBucketIsNull() {
doReturn(null).when(indexRepositoryFactory).getMatchingBucket(fileRegion, bucketId);
IndexRepository indexRepository = indexRepositoryFactory.finishComputingRepository(0,
@@ -91,8 +87,7 @@ public class IndexRepositoryFactoryTest {
}
@Test
- public void finishComputingRepositoryShouldReturnNullAndCleanOldRepositoryWhenFileAndChunkBucketIsNotPrimary()
- throws IOException {
+ public void finishComputingRepositoryShouldReturnNullAndCleanOldRepositoryWhenFileAndChunkBucketIsNotPrimary() {
when(fileAndChunkBucketAdvisor.isPrimary()).thenReturn(false);
IndexRepository indexRepository = indexRepositoryFactory.finishComputingRepository(0,
@@ -102,8 +97,7 @@ public class IndexRepositoryFactoryTest {
}
@Test
- public void finishComputingRepositoryShouldReturnOldRepositoryWhenNotNullAndNotClosed()
- throws IOException {
+ public void finishComputingRepositoryShouldReturnOldRepositoryWhenNotNullAndNotClosed() {
when(oldRepository.isClosed()).thenReturn(false);
when(fileAndChunkBucketAdvisor.isPrimary()).thenReturn(true);
@@ -114,8 +108,7 @@ public class IndexRepositoryFactoryTest {
}
@Test
- public void finishComputingRepositoryShouldReturnNullWhenLockCanNotBeAcquiredAndFileAndChunkBucketIsNotPrimary()
- throws IOException {
+ public void finishComputingRepositoryShouldReturnNullWhenLockCanNotBeAcquiredAndFileAndChunkBucketIsNotPrimary() {
when(oldRepository.isClosed()).thenReturn(true);
when(fileAndChunkBucketAdvisor.isPrimary()).thenReturn(true).thenReturn(false);
when(distributedLockService.lock(any(), anyLong(), anyLong())).thenReturn(false);
@@ -126,7 +119,7 @@ public class IndexRepositoryFactoryTest {
}
@Test
- public void finishComputingRepositoryShouldThrowExceptionAndReleaseLockWhenIOExceptionIsThrownWhileBuildingTheIndex()
+ public void finishComputingRepositoryShouldReturnNullAndReleaseLockWhenIOExceptionIsThrownWhileBuildingTheIndex()
throws IOException {
when(oldRepository.isClosed()).thenReturn(true);
when(fileAndChunkBucketAdvisor.isPrimary()).thenReturn(true);
@@ -134,8 +127,9 @@ public class IndexRepositoryFactoryTest {
doThrow(new IOException("Test Exception")).when(indexRepositoryFactory)
.buildIndexWriter(bucketId, fileAndChunkBucket, luceneIndex);
- assertThatThrownBy(() -> indexRepositoryFactory.finishComputingRepository(0,
- serializer, userRegion, oldRepository, luceneIndex)).isInstanceOf(IOException.class);
+ IndexRepository indexRepository = indexRepositoryFactory.finishComputingRepository(0,
+ serializer, userRegion, oldRepository, luceneIndex);
+ assertThat(indexRepository).isNull();
verify(distributedLockService).unlock(any());
}
@@ -152,27 +146,4 @@ public class IndexRepositoryFactoryTest {
userRegion, oldRepository, luceneIndex)).isInstanceOf(CacheClosedException.class);
verify(distributedLockService).unlock(any());
}
-
- @Test
- public void buildIndexWriterRetriesCreatingIndexWriterWhenIOExceptionEncountered()
- throws IOException {
- IndexWriter writer = mock(IndexWriter.class);
- doThrow(new IOException()).doReturn(writer).when(indexRepositoryFactory).getIndexWriter(any(),
- any());
- assertThat(indexRepositoryFactory.buildIndexWriter(bucketId, fileAndChunkBucket, luceneIndex))
- .isEqualTo(writer);
- verify(indexRepositoryFactory, times(2)).getIndexWriter(any(), any());
- }
-
- @Test
- public void buildIndexWriterThrowsExceptionWhenIOExceptionConsistentlyEncountered()
- throws IOException {
- IOException testException = new IOException("Test exception");
- doThrow(testException).when(indexRepositoryFactory).getIndexWriter(any(), any());
- assertThatThrownBy(
- () -> indexRepositoryFactory.buildIndexWriter(bucketId, fileAndChunkBucket, luceneIndex))
- .isEqualTo(testException);
- verify(indexRepositoryFactory, times(GET_INDEX_WRITER_MAX_ATTEMPTS)).getIndexWriter(any(),
- any());
- }
}