You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/06/08 02:31:19 UTC
[GitHub] [kafka] mumrah commented on a change in pull request #10749: KAFKA-12773: Use UncheckedIOException when wrapping IOException
mumrah commented on a change in pull request #10749:
URL: https://github.com/apache/kafka/pull/10749#discussion_r647047054
##########
File path: raft/src/main/java/org/apache/kafka/raft/FileBasedStateStore.java
##########
@@ -91,14 +92,17 @@ private QuorumStateData readStateFromFile(File file) throws IOException {
final short dataVersion = dataVersionNode.shortValue();
return QuorumStateDataJsonConverter.read(dataObject, dataVersion);
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ String.format("Error while reading the Quorum status from the file %s", file), e);
Review comment:
nit: indentation
##########
File path: raft/src/main/java/org/apache/kafka/raft/QuorumState.java
##########
@@ -463,9 +464,14 @@ public void transitionToCandidate() throws IOException {
return state;
}
- private void transitionTo(EpochState state) throws IOException {
+ private void transitionTo(EpochState state) {
if (this.state != null) {
- this.state.close();
+ try {
+ this.state.close();
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ "Failed to transition from " + this.state.name() + " to " + state.name(), e);
Review comment:
nit: indentation
##########
File path: raft/src/main/java/org/apache/kafka/snapshot/FileRawSnapshotWriter.java
##########
@@ -147,24 +171,27 @@ void checkIfFrozen(String operation) {
*
* @param logDir the directory for the topic partition
* @param snapshotId the end offset and epoch for the snapshotId
- * @throws IOException for any IO error while creating the snapshot
*/
public static FileRawSnapshotWriter create(
Path logDir,
OffsetAndEpoch snapshotId,
Optional<ReplicatedLog> replicatedLog
) {
- try {
- Path path = Snapshots.createTempFile(logDir, snapshotId);
+ Path path = Snapshots.createTempFile(logDir, snapshotId);
+ try {
return new FileRawSnapshotWriter(
path,
FileChannel.open(path, Utils.mkSet(StandardOpenOption.WRITE, StandardOpenOption.APPEND)),
snapshotId,
replicatedLog
);
} catch (IOException e) {
- throw new RuntimeException(e);
+ throw new UncheckedIOException(
+ String.format("Error creating snapshot writer." +
+ "temp path = %s, snapshotId %s", path, snapshotId),
+ e
+ );
Review comment:
nit: indentation
##########
File path: raft/src/main/java/org/apache/kafka/snapshot/FileRawSnapshotWriter.java
##########
@@ -58,7 +59,14 @@ public long sizeInBytes() {
try {
return channel.size();
} catch (IOException e) {
- throw new RuntimeException(e);
+ throw new UncheckedIOException(
+ String.format(
+ "Error calculating snapshot size.temp path = %s, snapshotId = %s",
+ this.tempSnapshotPath,
+ this.snapshotId
+ ),
+ e
Review comment:
nit: same formatting comment as above
##########
File path: raft/src/main/java/org/apache/kafka/snapshot/FileRawSnapshotWriter.java
##########
@@ -115,7 +135,11 @@ public void close() {
// This is a noop if freeze was called before calling close
Files.deleteIfExists(tempSnapshotPath);
} catch (IOException e) {
- throw new RuntimeException(e);
+ throw new UncheckedIOException(
+ String.format("Error closing snapshot writer." +
+ "temp path = %s, snapshotId %s", this.tempSnapshotPath, this.snapshotId),
+ e
+ );
Review comment:
nit: indentation
##########
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 while deleting file %s", file.getAbsoluteFile()), e
Review comment:
nit: indentation
##########
File path: raft/src/main/java/org/apache/kafka/snapshot/FileRawSnapshotReader.java
##########
@@ -75,17 +75,27 @@ public void close() {
*
* @param logDir the directory for the topic partition
* @param snapshotId the end offset and epoch for the snapshotId
- * @throws java.nio.file.NoSuchFileException if the snapshot doesn't exist
- * @throws IOException for any IO error while opening the snapshot
*/
- public static FileRawSnapshotReader open(Path logDir, OffsetAndEpoch snapshotId) throws IOException {
- FileRecords fileRecords = FileRecords.open(
- Snapshots.snapshotPath(logDir, snapshotId).toFile(),
- false, // mutable
- true, // fileAlreadyExists
- 0, // initFileSize
- false // preallocate
- );
+ public static FileRawSnapshotReader open(Path logDir, OffsetAndEpoch snapshotId) {
+ FileRecords fileRecords;
+ Path filePath = Snapshots.snapshotPath(logDir, snapshotId);
+ try {
+ fileRecords = FileRecords.open(
+ filePath.toFile(),
+ false, // mutable
+ true, // fileAlreadyExists
+ 0, // initFileSize
+ false // preallocate
+ );
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ String.format(
+ "Unable to Opens a snapshot file %s",
+ filePath.toAbsolutePath()
+ ),
+ e
Review comment:
nit: seems like this could all fit on one line without going over 120 characters
##########
File path: raft/src/main/java/org/apache/kafka/snapshot/FileRawSnapshotWriter.java
##########
@@ -78,7 +90,11 @@ public void append(MemoryRecords records) {
checkIfFrozen("Append");
Utils.writeFully(channel, records.buffer());
} catch (IOException e) {
- throw new RuntimeException(e);
+ throw new UncheckedIOException(
+ String.format("Error writing file snapshot," +
+ "temp path = %s, snapshotId = %s", this.tempSnapshotPath, this.snapshotId),
+ e
+ );
Review comment:
nit: indentation
##########
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);
Review comment:
nit: indentation
##########
File path: raft/src/main/java/org/apache/kafka/snapshot/FileRawSnapshotWriter.java
##########
@@ -104,7 +120,11 @@ public void freeze() {
replicatedLog.ifPresent(log -> log.onSnapshotFrozen(snapshotId));
} catch (IOException e) {
- throw new RuntimeException(e);
+ throw new UncheckedIOException(
+ String.format("Error freezing file snapshot." +
+ "temp path = %s, snapshotId = %s", this.tempSnapshotPath, this.snapshotId),
+ e
+ );
Review comment:
nit: indentation
--
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:
users@infra.apache.org