tombentley commented on a change in pull request #9676:
URL: https://github.com/apache/kafka/pull/9676#discussion_r545145005



##########
File path: core/src/test/scala/unit/kafka/log/LogTest.scala
##########
@@ -2818,6 +2818,22 @@ class LogTest {
       new SimpleRecord(RecordBatch.NO_TIMESTAMP, "key".getBytes, 
"value".getBytes)), leaderEpoch = 0)
   }
 
+  @Test
+  def testAppendToOrReadFromLogInFailedLogDir(): Unit = {
+    val log = createLog(logDir, LogConfig())
+    log.appendAsLeader(TestUtils.singletonRecords(value = null), leaderEpoch = 
0)
+    assertEquals(0, readLog(log, 0, 
4096).records.records.iterator.next().offset)
+    try {
+      log.maybeHandleIOException("Simulating failed log dir") {

Review comment:
       That only seems to work when writing to the transaction index (because 
the file opening is deferred so that it's only at the point where we try to 
append some transaction markers that the index file open is attempted and the 
path found to be a directory, which means that the exception propagates through 
`Log`). Renaming the segment file, on the other hand, doesn't propagate through 
`Log`, because the rename throws immediately and propagates directly from 
`FileRecords` to the test. Obviously when the exception doesn't propagate via 
`Log` the `logDirOffline` doesn't get set.
   
   Making this change kind-of mixes transactional appends into a test for 
something with is orthogonal to transactional logs. In any case, I've done as 
you suggest, I just thought it worth mentioning.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to