jsancio commented on a change in pull request #10749:
URL: https://github.com/apache/kafka/pull/10749#discussion_r644156023
##########
File path: raft/src/main/java/org/apache/kafka/snapshot/Snapshots.java
##########
@@ -68,15 +68,27 @@ public static Path snapshotPath(Path logDir, OffsetAndEpoch
snapshotId) {
return snapshotDir(logDir).resolve(filenameFromSnapshotId(snapshotId)
+ SUFFIX);
}
- public static Path createTempFile(Path logDir, OffsetAndEpoch snapshotId)
throws IOException {
+ public static Path createTempFile(Path logDir, OffsetAndEpoch snapshotId) {
Path dir = snapshotDir(logDir);
+ Path tempFile;
- // Create the snapshot directory if it doesn't exists
- Files.createDirectories(dir);
-
- String prefix = String.format("%s-",
filenameFromSnapshotId(snapshotId));
+ try {
+ // Create the snapshot directory if it doesn't exists
+ Files.createDirectories(dir);
- return Files.createTempFile(dir, prefix, PARTIAL_SUFFIX);
+ String prefix = String.format("%s-",
filenameFromSnapshotId(snapshotId));
+ tempFile = Files.createTempFile(dir, prefix, PARTIAL_SUFFIX);
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ String.format(
+ "Error creating temporary file. logDir = %s,
snapshotId = %s",
+ dir.toAbsolutePath(),
+ snapshotId
+ ),
+ e
Review comment:
Some of these lines have extra 4 space. Other lines have extra 8 spaces.
##########
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
Review comment:
These 5 lines have extra 4 spaces of 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()
Review comment:
These 2 lines have extra 4 spaces of indentation.
##########
File path:
raft/src/main/java/org/apache/kafka/snapshot/FileRawSnapshotWriter.java
##########
@@ -58,7 +59,11 @@ 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:
Extra spaces of indentation. Even for method parameters, we only indent
by 4 spaces. Also, you can clean this up if each argument for `String.format`
is on its own line. E.g.
```java
String.format(
"....",
tempSnapshotPath,
snapshotId
)
```
Note, this comment applies to a few other places in this 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
