Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed snapshot read from disk #13144

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

eastorski
Copy link
Member

No description provided.

@eastorski eastorski force-pushed the eastorski/fix-snapshot-read branch from c2162e4 to c406c8a Compare December 17, 2024 16:03
Copy link
Collaborator

@AskAlexSharov AskAlexSharov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very hard to understand intention of PR:

  • title is meaningless "Fixed snapshot read from disk"
  • removed segmentsTypeCheck func. It means in case header.seg exists and transactions.seg doesn't exists (pkill -9 between files creation) user will start see new header.seg. I think it's wrong: headers.seg must become visible only when all files (of same block-range) in type-set are exist and indexed.
  • no unit-test changes

So, i don't know: which error PR fixing, what PR does, how it was tested.

@AskAlexSharov
Copy link
Collaborator

from chat (will translate later):

привет. 
если функция работает в 2 случаях из 5 - не значит что ее удаление что-то починит
Мне не очевидно TypedSegments функция возвращает dirty или visible файлы.

Но глянув в код - понятно что dirty. 

Вообще там было так: 
раньше не было dirty/visible разделения - поэтому мы накидывали гору валидации/фильтрации когда читали dirty list с диска.
Потом мы добавили recalcVisibleFiles - который как раз на dirty list применяет гору чеков и что проходит попадает в visible list. 
Получается что нам теперь не нужно валидировать dirty list - и даже вредно его валидировать - иначе не увидим новые недоделанные файлы (как в твоем случае). 

С этой точки зрения твой PR make sense. 

Только его надобы потестировать на какой-то реальной ноде - типа amoy snapshotter - и вмержить после alpha7 релиза (вроде сегодня релизят его).

Единственное что меня беспокоет - у меня в голове нет картинки - какие методы трогают dirty list (их список должен быть сильно ограничен, и в их имени должно быть слово dirty - чтобы нечайно из обычных методов не вызвать dirty методы). Глянул в код - там 3 метода DirtyBlocksAvailable() и  integrateMergedDirtyFiles() и buildMissedIndices(): и это думаю верно.

Вы (bor snaps) полагаетесь на DirtyBlocksAvailable() - наверное это ок. 
давай попробуем потестить на amoy-snapshotter - если нужна какая помощь - пиши

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants