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

Recovery API reports wrong reused file bytes during snapshot restore #11876

Closed
bleskes opened this issue Jun 25, 2015 · 6 comments
Closed

Recovery API reports wrong reused file bytes during snapshot restore #11876

bleskes opened this issue Jun 25, 2015 · 6 comments
Labels
>bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs help wanted adoptme

Comments

@bleskes
Copy link
Contributor

bleskes commented Jun 25, 2015

The snapshot restore procedure copies over all the bytes. The output however is: (note reused_in_bytes == total_in_bytes):

 "index": {
               "size": {
                  "total_in_bytes": 4390745019,
                  "reused_in_bytes": 4390745019,
                  "recovered_in_bytes": 31053339,
                  "percent": "0.7%"
               },
               "files": {
                  "total": 136,
                  "reused": 0,
                  "recovered": 3,
                  "percent": "2.2%"
               },
               "total_time_in_millis": 131455,
               "source_throttle_time_in_millis": 0,
               "target_throttle_time_in_millis": 0
@bleskes bleskes added >bug help wanted adoptme :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Jun 25, 2015
@szroland
Copy link
Contributor

This seems to be a simple typo in org.elasticsearch.indices.recovery.RecoveryState toXContent():

            builder.byteSizeField(Fields.TOTAL_IN_BYTES, Fields.TOTAL, totalBytes());
            builder.byteSizeField(Fields.REUSED_IN_BYTES, Fields.REUSED, totalBytes());

The result of totalBytes() is used for both fields when serializing, instead of calling recoveredBytes() in the second case. The values produced by the latter seem to be tested, so I guess this is just an issue of serization, which may not be explicilty tested.

I guess I could fix this, and maybe add a test, if no one else is working on this.

@clintongormley
Copy link

@szroland please do

@szroland
Copy link
Contributor

Well, as expected the code wasn't too hard to fix, but while creating the test, I actually ran into an issue with how the Snapshot Create API is described in the rest API json spec file (or rather how that spec is treated in rest tests) vs. how it is actually mapped in its rest controller, see #11897.

Practically if you use snapshot create in a rest test, it sometimes works and sometimes fails, depending on which HTTP method / path combination is picked by the RestClient from the API spec.

szroland added a commit to szroland/elasticsearch that referenced this issue Jun 27, 2015
szroland added a commit to szroland/elasticsearch that referenced this issue Jun 27, 2015
@szroland
Copy link
Contributor

@clintongormley, for now I choose to remove the POST option from the snapshot create API json spec file, which makes it possible to create rest tests that include this API.

I think how that spec file and API should look like can be discussed and implemented in the context of #11897. The test case created here should simply continue to work whatever the outcome of that will be.

Let me know what you think.

@bleskes
Copy link
Contributor Author

bleskes commented Jun 30, 2015

@szroland now that #11928 is taken care of, care to extract the fix for this one and make a PR?

szroland added a commit to szroland/elasticsearch that referenced this issue Jun 30, 2015
szroland added a commit to szroland/elasticsearch that referenced this issue Jun 30, 2015
@szroland
Copy link
Contributor

I created the new pull request, see #11965 above

@bleskes bleskes closed this as completed in 68d658a Jul 1, 2015
bleskes pushed a commit that referenced this issue Jul 1, 2015
Simple snapshot.restore test case that also exposes the bug mentioned in #11876

Fix #11876
Closes #11965
bleskes pushed a commit that referenced this issue Jul 1, 2015
Simple snapshot.restore test case that also exposes the bug mentioned in #11876

Fix #11876
Closes #11965
mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015
Simple snapshot.restore test case that also exposes the bug mentioned in elastic#11876

Fix elastic#11876
Closes elastic#11965
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs help wanted adoptme
Projects
None yet
3 participants