honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

AbstractBlobContainer.deleteByPrefix() should not list all blobs 

The current implementation of AbstractBlobContainer.deleteByPrefix() calls AbstractBlobContainer.deleteBlobsByFilter() which calls BlobContainer.listBlobs() for deleting files, resulting in loading all files in order to delete few of them. This can be improved by calling BlobContainer.listBlobsByPrefix() directly.

This problem happened in #10344 when the repository verification process tries to delete a blob prefixed by "tests-" to ensure that the repository is accessible for the node. When doing so we have the following calling graph: BlobStoreRepository.endVerification() -> BlobContainer.deleteByPrefix() -> AbstractBlobContainer.deleteByPrefix() -> AbstractBlobContainer.deleteBlobsByFilter() -> BlobContainer.listBlobs()... and boom.

Also, AbstractBlobContainer.listBlobsByPrefix() and BlobContainer.deleteBlobsByFilter() can be removed because it has the same drawbacks as AbstractBlobContainer.deleteByPrefix() and also lists all blobs. Listing blobs by prefix can be done at the FsBlobContainer level.

Related to #10344
This commit is contained in:
Tanguy Leroux 2015-04-01 14:05:10 +02:00
parent 110b4c9d81
commit a598123838
6 changed files with 30 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java
View File

@ -30,13 +30,6 @@ import java.io.OutputStream;
*/
public interface BlobContainer {
interface BlobNameFilter {
/**
* Return <tt>false</tt> if the blob should be filtered.
*/
boolean accept(String blobName);
}
BlobPath path();
boolean blobExists(String blobName);
@ -63,11 +56,6 @@ public interface BlobContainer {
*/
void deleteBlobsByPrefix(String blobNamePrefix) throws IOException;
/**
* Deletes all blobs in the container that match the supplied filter.
*/
void deleteBlobsByFilter(BlobNameFilter filter) throws IOException;
/**
* Lists all blobs in the container
*/

27
src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java
View File

@ -26,10 +26,12 @@ import org.elasticsearch.common.blobstore.BlobPath;
import org.elasticsearch.common.blobstore.support.AbstractBlobContainer;
import org.elasticsearch.common.blobstore.support.PlainBlobMetaData;
import org.elasticsearch.common.collect.MapBuilder;
import org.elasticsearch.common.io.FileSystemUtils;
import java.io.*;
import java.nio.file.*;
import java.nio.file.DirectoryStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.nio.file.attribute.BasicFileAttributes;
/**
@ -49,16 +51,21 @@ public class FsBlobContainer extends AbstractBlobContainer {
@Override
public ImmutableMap<String, BlobMetaData> listBlobs() throws IOException {
Path[] files = FileSystemUtils.files(path);
if (files.length == 0) {
return ImmutableMap.of();
}
return listBlobsByPrefix(null);
}
@Override
public ImmutableMap<String, BlobMetaData> listBlobsByPrefix(String blobNamePrefix) throws IOException {
// using MapBuilder and not ImmutableMap.Builder as it seems like File#listFiles might return duplicate files!
MapBuilder<String, BlobMetaData> builder = MapBuilder.newMapBuilder();
for (Path file : files) {
final BasicFileAttributes attrs = Files.readAttributes(file, BasicFileAttributes.class);
if (attrs.isRegularFile()) {
builder.put(file.getFileName().toString(), new PlainBlobMetaData(file.getFileName().toString(), attrs.size()));
blobNamePrefix = blobNamePrefix == null ? "" : blobNamePrefix;
try (DirectoryStream<Path> stream = Files.newDirectoryStream(path, blobNamePrefix + "*")) {
for (Path file : stream) {
final BasicFileAttributes attrs = Files.readAttributes(file, BasicFileAttributes.class);
if (attrs.isRegularFile()) {
builder.put(file.getFileName().toString(), new PlainBlobMetaData(file.getFileName().toString(), attrs.size()));
}
}
}
return builder.immutableMap();

31
src/main/java/org/elasticsearch/common/blobstore/support/AbstractBlobContainer.java
View File

@ -24,7 +24,7 @@ import org.elasticsearch.common.blobstore.BlobContainer;
import org.elasticsearch.common.blobstore.BlobMetaData;
import org.elasticsearch.common.blobstore.BlobPath;
import java.io.*;
import java.io.IOException;
/**
*
@ -42,36 +42,11 @@ public abstract class AbstractBlobContainer implements BlobContainer {
return this.path;
}
@Override
public ImmutableMap<String, BlobMetaData> listBlobsByPrefix(String blobNamePrefix) throws IOException {
ImmutableMap<String, BlobMetaData> allBlobs = listBlobs();
ImmutableMap.Builder<String, BlobMetaData> blobs = ImmutableMap.builder();
for (BlobMetaData blob : allBlobs.values()) {
if (blob.name().startsWith(blobNamePrefix)) {
blobs.put(blob.name(), blob);
}
}
return blobs.build();
}
@Override
public void deleteBlobsByPrefix(final String blobNamePrefix) throws IOException {
deleteBlobsByFilter(new BlobNameFilter() {
@Override
public boolean accept(String blobName) {
return blobName.startsWith(blobNamePrefix);
}
});
}
@Override
public void deleteBlobsByFilter(BlobNameFilter filter) throws IOException {
ImmutableMap<String, BlobMetaData> blobs = listBlobs();
ImmutableMap<String, BlobMetaData> blobs = listBlobsByPrefix(blobNamePrefix);
for (BlobMetaData blob : blobs.values()) {
if (filter.accept(blob.name())) {
deleteBlob(blob.name());
}
deleteBlob(blob.name());
}
}
}

8
src/main/java/org/elasticsearch/common/blobstore/url/URLBlobContainer.java
View File

@ -69,6 +69,14 @@ public class URLBlobContainer extends AbstractBlobContainer {
throw new UnsupportedOperationException("URL repository doesn't support this operation");
}
/**
* This operation is not supported by URLBlobContainer
*/
@Override
public ImmutableMap<String, BlobMetaData> listBlobsByPrefix(String blobNamePrefix) throws IOException {
throw new UnsupportedOperationException("URL repository doesn't support this operation");
}
@Override
public void move(String from, String to) throws IOException {
throw new UnsupportedOperationException("URL repository doesn't support this operation");

7
src/test/java/org/elasticsearch/snapshots/mockstore/BlobContainerWrapper.java
View File

@ -19,9 +19,9 @@
package org.elasticsearch.snapshots.mockstore;
import com.google.common.collect.ImmutableMap;
import org.elasticsearch.common.blobstore.BlobContainer;
import org.elasticsearch.common.blobstore.BlobMetaData;
import org.elasticsearch.common.blobstore.BlobPath;
import org.elasticsearch.common.blobstore.BlobContainer;
import java.io.IOException;
import java.io.InputStream;
@ -67,11 +67,6 @@ public class BlobContainerWrapper implements BlobContainer {
delegate.deleteBlobsByPrefix(blobNamePrefix);
}
@Override
public void deleteBlobsByFilter(BlobNameFilter filter) throws IOException {
delegate.deleteBlobsByFilter(filter);
}
@Override
public ImmutableMap<String, BlobMetaData> listBlobs() throws IOException {
return delegate.listBlobs();

10
src/test/java/org/elasticsearch/snapshots/mockstore/MockRepository.java
View File

@ -21,15 +21,14 @@ package org.elasticsearch.snapshots.mockstore;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.cluster.ClusterService;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.cluster.metadata.SnapshotId;
import org.elasticsearch.common.blobstore.BlobContainer;
import org.elasticsearch.common.blobstore.BlobMetaData;
import org.elasticsearch.common.blobstore.BlobPath;
import org.elasticsearch.common.blobstore.BlobStore;
import org.elasticsearch.common.blobstore.BlobContainer;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.snapshots.IndexShardRepository;
@ -49,7 +48,6 @@ import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicLong;
import static com.carrotsearch.randomizedtesting.RandomizedTest.randomAsciiOfLength;
import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
/**
@ -291,12 +289,6 @@ public class MockRepository extends FsRepository {
super.deleteBlobsByPrefix(blobNamePrefix);
}
@Override
public void deleteBlobsByFilter(BlobNameFilter filter) throws IOException {
maybeIOExceptionOrBlock("");
super.deleteBlobsByFilter(filter);
}
@Override
public ImmutableMap<String, BlobMetaData> listBlobs() throws IOException {
maybeIOExceptionOrBlock("");