Skip to content
This repository has been archived by the owner on Mar 3, 2024. It is now read-only.

Шеметов Алексей ФИТиП HW 5 #276

Merged
merged 59 commits into from
Dec 28, 2023

Conversation

AlexShemetov
Copy link
Contributor

No description provided.

try {
Files.createFile(indexFile);
} catch (FileAlreadyExistsException ignored) {
// ignore

Choose a reason for hiding this comment

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

Можно логгер прикрутить (Как совет, это не обязательно делать)


Files.deleteIfExists(indexFile);

Files.move(indexTmp, indexFile, StandardCopyOption.ATOMIC_MOVE);
Copy link

@FedosOnGIT FedosOnGIT Dec 14, 2023

Choose a reason for hiding this comment

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

А здесь почему StandardCopyOption.REPLACE_EXISTING не добавил? (Тут больше пояснение хочется)

Copy link
Contributor Author

@AlexShemetov AlexShemetov Dec 14, 2023

Choose a reason for hiding this comment

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

Ну, да, согласен, удаление + перемещение = замене. Исправлю


public InMemoryDao(Config config) throws IOException {
this.path = config.basePath().resolve("data");
maxSize = config.flushThresholdBytes() == 0 ? config.flushThresholdBytes() : Long.MAX_VALUE / 2;

Choose a reason for hiding this comment

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

А this зачем убрал?

public void flush() throws IOException {
bgExecutor.execute(() -> {
SSTableStates prevState = sstableState.get();
ConcurrentSkipListMap<MemorySegment, Entry<MemorySegment>> writeStorage = prevState.getWriteStorage();

Choose a reason for hiding this comment

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

А что если 2 потока дойдут досюда и оба вызовут getWriteStorage()?

Choose a reason for hiding this comment

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

Получается ты 2 раза сохранишь одну и ту же вещь? Или я где-то ошибаюсь?


upsertLock.writeLock().lock();
try {
sstableState.set(nextState);

Choose a reason for hiding this comment

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

Так, а теперь ещё ситуация.

  1. Оба потока зашли во flush
  2. Оба взяли nextState
  3. Второй поток уснул, а первый закончил flush полностью и даже подабовлял ещё элементов
    Почему когда второй поток проснётся, ничего не сломается?

Choose a reason for hiding this comment

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

Пояснение. Он проснётся и начнёт со 175 строчки upsertLock.writeLock().lock();

@FedosOnGIT FedosOnGIT self-requested a review December 14, 2023 13:49
long valueSize = entry.value() == null ? 0 : entry.value().byteSize();
if (size.addAndGet(keySize + valueSize) >= maxSize) {
flush();
size.set(0);

Choose a reason for hiding this comment

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

Это лучше делать во flush. Представь, что у тебя какой-то поток зайдёт во flush, bgExecutor начнёт выполнять его таску и уснёт. Тогда таски от других потоков не будут выполняться, зато size всё время будет проставляться в 0 и в какой-то момент writeStorage может переполниться

Copy link

@FedosOnGIT FedosOnGIT left a comment

Choose a reason for hiding this comment

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

Окей 25/30 баллов

@FedosOnGIT FedosOnGIT merged commit 6db5701 into polis-vk:main Dec 28, 2023
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants