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

Corruption detection is fragile? #24800

Closed
jpountz opened this issue May 19, 2017 · 5 comments · Fixed by #41889
Closed

Corruption detection is fragile? #24800

jpountz opened this issue May 19, 2017 · 5 comments · Fixed by #41889
Assignees
Labels
:Distributed/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. >enhancement

Comments

@jpountz
Copy link
Contributor

jpountz commented May 19, 2017

I was looking at how we detect corruptions, and the current logic tries to see whether the exception itself or any of its causes is an instance of CorruptIndexException. However, the following pattern that Lucene uses in multiple places:

Throwable priorE = null;
try (ChecksumIndexInput input = dir.openChecksumInput(fileName, context)) {
  // do some stuff that may throw any exceptions
} catch (Throwable exception) {
  priorE = exception;
} finally {
  CodecUtil.checkFooter(input, priorE);
}

causes the CorruptIndexException to be suppressed if some other exception was thrown in the try block. So the thrown exception would not be seen as a corruption exception. This makes me want to check for suppressed exceptions as well in ExceptionsHelper.unwrapCorruption but I don't know enough about corruption detection to know whether that is safe to do?

@s1monw
Copy link
Contributor

s1monw commented May 23, 2017

I think we can expand the unwrapping to suppressed exceptions but I am not sure it will help much. Usually we will just detect it a bit later?

@s1monw s1monw self-assigned this May 23, 2017
@s1monw s1monw added the :Distributed/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. label Mar 15, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@ywelsch
Copy link
Contributor

ywelsch commented Mar 11, 2019

@jpountz @s1monw is this still something you think needs to be addressed, given that the corruption detection seems to be best-effort anyway?

@ywelsch
Copy link
Contributor

ywelsch commented May 7, 2019

This issue is causing pain across our test suite, see e.g. #41201
We will implement the workaround here (scanning for suppressed corruption exceptions).

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue May 7, 2019
* As discussed in elastic#24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes elastic#24800, 41201
@original-brownbear
Copy link
Member

Workaround incoming in #41889 :)

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue May 8, 2019
* As discussed in elastic#24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes elastic#24800, 41201
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue May 8, 2019
* As discussed in elastic#24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes elastic#24800, 41201
original-brownbear added a commit that referenced this issue May 21, 2019
* Make unwrapCorrupt Check Suppressed Ex.

* As discussed in #24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes #24800, 41201
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this issue May 27, 2019
* Make unwrapCorrupt Check Suppressed Ex.

* As discussed in elastic#24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes elastic#24800, 41201
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue May 28, 2019
* Make unwrapCorrupt Check Suppressed Ex.

* As discussed in elastic#24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes elastic#24800, 41201
original-brownbear added a commit that referenced this issue May 28, 2019
* Make unwrapCorrupt Check Suppressed Ex. (#41889)
* As discussed in #24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes #24800, 41201
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jun 3, 2019
* Make unwrapCorrupt Check Suppressed Ex.

* As discussed in elastic#24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes elastic#24800, 41201
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jun 3, 2019
* Make unwrapCorrupt Check Suppressed Ex. (elastic#41889)
* As discussed in elastic#24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes elastic#24800, 41201
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jun 3, 2019
* Make unwrapCorrupt Check Suppressed Ex. (elastic#41889)
* As discussed in elastic#24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes elastic#24800, 41201
original-brownbear added a commit that referenced this issue Jun 3, 2019
* Make unwrapCorrupt Check Suppressed Ex. (#41889)
* As discussed in #24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes #24800, 41201
original-brownbear added a commit that referenced this issue Jun 3, 2019
* Make unwrapCorrupt Check Suppressed Ex. (#41889)
* As discussed in #24800 we want to check for suppressed corruption
indicating exceptions here as well to more reliably categorize
corruption related exceptions
* Closes #24800, 41201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. >enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants