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
Fix NullPointerException in cat-recovery API #6190
Conversation
the change looks good but how come this never triggered a failure? can you add a test maybe? |
@martijnvg can we make this fail somehow with a simple test? |
@s1monw Updated the PR, when backporting I will add the 1.2.2 version to the 1.2 branch. |
assertThat(outRequest.recoveryType(), equalTo(inRequest.recoveryType())); | ||
assertThat(outRequest.recoveryId(), equalTo(inRequest.recoveryId())); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could also have a test for the backwards compatibility where we randomize the versions we read from and we write to and we check that everything is ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. I updated the PR.
@@ -115,6 +115,9 @@ public void readFrom(StreamInput in) throws IOException { | |||
StoreFileMetaData md = StoreFileMetaData.readStoreFileMetaData(in); | |||
existingFiles.put(md.name(), md); | |||
} | |||
if (in.getVersion().onOrAfter(Version.V_1_3_0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we intend to backport we should set it to 1.2.2?
left a comment regarding the version check, other than that LGTM |
…coveryRequest. Closes elastic#6190
…coveryRequest. Closes #6190
…coveryRequest. Closes #6190
…coveryRequest. Closes elastic#6190
The recovery type isn't serialized. This can cause a NPE like this: