[GitHub] [kafka] showuon commented on a change in pull request #10749: KAFKA-12773: Use UncheckedIOException when wrapping IOException

[GitBox]

showuon commented on a change in pull request #10749:
URL: https://github.com/apache/kafka/pull/10749#discussion_r640356874



##########
File path: raft/src/test/java/org/apache/kafka/raft/QuorumStateTest.java
##########
@@ -945,11 +946,10 @@ public void testObserverUnattachedToFollower() throws 
IOException {
     }
 
     @Test
-    public void testInitializeWithCorruptedStore() throws IOException {
-        QuorumStateStore stateStore = Mockito.mock(QuorumStateStore.class);
-        
Mockito.doThrow(IOException.class).when(stateStore).readElectionState();
+    public void testInitializeWithCorruptedStore() {
         QuorumState state = buildQuorumState(Utils.mkSet(localId));
-
+        QuorumStateStore stateStore = Mockito.mock(QuorumStateStore.class);
+        
Mockito.doThrow(UncheckedIOException.class).when(stateStore).readElectionState();

Review comment:
       `nit`: I think it'd better to put the 2 line mock at the beginning of 
the test as before. (i.e. before `QuorumState state = 
buildQuorumState(Utils.mkSet(localId));`)

##########
File path: raft/src/main/java/org/apache/kafka/raft/FileBasedStateStore.java
##########
@@ -146,25 +150,36 @@ private void writeElectionStateToFile(final File 
stateFile, QuorumStateData stat
             writer.flush();
             fileOutputStream.getFD().sync();
             Utils.atomicMoveWithFallback(temp.toPath(), stateFile.toPath());
+        } catch (IOException e) {
+            throw new UncheckedIOException(
+                    String.format("Error while writing the Quorum status from 
the file %s", stateFile.getAbsolutePath()), e);
         } finally {
             // cleanup the temp file when the write finishes (either success 
or fail).
-            Files.deleteIfExists(temp.toPath());
+            deleteFileIfExists(temp);
         }
     }
 
     /**
      * Clear state store by deleting the local quorum state file
-     *
-     * @throws IOException if there is any IO exception during delete
      */
     @Override
-    public void clear() throws IOException {
-        Files.deleteIfExists(stateFile.toPath());
-        Files.deleteIfExists(new File(stateFile.getAbsolutePath() + 
".tmp").toPath());
+    public void clear() {
+        deleteFileIfExists(stateFile);
+        deleteFileIfExists(new File(stateFile.getAbsolutePath() + ".tmp"));
     }
 
     @Override
     public String toString() {
         return "Quorum state filepath: " + stateFile.getAbsolutePath();
     }
+
+    private void deleteFileIfExists(File file) {
+        try {
+            Files.deleteIfExists(file.toPath());
+        } catch (IOException e) {
+            throw new UncheckedIOException(
+                    String.format("Error deleting file %s", 
file.getAbsoluteFile()), e

Review comment:
       Maybe this is better: `Error while deleting file...`




-- 
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