Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing dead code in RecoveryTarget. #4278

Merged
merged 2 commits into from
Aug 24, 2022
Merged

Removing dead code in RecoveryTarget. #4278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdbdf26
Removing dead code in RecoveryTarget.
mch2 Aug 22, 2022
574122e
PR Comments.
mch2 Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 0 additions & 47 deletions server/src/main/java/org/opensearch/indices/recovery/RecoveryTarget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,51 +177,6 @@ public boolean reset(CancellableThreads newTargetCancellableThreads) throws IOEx
return false;
}

/**
* cancel the recovery. calling this method will clean temporary files and release the store
* unless this object is in use (in which case it will be cleaned once all ongoing users call
* {@link #decRef()}
* <p>
* if {@link #cancellableThreads()} was used, the threads will be interrupted.
*/
public void cancel(String reason) {
if (finished.compareAndSet(false, true)) {
try {
logger.debug("recovery canceled (reason: [{}])", reason);
kartg marked this conversation as resolved.
Show resolved Hide resolved
cancellableThreads.cancel(reason);
} finally {
// release the initial reference. recovery files will be cleaned as soon as ref count goes to zero, potentially now
decRef();
}
}
}

/**
* fail the recovery and call listener
*
* @param e exception that encapsulating the failure
* @param sendShardFailure indicates whether to notify the cluster-manager of the shard failure
*/
public void fail(RecoveryFailedException e, boolean sendShardFailure) {
Copy link
Member

@kartg kartg Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the reason this exists is to force the RecoveryFailedException type (the super class method parameter uses OpenSearchException).

Are we OK with broadening the scope of accepted exceptions? Alternatively, should we create a ReplicationFailedException to use in the super class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, the listeners were refactored to a common interface that accepts OpenSearchException.

Copy link
Member Author

@mch2 mch2 Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, should we create a ReplicationFailedException to use in the super class?

Not a bad idea, would like to make that a separate issue from this removal if thats ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure you leave a comment here with the issue name so they're linked together

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4291

super.fail(e, sendShardFailure);
}

/** mark the current recovery as done */
public void markAsDone() {
if (finished.compareAndSet(false, true)) {
assert multiFileWriter.tempFileNames.isEmpty() : "not all temporary files are renamed";
try {
// this might still throw an exception ie. if the shard is CLOSED due to some other event.
kartg marked this conversation as resolved.
Show resolved Hide resolved
// it's safer to decrement the reference in a try finally here.
indexShard.postRecovery("peer recovery done");
} finally {
// release the initial reference. recovery files will be cleaned as soon as ref count goes to zero, potentially now
decRef();
}
listener.onDone(state());
}
}

@Override
protected void closeInternal() {
try {
Expand All @@ -246,8 +201,6 @@ protected String getPrefix() {
@Override
protected void onDone() {
assert multiFileWriter.tempFileNames.isEmpty() : "not all temporary files are renamed";
// this might still throw an exception ie. if the shard is CLOSED due to some other event.
// it's safer to decrement the reference in a try finally here.
indexShard.postRecovery("peer recovery done");
}

Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/indices/replication/common/ReplicationTarget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public void markAsDone() {
public void cancel(String reason) {
if (finished.compareAndSet(false, true)) {
try {
logger.debug("replication cancelled (reason: [{}])", reason);
logger.debug("replication/recovery cancelled (reason: [{}])", reason);
onCancel(reason);
} finally {
// release the initial reference. replication files will be cleaned as soon as ref count goes to zero, potentially now
Expand Down