Copilot commented on code in PR #7638:
URL: https://github.com/apache/ignite-3/pull/7638#discussion_r2828483221
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/impl/VolatileLogStorageManagerCreator.java:
##########
@@ -125,9 +140,15 @@ public CompletableFuture<Void> startAsync(ComponentContext
componentContext) {
throw new RuntimeException(e);
}
Review Comment:
Resource leak on exception: If an exception occurs after creating env (line
110), sstFileManager (line 122), or dbOptions (line 112), these resources are
not cleaned up. The ColumnFamilyOptions cfOption created on line 113 is also
not closed. Consider adding a try-catch block that properly cleans up these
resources on failure, similar to the pattern used in
DefaultLogStorageManager.start() at line 226-230.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/logit/LogitLogStorageManager.java:
##########
@@ -127,6 +131,17 @@ public LogSyncer logSyncer() {
return new NoOpLogSyncer();
}
+ @Override
+ public long totalBytesOnDisk() {
+ try (Stream<Path> paths = Files.walk(logPath)) {
+ return paths.filter(Files::isRegularFile)
+ .mapToLong(path -> path.toFile().length())
+ .sum();
+ } catch (IOException e) {
+ throw new IgniteInternalException(INTERNAL_ERR, e);
+ }
+ }
Review Comment:
The implementation walks the entire directory tree to calculate disk usage
on every call. This could be inefficient for large directory structures with
many files. Consider caching the result and updating it incrementally, or
documenting that this method should be called sparingly due to its I/O cost.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/logit/LogitLogStorageManager.java:
##########
@@ -127,6 +131,17 @@ public LogSyncer logSyncer() {
return new NoOpLogSyncer();
}
+ @Override
+ public long totalBytesOnDisk() {
+ try (Stream<Path> paths = Files.walk(logPath)) {
+ return paths.filter(Files::isRegularFile)
+ .mapToLong(path -> path.toFile().length())
+ .sum();
+ } catch (IOException e) {
+ throw new IgniteInternalException(INTERNAL_ERR, e);
+ }
+ }
Review Comment:
Missing test coverage: The totalBytesOnDisk method in LogitLogStorageManager
lacks test coverage, while similar implementations (DefaultLogStorageManager
and VolatileLogStorageManagerCreator) have dedicated tests. Consider adding a
test to verify that totalBytesOnDisk correctly walks the directory tree and
sums file sizes, especially given the different implementation approach
compared to RocksDB-based managers.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/impl/VolatileLogStorageManagerCreator.java:
##########
@@ -99,6 +107,8 @@ public CompletableFuture<Void> startAsync(ComponentContext
componentContext) {
wipeOutDb();
+ env = createEnv();
+
dbOptions = createDbOptions();
ColumnFamilyOptions cfOption = createColumnFamilyOptions();
Review Comment:
Resource leak: The ColumnFamilyOptions object created on line 113 is not
stored as a field and is not closed in stopAsync (line 209). In contrast,
DefaultLogStorageManager stores cfOption as a field and closes it in
closeRocksResources. The cfOption should either be stored as a field and closed
in stopAsync, or closed immediately after creating the database on line 127.
Recommend storing it as a field and closing it in the stopAsync method.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
