fix: fix NPE when backing a record that has null key/values #7268
Conversation
|
|
||
| if (record.key() == null || record.value() == null) { | ||
| throw new IllegalArgumentException(String.format( | ||
| "Record contain a null key/value: key=%s, value=%s", record.key(), record.value())); |
There was a problem hiding this comment.
it might also help to output the partition/offset (even though partition is always 0 here) - ditto for other error messages
There was a problem hiding this comment.
Done. Btw, I removed this redundant null check from this file. I left only the null checks in the CommandTopicBackupImpl.
| @@ -84,6 +84,15 @@ public void write(final ConsumerRecord<byte[], byte[]> record) throws IOExceptio | |||
| throw new IOException("Write permission denied."); | |||
| } | |||
|
|
|||
| if (record == null) { | |||
| throw new IllegalArgumentException("Record is null"); | |||
There was a problem hiding this comment.
when can this ever happen? can we throw a more useful error message?
There was a problem hiding this comment.
Because any records read from the cmd topic should not have null records, I feel this can only happen if the local file was manipulated somehow?
There was a problem hiding this comment.
I don't think this never happen. I added just in case, but seems unnecessary. I removed it.
| @@ -84,6 +84,15 @@ public void write(final ConsumerRecord<byte[], byte[]> record) throws IOExceptio | |||
| throw new IOException("Write permission denied."); | |||
| } | |||
|
|
|||
| if (record == null) { | |||
| throw new IllegalArgumentException("Record is null"); | |||
There was a problem hiding this comment.
Because any records read from the cmd topic should not have null records, I feel this can only happen if the local file was manipulated somehow?
| recordString = String.format("key=%s, value=%s", record.key(), record.value()); | ||
| } | ||
|
|
||
| LOG.warn("Cannot write a null record or null key/value to the backup file. This could be " |
There was a problem hiding this comment.
Ditto here: could this ever happen, if we already check for null record/key/value when writing to cmd topic?
There was a problem hiding this comment.
I removed the redundant null check from the BackupReplayFile, which is called by the CommandTopicBackupImpl. I just left the one from this file.
Description
What behavior do you want to change, why, how does your patch achieve the changes?
Fixes #7184
Testing done
Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Reviewer checklist