Skip to content

Commit

Permalink
Prevent accidental deletion of files
Browse files Browse the repository at this point in the history
  • Loading branch information
anujshahwork authored and anujshahwork committed Aug 18, 2015
1 parent e222c12 commit a31e93c
Show file tree
Hide file tree
Showing 11 changed files with 198 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,8 @@ public interface ExternalizerIds {
*/
static final Integer FILE_LIST_DELTA_DEL = 1308;

/**
* @see org.infinispan.lucene.FileDeleteMarkerKey.Externalizer
*/
static final Integer FILE_DELETE_MARKER_KEY = 1309;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package org.infinispan.lucene;

import java.io.IOException;
import java.io.ObjectInput;
import java.io.ObjectOutput;
import java.util.Set;

import org.infinispan.commons.marshall.AbstractExternalizer;
import org.infinispan.commons.util.Util;

/**
* Lucene's index segment files are chunked, for safe deletion of elements a read lock is
* implemented so that all chunks are deleted only after the usage counter is decremented to zero.
* FileDeleteMarkerKey is used as a key for the reference counters; a special purpose key was needed to
* make atomic operation possible.
*/
public class FileDeleteMarkerKey implements IndexScopedKey {

private final String indexName;
private final String fileName;
private final int hashCode;

public FileDeleteMarkerKey(final String indexName, final String fileName) {
if (indexName == null)
throw new IllegalArgumentException("indexName shall not be null");
if (fileName == null)
throw new IllegalArgumentException("fileName shall not be null");
this.indexName = indexName;
this.fileName = fileName;
this.hashCode = generateHashCode();
}

/**
* Get the indexName.
*
* @return the indexName.
*/
@Override
public String getIndexName() {
return indexName;
}

/**
* Get the fileName.
*
* @return the fileName.
*/
public String getFileName() {
return fileName;
}

@Override
public Object accept(KeyVisitor visitor) throws Exception {
return visitor.visit(this);
}

@Override
public int hashCode() {
return hashCode;
}

private int generateHashCode() {
final int prime = 31;
int result = prime + fileName.hashCode();
return prime * result + indexName.hashCode();
}

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null)
return false;
if (FileDeleteMarkerKey.class != obj.getClass())
return false;
FileDeleteMarkerKey other = (FileDeleteMarkerKey) obj;
return fileName.equals(other.fileName) && indexName.equals(other.indexName);
}

@Override
public String toString() {
return fileName + "|D|"+ indexName;
}

public static final class Externalizer extends AbstractExternalizer<FileDeleteMarkerKey> {

@Override
public void writeObject(final ObjectOutput output, final FileDeleteMarkerKey key) throws IOException {
output.writeUTF(key.indexName);
output.writeUTF(key.fileName);
}

@Override
public FileDeleteMarkerKey readObject(final ObjectInput input) throws IOException {
String indexName = input.readUTF();
String fileName = input.readUTF();
return new FileDeleteMarkerKey(indexName, fileName);
}

@Override
public Integer getId() {
return ExternalizerIds.FILE_DELETE_MARKER_KEY;
}

@Override
public Set<Class<? extends FileDeleteMarkerKey>> getTypeClasses() {
return Util.<Class<? extends FileDeleteMarkerKey>>asSet(FileDeleteMarkerKey.class);
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,6 @@ public interface KeyVisitor<T> {

T visit(FileReadLockKey fileReadLockKey) throws Exception;

T visit(FileDeleteMarkerKey fileDeleteMarkerKey) throws Exception;

}
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,18 @@
import org.apache.lucene.store.IOContext;
import org.apache.lucene.store.IndexInput;
import org.infinispan.commons.marshall.StreamingMarshaller;
import org.infinispan.marshall.core.MarshalledEntryImpl;
import org.infinispan.marshall.core.MarshalledEntry;
import org.infinispan.lucene.ChunkCacheKey;
import org.infinispan.lucene.FileCacheKey;
import org.infinispan.lucene.FileDeleteMarkerKey;
import org.infinispan.lucene.FileListCacheKey;
import org.infinispan.lucene.FileMetadata;
import org.infinispan.lucene.FileReadLockKey;
import org.infinispan.lucene.IndexScopedKey;
import org.infinispan.lucene.KeyVisitor;
import org.infinispan.lucene.impl.FileListCacheValue;
import org.infinispan.lucene.logging.Log;
import org.infinispan.marshall.core.MarshalledEntry;
import org.infinispan.marshall.core.MarshalledEntryImpl;
import org.infinispan.util.logging.LogFactory;

/**
Expand Down Expand Up @@ -294,6 +295,12 @@ public Object visit(final FileReadLockKey fileReadLockKey) {
//ReadLocks should not leak to the actual storage
return null;
}

@Override
public Object visit(FileDeleteMarkerKey fileDeleteMarkerKey) throws Exception {
//Delete markers should not leak to the actual storage
return null;
}
}

/**
Expand Down Expand Up @@ -323,6 +330,12 @@ public Boolean visit(final FileReadLockKey fileReadLockKey) {
//ReadLocks should not leak to the actual storage
return Boolean.FALSE;
}

@Override
public Boolean visit(FileDeleteMarkerKey fileDeleteMarkerKey) throws Exception {
//Delete markers should not leak to the actual storage
return Boolean.FALSE;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ boolean fileExists(final String name) {

void deleteFile(final String name) {
fileOps.deleteFileName(name);
readLocks.deleteOrReleaseReadLock(name);
readLocks.markForDeletion(name);
if (log.isDebugEnabled()) {
log.debugf("Removed file: %s from index: %s", name, indexName);
}
Expand Down Expand Up @@ -91,7 +91,7 @@ void renameFile(final String from, final String to) {
fileOps.removeAndAdd(from, to);

// now trigger deletion of old file chunks:
readLocks.deleteOrReleaseReadLock(from);
readLocks.markForDeletion(from);
if (log.isTraceEnabled()) {
log.tracef("Renamed file from: %s to: %s in index %s", from, to, indexName);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void close() {
currentLoadedChunk = -1;
buffer = null;
if (isClone) return;
readLocks.deleteOrReleaseReadLock(filename);
readLocks.releaseReadLock(filename);
if (trace) {
log.tracef("Closed IndexInput for file:%s in index: %s", filename, fileKey.getIndexName());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.infinispan.context.Flag;
import org.infinispan.lucene.ChunkCacheKey;
import org.infinispan.lucene.FileCacheKey;
import org.infinispan.lucene.FileDeleteMarkerKey;
import org.infinispan.lucene.FileMetadata;
import org.infinispan.lucene.FileReadLockKey;
import org.infinispan.util.logging.Log;
Expand Down Expand Up @@ -67,7 +68,7 @@ public DistributedSegmentReadLocker(Cache<?, ?> cache, String indexName) {
* @see org.apache.lucene.store.Directory#deleteFile(String)
*/
@Override
public void deleteOrReleaseReadLock(final String filename) {
public void releaseReadLock(final String filename) {
if (isMultiChunked(filename)) {
int newValue = 0;
FileReadLockKey readLockKey = new FileReadLockKey(indexName, filename);
Expand Down Expand Up @@ -173,7 +174,7 @@ public boolean acquireReadLock(String filename) {
* The {@link org.apache.lucene.store.Directory#deleteFile(String)} is not deleting the elements from the cache
* but instead flagging the file as deletable.
* This method will really remove the elements from the cache; should be invoked only
* by {@link #deleteOrReleaseReadLock(String)} after having verified that there
* by {@link #releaseReadLock(String)} after having verified that there
* are no users left in need to read these chunks.
*
* @param indexName the index name to delete the file from
Expand All @@ -188,6 +189,12 @@ static void realFileDelete(String indexName, String fileName, AdvancedCache<Obje
final boolean trace = log.isTraceEnabled();
final FileCacheKey key = new FileCacheKey(indexName, fileName);
if (trace) log.tracef("deleting metadata: %s", key);

if (locksCache.remove(new FileDeleteMarkerKey(indexName, fileName)) == null) {
log.warn("Attempt made to delete file [" + fileName + "] from index [" + indexName + "] but no delete marker was present.");
return;
}

final FileMetadata file = (FileMetadata) metadataCache.remove(key);
if (file != null) { //during optimization of index a same file could be deleted twice, so you could see a null here
final int bufferSize = file.getBufferSize();
Expand Down Expand Up @@ -221,4 +228,18 @@ private static void verifyCacheHasNoEviction(AdvancedCache<?, ?> cache) {
throw new IllegalArgumentException("DistributedSegmentReadLocker is not reliable when using a cache with eviction enabled, disable eviction on this cache instance");
}


/**
* Adds a value to the locks cache to indicate that the file is in need of deleting.
* An actual file delete may not occur until all read locks are released.
* @see InfinispanDirectory#deleteFile(String)
*/
@Override
public void markForDeletion(String fileName) {
FileDeleteMarkerKey key = new FileDeleteMarkerKey(indexName, fileName);
if (log.isTraceEnabled()) log.tracef("Marking for deletion: %s", key);

locksCache.put(key, 1);
releaseReadLock(fileName);
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package org.infinispan.lucene.readlocks;

import org.infinispan.Cache;
import java.util.HashMap;

import org.infinispan.Cache;

/**
* LocalLockMergingSegmentReadLocker decorates the {@link DistributedSegmentReadLocker} to minimize
* remote operations in case several IndexReaders are opened on the same Infinispan based {@link org.apache.lucene.store.Directory}.
Expand Down Expand Up @@ -73,7 +74,7 @@ private LocalReadLock getLocalLockByName(String name) {
* {@inheritDoc}
*/
@Override
public synchronized void deleteOrReleaseReadLock(String name) {
public synchronized void releaseReadLock(String name) {
getLocalLockByName(name).release();
}

Expand Down Expand Up @@ -112,9 +113,14 @@ void release() {
value--;
if (value <= 0) {
localLocks.remove(name);
delegate.deleteOrReleaseReadLock(name);
delegate.releaseReadLock(name);
}
}
}


@Override
public void markForDeletion(String fileName) {
delegate.markForDeletion(fileName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,15 @@ public boolean acquireReadLock(String filename) {
* doesn't do anything
*/
@Override
public void deleteOrReleaseReadLock(String filename) {
public void releaseReadLock(String filename) {
return;
}

/**
* doesn't do anything
*/
@Override
public void markForDeletion(String fileName) {
return;
}
}
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package org.infinispan.lucene.readlocks;



/**
* <p>SegmentReadLocker implementations have to make sure that segments are not deleted while they are
* being used by an IndexReader.</p>
* <p>When an {@link org.infinispan.lucene.impl.InfinispanIndexInput} is opened on a file which is split in smaller chunks,
* {@link #acquireReadLock(String)} is invoked; then the {@link #deleteOrReleaseReadLock(String)} is
* <p>When an {@link org.infinispan.lucene.InfinispanIndexInput} is opened on a file which is split in smaller chunks,
* {@link #acquireReadLock(String)} is invoked; then the {@link #releaseReadLock(String)} is
* invoked when the stream is closed.</p>
* <p>The same {@link #deleteOrReleaseReadLock(String)} is invoked when a file is deleted, so if this invocation is not balancing
* a lock acquire this implementation must delete all segment chunks and the associated metadata.</p>
* <p>{@link #markForDeletion(String)} is invoked when a file is deleted, so {@link #releaseReadLock(String)}
* is invoked for the last time this implementation must delete all segment chunks and the associated metadata.</p>
* <p>Note that if you can use and tune the {@link org.apache.lucene.index.LogByteSizeMergePolicy} you could avoid the need
* for readlocks by setting a maximum segment size to equal the chunk size used by the InfinispanDirectory; readlocks
* will be skipped automatically when not needed, so it's advisable to still configure an appropriate SegmentReadLocker
Expand All @@ -18,25 +20,31 @@
*/
public interface SegmentReadLocker {

/**
* Set file to be deleted when all acquired read locks have been released. The file will be deleted immediately
* if the file is not locked.
* @param fileName of the file to mark for deletion
* @see InfinispanDirectory#deleteFile(String)
*/
void markForDeletion(String fileName);

/**
* It will release a previously acquired readLock, or
* if no readLock was acquired it will mark the file to be deleted as soon
* as all pending locks are releases.
* If it's invoked on a file without pending locks the file is deleted.
* It will release a previously acquired readLock.
* If it's invoked on a file without pending locks and the file is marked for deletion it will be deleted.
*
* @param fileName of the file to release or delete
* @see org.apache.lucene.store.Directory#deleteFile(String)
* @see markForDeletion()
*/
void deleteOrReleaseReadLock(String fileName);
void releaseReadLock(String fileName);

/**
* Acquires a readlock, in order to prevent other invocations to {@link #deleteOrReleaseReadLock(String)}
* Acquires a readlock, in order to prevent other invocations to {@link #releaseReadLock(String)}
* from deleting the file.
*
* @param filename of the file to acquire the lock at
* @param filename
* @return true if the lock was acquired, false if the implementation
* detects the file does not exist, or that it's being deleted by some other thread.
* @see org.apache.lucene.store.Directory#openInput
* @see InfinispanDirectory#openInput(String)
*/
boolean acquireReadLock(String filename);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
import org.infinispan.manager.EmbeddedCacheManager;
import org.infinispan.test.SingleCacheManagerTest;
import org.infinispan.test.fwk.TestCacheManagerFactory;
import org.testng.annotations.Test;
import org.testng.AssertJUnit;
import org.testng.annotations.Test;

/**
* Stress test for {@link org.infinispan.lucene.readlocks.LocalLockMergingSegmentReadLocker}. See also ISPN-4497 for an
Expand Down Expand Up @@ -57,7 +57,7 @@ public void run() {

locker.acquireReadLock("fileName");
Thread.sleep(2);
locker.deleteOrReleaseReadLock("fileName");
locker.releaseReadLock("fileName");

// Take a break every now and a again to try and avoid the same LocalReadLock being used constantly
if (counter++ % 900 == 0) {
Expand Down

0 comments on commit a31e93c

Please sign in to comment.