ibessonov commented on code in PR #6839:
URL: https://github.com/apache/ignite-3/pull/6839#discussion_r2480350597
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/IndexFileManager.java:
##########
@@ -120,20 +129,36 @@ class IndexFileManager {
Files.createDirectories(indexFilesDir);
}
+ void start() throws IOException {
+ try (Stream<Path> indexFiles = Files.list(indexFilesDir)) {
+ Iterator<Path> it = indexFiles.sorted().iterator();
+
+ while (it.hasNext()) {
+ recoverIndexFileMetas(it.next());
+ }
Review Comment:
Just a question: why did you mix iterators and streams here? A simple
`forEach` would do the job just as well, is there some important difference
that I'm unaware of?
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/IndexMemTable.java:
##########
@@ -114,6 +114,8 @@ public Iterator<Entry<Long, SegmentInfo>> iterator() {
private Stripe stripe(long groupId) {
int stripeIndex = Long.hashCode(groupId) % stripes.length;
+ assert stripeIndex >= 0 : String.format("Stripe index must not be
negative [groupId=%d]", groupId);
Review Comment:
I don't trust this code, please add a little bit of
`org.apache.ignite.internal.util.IgniteUtils#safeAbs(int)`. How does this not
failing yet?
Another question - this striping method doesn't match the distribution of
disruptors between groups. This will eventually lead to unnecessary contention.
Could you please explain this choice, and maybe fix it? If it can't be fixed
then please describe the reasons. Thank you!
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentFileManager.java:
##########
@@ -150,10 +157,44 @@ class SegmentFileManager implements ManuallyCloseable {
}
void start() throws IOException {
- checkpointer.start();
+ Path lastSegmentFilePath = null;
+
+ try (Stream<Path> segmentFiles = Files.list(segmentFilesDir)) {
Review Comment:
Same comment about `tmp` files, I don't see them being deleted. Maybe I'm
looking at the wrong place
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/IndexFileManager.java:
##########
@@ -120,20 +129,36 @@ class IndexFileManager {
Files.createDirectories(indexFilesDir);
}
+ void start() throws IOException {
+ try (Stream<Path> indexFiles = Files.list(indexFilesDir)) {
Review Comment:
Please delete all `tmp` files preemptively, I believe `start` is a right
place for it. We should also have a test for restart with an incomplete index
file in the folder.
--
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]
