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.

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

GetSnapshotsResponse should re-wrap failed exceptions so callsites are visible in the stacktrace #43462

Closed
dakrone opened this issue Jun 20, 2019 · 4 comments · Fixed by #74451
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination.

Comments

@dakrone
Copy link
Member

dakrone commented Jun 20, 2019

From the following code:

public List<SnapshotInfo> getSnapshots(String repo) {
List<SnapshotInfo> snapshots = successfulResponses.get(repo);
if (snapshots != null) {
return snapshots;
}
ElasticsearchException error = failedResponses.get(repo);
if (error == null) {
throw new IllegalArgumentException("No such repository");
}
throw error;
}

It's possible for snapshot exceptions to be generated on a different line than where they are actually thrown, for example, code that prior to #42090 worked like this:

GetSnapshotsRequest getSnapshotsRequest = new GetSnapshotsRequest(new String[]{repo}, new String[]{snapshotName});
final GetSnapshotsResponse snaps;
try {
    snaps = client.snapshot().get(getSnapshotsRequest, RequestOptions.DEFAULT);
} catch (Exception e) {
    if (e.getMessage().contains("snapshot_missing_exception")) {
        fail("snapshot does not exist: " + snapshotName);
    }
    throw e;
}

Optional<SnapshotInfo> info = snaps.getSnapshots(repo).stream().findFirst();
if (info.isPresent()) {
    info.ifPresent(si -> {
        assertThat(si.snapshotId().getName(), equalTo(snapshotName));
        assertThat(si.state(), equalTo(SnapshotState.SUCCESS));
    });
} else {
    fail("unable to find snapshot; " + snapshotName);
}

Now fails.

The exception is generated from the snaps.getSnapshots(repo) call above, however, the stacktrace for the exception actually traces back to the line

    snaps = client.snapshot().get(getSnapshotsRequest, RequestOptions.DEFAULT);

which is very confusing, because that line itself is wrapped in a try/catch.

GetSnapshotsResponse should change this line:

Into something like

throw new ElasticsearchException(error);

So that the stacktrace correctly identifies the caller.

@dakrone dakrone added the :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Jun 20, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@ywelsch
Copy link
Contributor

ywelsch commented Jun 21, 2019

@dakrone Thanks for your analysis. Would you like to contribute a PR?

@0xamogh
Copy link

0xamogh commented Jun 22, 2019

I am willing to take this up

@dakrone
Copy link
Member Author

dakrone commented Jul 23, 2019

@amogh-jrules if you'd like to submit a PR for this that would be great, check out our contributing document if you need assistance getting an environment set up.

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Nov 25, 2019
As asked for in elastic#43462.
I think this is a worthwhile change if only to make tests easier to debug.

Closes elastic#43462
@rjernst rjernst added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 4, 2020
original-brownbear added a commit that referenced this issue Jun 24, 2021
This PR returns the get snapshots API to the 7.x format (and transport client behavior) and enhances it for requests that ask for multiple repositories.
The changes for requests that target multiple repositories are:
* Add `repository` field to `SnapshotInfo` and REST response
* Add `failures` map alongside `snapshots` list instead of returning just an exception response as done for single repo requests
* Pagination now works across repositories instead of being per repository for multi-repository requests

closes #69108
closes #43462
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jun 29, 2021
This PR returns the get snapshots API to the 7.x format (and transport client behavior) and enhances it for requests that ask for multiple repositories.
The changes for requests that target multiple repositories are:
* Add `repository` field to `SnapshotInfo` and REST response
* Add `failures` map alongside `snapshots` list instead of returning just an exception response as done for single repo requests
* Pagination now works across repositories instead of being per repository for multi-repository requests

closes elastic#69108
closes elastic#43462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination.
Projects
None yet
5 participants