Copilot commented on code in PR #7953:
URL: https://github.com/apache/ignite-3/pull/7953#discussion_r3050030740
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/RaftLogGarbageCollector.java:
##########
@@ -110,7 +111,7 @@ class RaftLogGarbageCollector {
this.strategy = strategy;
this.failureProcessor = failureProcessor;
- gcThread = new IgniteThread(nodeName, "segstore-gc", new GcTask());
+ gcThread = new IgniteThread(nodeName, "segstore-gc-" + storageName,
new GcTask());
Review Comment:
There are two spaces after the comma in the `IgniteThread` constructor call;
please remove the extra whitespace to match surrounding formatting.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentFileManager.java:
##########
@@ -156,13 +158,18 @@ class SegmentFileManager implements ManuallyCloseable {
SegmentFileManager(
String nodeName,
+ String storageName,
Path baseDir,
int stripes,
FailureProcessor failureProcessor,
RaftConfiguration raftConfiguration,
LogStorageConfiguration storageConfiguration
) throws IOException {
- this.segmentFilesDir = baseDir.resolve("segments");
+ this.storageName = storageName;
+
+ Path storageDir = baseDir.resolve(storageName);
+
+ this.segmentFilesDir = storageDir.resolve("segments");
Review Comment:
`storageName` is used as a path component via
`baseDir.resolve(storageName)`. If `storageName` is absolute or contains
`..`/separators, this can escape `baseDir` (or create unintended nested
directories), potentially writing Raft log files outside the intended data dir.
Please validate/sanitize `storageName` (non-null/non-blank, not absolute, no
traversal) before resolving it, and fail fast with a clear exception if invalid.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentFileManager.java:
##########
@@ -156,13 +158,18 @@ class SegmentFileManager implements ManuallyCloseable {
SegmentFileManager(
String nodeName,
+ String storageName,
Path baseDir,
int stripes,
FailureProcessor failureProcessor,
RaftConfiguration raftConfiguration,
LogStorageConfiguration storageConfiguration
) throws IOException {
- this.segmentFilesDir = baseDir.resolve("segments");
+ this.storageName = storageName;
+
+ Path storageDir = baseDir.resolve(storageName);
+
+ this.segmentFilesDir = storageDir.resolve("segments");
this.stripes = stripes;
this.isSync = raftConfiguration.fsync().value();
Review Comment:
The new `storageName` subdirectory layout is a key behavior change, but
there is no test asserting that segment/index files are created under
`baseDir/<storageName>/...` and that two different `storageName` values do not
collide when sharing the same `baseDir`. Please add a targeted test to cover
this, so regressions in directory layout are caught.
--
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]
