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

Remove Custom Listeners from SnapshotsService (#37629) 

* Remove Custom Listeners from SnapshotsService

Motivations:
    * Shorten the code some more
    * Use ActionListener#wrap to get easy to reason about behavior in failure scenarios
    * Remove duplication in the logic of handling snapshot completion listeners (listeners removing themselves and comparing snapshots to their targets)
        * Also here, move all listener handling into `SnapshotsService` and remove custom listener class by putting listeners in a map
This commit is contained in:
Armin Braun 2019-01-24 10:11:18 +01:00 committed by GitHub
parent bdef2ab8c0
commit 36889e8a2f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 72 additions and 161 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/create/TransportCreateSnapshotAction.java
View File

@ -28,8 +28,6 @@ import org.elasticsearch.cluster.block.ClusterBlockLevel;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.snapshots.Snapshot;
import org.elasticsearch.snapshots.SnapshotInfo;
import org.elasticsearch.snapshots.SnapshotsService; import org.elasticsearch.snapshots.SnapshotsService;
import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService; import org.elasticsearch.transport.TransportService;
@ -72,37 +70,13 @@ public class TransportCreateSnapshotAction extends TransportMasterNodeAction<Cre
@Override @Override
protected void masterOperation(final CreateSnapshotRequest request, ClusterState state, protected void masterOperation(final CreateSnapshotRequest request, ClusterState state,
final ActionListener<CreateSnapshotResponse> listener) { final ActionListener<CreateSnapshotResponse> listener) {
snapshotsService.createSnapshot(request, new SnapshotsService.CreateSnapshotListener() { if (request.waitForCompletion()) {
@Override snapshotsService.executeSnapshot(request,
public void onResponse(Snapshot snapshotCreated) { ActionListener.wrap(snapshotInfo-> listener.onResponse(new CreateSnapshotResponse(snapshotInfo)), listener::onFailure));
if (request.waitForCompletion()) { } else {
snapshotsService.addListener(new SnapshotsService.SnapshotCompletionListener() { snapshotsService.createSnapshot(request,
@Override ActionListener.wrap(snapshot -> listener.onResponse(new CreateSnapshotResponse()), listener::onFailure));
public void onSnapshotCompletion(Snapshot snapshot, SnapshotInfo snapshotInfo) { }
if (snapshotCreated.equals(snapshot)) {
listener.onResponse(new CreateSnapshotResponse(snapshotInfo));
snapshotsService.removeListener(this);
}
}
@Override
public void onSnapshotFailure(Snapshot snapshot, Exception e) {
if (snapshotCreated.equals(snapshot)) {
listener.onFailure(e);
snapshotsService.removeListener(this);
}
}
});
} else {
listener.onResponse(new CreateSnapshotResponse());
}
}
@Override
public void onFailure(Exception e) {
listener.onFailure(e);
}
});
} }
} }

13
server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/delete/TransportDeleteSnapshotAction.java
View File

@ -67,16 +67,7 @@ public class TransportDeleteSnapshotAction extends TransportMasterNodeAction<Del
@Override @Override
protected void masterOperation(final DeleteSnapshotRequest request, ClusterState state, protected void masterOperation(final DeleteSnapshotRequest request, ClusterState state,
final ActionListener<AcknowledgedResponse> listener) { final ActionListener<AcknowledgedResponse> listener) {
snapshotsService.deleteSnapshot(request.repository(), request.snapshot(), new SnapshotsService.DeleteSnapshotListener() { snapshotsService.deleteSnapshot(request.repository(), request.snapshot(),
@Override ActionListener.wrap(v -> listener.onResponse(new AcknowledgedResponse(true)), listener::onFailure), false);
public void onResponse() {
listener.onResponse(new AcknowledgedResponse(true));
}
@Override
public void onFailure(Exception e) {
listener.onFailure(e);
}
}, false);
} }
} }

178
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
View File

@ -80,6 +80,7 @@ import java.util.Locale;
import java.util.Map; import java.util.Map;
import java.util.Optional; import java.util.Optional;
import java.util.Set; import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CopyOnWriteArrayList;
import java.util.stream.Collectors; import java.util.stream.Collectors;
@ -91,10 +92,10 @@ import static org.elasticsearch.cluster.SnapshotsInProgress.completed;
* <p> * <p>
* A typical snapshot creating process looks like this: * A typical snapshot creating process looks like this:
* <ul> * <ul>
* <li>On the master node the {@link #createSnapshot(CreateSnapshotRequest, CreateSnapshotListener)} is called and makes sure that * <li>On the master node the {@link #createSnapshot(CreateSnapshotRequest, ActionListener)} is called and makes sure that
* no snapshot is currently running and registers the new snapshot in cluster state</li> * no snapshot is currently running and registers the new snapshot in cluster state</li>
* <li>When cluster state is updated * <li>When cluster state is updated
* the {@link #beginSnapshot(ClusterState, SnapshotsInProgress.Entry, boolean, CreateSnapshotListener)} method kicks in and initializes * the {@link #beginSnapshot(ClusterState, SnapshotsInProgress.Entry, boolean, ActionListener)} method kicks in and initializes
* the snapshot in the repository and then populates list of shards that needs to be snapshotted in cluster state</li> * the snapshot in the repository and then populates list of shards that needs to be snapshotted in cluster state</li>
* <li>Each data node is watching for these shards and when new shards scheduled for snapshotting appear in the cluster state, data nodes * <li>Each data node is watching for these shards and when new shards scheduled for snapshotting appear in the cluster state, data nodes
* start processing them through {@link SnapshotShardsService#processIndexShardSnapshots(ClusterChangedEvent)} method</li> * start processing them through {@link SnapshotShardsService#processIndexShardSnapshots(ClusterChangedEvent)} method</li>
@ -118,7 +119,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
private final ThreadPool threadPool; private final ThreadPool threadPool;
private final CopyOnWriteArrayList<SnapshotCompletionListener> snapshotCompletionListeners = new CopyOnWriteArrayList<>(); private final Map<Snapshot, List<ActionListener<SnapshotInfo>>> snapshotCompletionListeners = new ConcurrentHashMap<>();
@Inject @Inject
public SnapshotsService(Settings settings, ClusterService clusterService, IndexNameExpressionResolver indexNameExpressionResolver, public SnapshotsService(Settings settings, ClusterService clusterService, IndexNameExpressionResolver indexNameExpressionResolver,
@ -225,6 +226,17 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
return Collections.unmodifiableList(snapshotList); return Collections.unmodifiableList(snapshotList);
} }
/**
* Same as {@link #createSnapshot(CreateSnapshotRequest, ActionListener)} but invokes its callback on completion of
* the snapshot.
*
* @param request snapshot request
* @param listener snapshot completion listener
*/
public void executeSnapshot(final CreateSnapshotRequest request, final ActionListener<SnapshotInfo> listener) {
createSnapshot(request, ActionListener.wrap(snapshot -> addListener(snapshot, listener), listener::onFailure));
}
/** /**
* Initializes the snapshotting process. * Initializes the snapshotting process.
* <p> * <p>
@ -234,7 +246,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
* @param request snapshot request * @param request snapshot request
* @param listener snapshot creation listener * @param listener snapshot creation listener
*/ */
public void createSnapshot(final CreateSnapshotRequest request, final CreateSnapshotListener listener) { public void createSnapshot(final CreateSnapshotRequest request, final ActionListener<Snapshot> listener) {
final String repositoryName = request.repository(); final String repositoryName = request.repository();
final String snapshotName = indexNameExpressionResolver.resolveDateMathExpression(request.snapshot()); final String snapshotName = indexNameExpressionResolver.resolveDateMathExpression(request.snapshot());
validate(repositoryName, snapshotName); validate(repositoryName, snapshotName);
@ -351,7 +363,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
private void beginSnapshot(final ClusterState clusterState, private void beginSnapshot(final ClusterState clusterState,
final SnapshotsInProgress.Entry snapshot, final SnapshotsInProgress.Entry snapshot,
final boolean partial, final boolean partial,
final CreateSnapshotListener userCreateSnapshotListener) { final ActionListener<Snapshot> userCreateSnapshotListener) {
threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(new AbstractRunnable() { threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(new AbstractRunnable() {
boolean snapshotCreated; boolean snapshotCreated;
@ -491,11 +503,11 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
private final SnapshotsInProgress.Entry snapshot; private final SnapshotsInProgress.Entry snapshot;
private final boolean snapshotCreated; private final boolean snapshotCreated;
private final CreateSnapshotListener userCreateSnapshotListener; private final ActionListener<Snapshot> userCreateSnapshotListener;
private final Exception e; private final Exception e;
CleanupAfterErrorListener(SnapshotsInProgress.Entry snapshot, boolean snapshotCreated, CleanupAfterErrorListener(SnapshotsInProgress.Entry snapshot, boolean snapshotCreated,
CreateSnapshotListener userCreateSnapshotListener, Exception e) { ActionListener<Snapshot> userCreateSnapshotListener, Exception e) {
this.snapshot = snapshot; this.snapshot = snapshot;
this.snapshotCreated = snapshotCreated; this.snapshotCreated = snapshotCreated;
this.userCreateSnapshotListener = userCreateSnapshotListener; this.userCreateSnapshotListener = userCreateSnapshotListener;
@ -781,9 +793,9 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
entries.add(updatedSnapshot); entries.add(updatedSnapshot);
// Clean up the snapshot that failed to start from the old master // Clean up the snapshot that failed to start from the old master
deleteSnapshot(snapshot.snapshot(), new DeleteSnapshotListener() { deleteSnapshot(snapshot.snapshot(), new ActionListener<Void>() {
@Override @Override
public void onResponse() { public void onResponse(Void aVoid) {
logger.debug("cleaned up abandoned snapshot {} in INIT state", snapshot.snapshot()); logger.debug("cleaned up abandoned snapshot {} in INIT state", snapshot.snapshot());
} }
@ -1077,15 +1089,16 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
@Override @Override
public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) {
for (SnapshotCompletionListener listener : snapshotCompletionListeners) { final List<ActionListener<SnapshotInfo>> completionListeners = snapshotCompletionListeners.remove(snapshot);
if (completionListeners != null) {
try { try {
if (snapshotInfo != null) { if (snapshotInfo == null) {
listener.onSnapshotCompletion(snapshot, snapshotInfo); ActionListener.onFailure(completionListeners, failure);
} else { } else {
listener.onSnapshotFailure(snapshot, failure); ActionListener.onResponse(completionListeners, snapshotInfo);
} }
} catch (Exception t) { } catch (Exception e) {
logger.warn(() -> new ParameterizedMessage("failed to notify listener [{}]", listener), t); logger.warn("Failed to notify listeners", e);
} }
} }
if (listener != null) { if (listener != null) {
@ -1103,7 +1116,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
* @param snapshotName snapshotName * @param snapshotName snapshotName
* @param listener listener * @param listener listener
*/ */
public void deleteSnapshot(final String repositoryName, final String snapshotName, final DeleteSnapshotListener listener, public void deleteSnapshot(final String repositoryName, final String snapshotName, final ActionListener<Void> listener,
final boolean immediatePriority) { final boolean immediatePriority) {
// First, look for the snapshot in the repository // First, look for the snapshot in the repository
final Repository repository = repositoriesService.repository(repositoryName); final Repository repository = repositoriesService.repository(repositoryName);
@ -1143,7 +1156,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
* @param listener listener * @param listener listener
* @param repositoryStateId the unique id for the state of the repository * @param repositoryStateId the unique id for the state of the repository
*/ */
private void deleteSnapshot(final Snapshot snapshot, final DeleteSnapshotListener listener, final long repositoryStateId, private void deleteSnapshot(final Snapshot snapshot, final ActionListener<Void> listener, final long repositoryStateId,
final boolean immediatePriority) { final boolean immediatePriority) {
Priority priority = immediatePriority ? Priority.IMMEDIATE : Priority.NORMAL; Priority priority = immediatePriority ? Priority.IMMEDIATE : Priority.NORMAL;
clusterService.submitStateUpdateTask("delete snapshot", new ClusterStateUpdateTask(priority) { clusterService.submitStateUpdateTask("delete snapshot", new ClusterStateUpdateTask(priority) {
@ -1234,8 +1247,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
} }
} }
SnapshotsInProgress.Entry newSnapshot = new SnapshotsInProgress.Entry(snapshotEntry, State.ABORTED, shards); SnapshotsInProgress.Entry newSnapshot = new SnapshotsInProgress.Entry(snapshotEntry, State.ABORTED, shards);
snapshots = new SnapshotsInProgress(newSnapshot); clusterStateBuilder.putCustom(SnapshotsInProgress.TYPE, new SnapshotsInProgress(newSnapshot));
clusterStateBuilder.putCustom(SnapshotsInProgress.TYPE, snapshots);
} }
return clusterStateBuilder.build(); return clusterStateBuilder.build();
} }
@ -1249,50 +1261,34 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) {
if (waitForSnapshot) { if (waitForSnapshot) {
logger.trace("adding snapshot completion listener to wait for deleted snapshot to finish"); logger.trace("adding snapshot completion listener to wait for deleted snapshot to finish");
addListener(new SnapshotCompletionListener() { addListener(snapshot, ActionListener.wrap(
@Override snapshotInfo -> {
public void onSnapshotCompletion(Snapshot completedSnapshot, SnapshotInfo snapshotInfo) { logger.debug("deleted snapshot completed - deleting files");
if (completedSnapshot.equals(snapshot)) { threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(() -> {
logger.debug("deleted snapshot completed - deleting files");
removeListener(this);
threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(() -> {
try {
deleteSnapshot(completedSnapshot.getRepository(), completedSnapshot.getSnapshotId().getName(),
listener, true);
} catch (Exception ex) {
logger.warn(() ->
new ParameterizedMessage("[{}] failed to delete snapshot", snapshot), ex);
}
}
);
}
}
@Override
public void onSnapshotFailure(Snapshot failedSnapshot, Exception e) {
if (failedSnapshot.equals(snapshot)) {
logger.warn("deleted snapshot failed - deleting files", e);
removeListener(this);
threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(() -> {
try { try {
deleteSnapshot(failedSnapshot.getRepository(), deleteSnapshot(snapshot.getRepository(), snapshot.getSnapshotId().getName(), listener, true);
failedSnapshot.getSnapshotId().getName(), } catch (Exception ex) {
listener, logger.warn(() -> new ParameterizedMessage("[{}] failed to delete snapshot", snapshot), ex);
true);
} catch (SnapshotMissingException smex) {
logger.info(() -> new ParameterizedMessage(
"Tried deleting in-progress snapshot [{}], but it " +
"could not be found after failing to abort.",
smex.getSnapshotName()), e);
listener.onFailure(new SnapshotException(snapshot,
"Tried deleting in-progress snapshot [" + smex.getSnapshotName() + "], but it " +
"could not be found after failing to abort.", smex));
} }
}); }
} );
},
e -> {
logger.warn("deleted snapshot failed - deleting files", e);
threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(() -> {
try {
deleteSnapshot(snapshot.getRepository(), snapshot.getSnapshotId().getName(), listener, true);
} catch (SnapshotMissingException smex) {
logger.info(() -> new ParameterizedMessage(
"Tried deleting in-progress snapshot [{}], but it could not be found after failing to abort.",
smex.getSnapshotName()), e);
listener.onFailure(new SnapshotException(snapshot,
"Tried deleting in-progress snapshot [" + smex.getSnapshotName() + "], but it " +
"could not be found after failing to abort.", smex));
}
});
} }
}); ));
} else { } else {
logger.debug("deleted snapshot is not running - deleting files"); logger.debug("deleted snapshot is not running - deleting files");
deleteSnapshotFromRepository(snapshot, listener, repositoryStateId); deleteSnapshotFromRepository(snapshot, listener, repositoryStateId);
@ -1335,8 +1331,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
* @param listener listener * @param listener listener
* @param repositoryStateId the unique id representing the state of the repository at the time the deletion began * @param repositoryStateId the unique id representing the state of the repository at the time the deletion began
*/ */
private void deleteSnapshotFromRepository(final Snapshot snapshot, @Nullable final DeleteSnapshotListener listener, private void deleteSnapshotFromRepository(Snapshot snapshot, @Nullable ActionListener<Void> listener, long repositoryStateId) {
long repositoryStateId) {
threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(() -> { threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(() -> {
try { try {
Repository repository = repositoriesService.repository(snapshot.getRepository()); Repository repository = repositoriesService.repository(snapshot.getRepository());
@ -1354,7 +1349,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
* Removes the snapshot deletion from {@link SnapshotDeletionsInProgress} in the cluster state. * Removes the snapshot deletion from {@link SnapshotDeletionsInProgress} in the cluster state.
*/ */
private void removeSnapshotDeletionFromClusterState(final Snapshot snapshot, @Nullable final Exception failure, private void removeSnapshotDeletionFromClusterState(final Snapshot snapshot, @Nullable final Exception failure,
@Nullable final DeleteSnapshotListener listener) { @Nullable final ActionListener<Void> listener) {
clusterService.submitStateUpdateTask("remove snapshot deletion metadata", new ClusterStateUpdateTask() { clusterService.submitStateUpdateTask("remove snapshot deletion metadata", new ClusterStateUpdateTask() {
@Override @Override
public ClusterState execute(ClusterState currentState) { public ClusterState execute(ClusterState currentState) {
@ -1388,7 +1383,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
if (failure != null) { if (failure != null) {
listener.onFailure(failure); listener.onFailure(failure);
} else { } else {
listener.onResponse(); listener.onResponse(null);
} }
} }
} }
@ -1508,19 +1503,11 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
/** /**
* Adds snapshot completion listener * Adds snapshot completion listener
* *
* @param snapshot Snapshot to listen for
* @param listener listener * @param listener listener
*/ */
public void addListener(SnapshotCompletionListener listener) { private void addListener(Snapshot snapshot, ActionListener<SnapshotInfo> listener) {
this.snapshotCompletionListeners.add(listener); snapshotCompletionListeners.computeIfAbsent(snapshot, k -> new CopyOnWriteArrayList<>()).add(listener);
}
/**
* Removes snapshot completion listener
*
* @param listener listener
*/
public void removeListener(SnapshotCompletionListener listener) {
this.snapshotCompletionListeners.remove(listener);
} }
@Override @Override
@ -1541,45 +1528,4 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
public RepositoriesService getRepositoriesService() { public RepositoriesService getRepositoriesService() {
return repositoriesService; return repositoriesService;
} }
/**
* Listener for create snapshot operation
*/
public interface CreateSnapshotListener {
/**
* Called when snapshot has successfully started
*
* @param snapshot snapshot that was created
*/
void onResponse(Snapshot snapshot);
/**
* Called if a snapshot operation couldn't start
*/
void onFailure(Exception e);
}
/**
* Listener for delete snapshot operation
*/
public interface DeleteSnapshotListener {
/**
* Called if delete operation was successful
*/
void onResponse();
/**
* Called if delete operation failed
*/
void onFailure(Exception e);
}
public interface SnapshotCompletionListener {
void onSnapshotCompletion(Snapshot snapshot, SnapshotInfo snapshotInfo);
void onSnapshotFailure(Snapshot snapshot, Exception e);
}
} }