PHOENIX-7786 Improved handling for empty files in Replication Log Processor#2392
Conversation
| // If trailer is missing or corrupt, create reader without trailer validation | ||
| LOG.warn("Invalid Trailer for file {}", filePath, invalidLogTrailerException); | ||
| logFileReaderContext.setValidateTrailer(false); | ||
| logFileReader.init(logFileReaderContext); |
There was a problem hiding this comment.
There is still a leak here because the socket is created in the init call. https://github.com/apache/phoenix/blob/PHOENIX-7562-feature-new/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileReader.java#L49
There was a problem hiding this comment.
There are 2 init calls here so we need to handle failures from any of the two and close the reader
| // If trailer is missing or corrupt, create reader without trailer validation | ||
| LOG.warn("Invalid Trailer for file {}", filePath, invalidLogTrailerException); | ||
| // close the reader first to avoid leaking socket connection | ||
| logFileReader.close(); |
There was a problem hiding this comment.
Unfortunately, this still doesn't completely fix the issue. While it fixes the leak, you simply cannot use the same LogFileReader object because it internally maintains a closed state and then when you try to iterate it will not return any result. https://github.com/apache/phoenix/blob/PHOENIX-7562-feature-new/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileReader.java#L73-L74
You have to construct a new LogFileReader object. This also means you need to enhance your test to correctly capture the issue.
| closeReader(logFileReader); | ||
| logFileReaderContext.setValidateTrailer(false); | ||
| LogFileReader newLogFileReader = new LogFileReader(); | ||
| newLogFileReader.init(logFileReaderContext); |
There was a problem hiding this comment.
Now we are leaking the reference newLogFileReader. Maybe don't introduce a new reference and reassign the existing reference logFileReader to the new object.
There was a problem hiding this comment.
Since this is variable inside the method, once the method call is done, the object would still be purged by GC right?
But it's good to keep the old reference, I have updated in next commit in which I was increasing the test coverage also.
Thank you!
There was a problem hiding this comment.
GC will not call close on the object. So you will have to explicitly close it to avoid leaking the socket.
2 participants
No description provided.