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

Ковальчук Владислав M33371 HW5 #285

Merged
merged 64 commits into from
Jan 9, 2024

Conversation

Dalvikk
Copy link
Contributor

@Dalvikk Dalvikk commented Nov 22, 2023

dr


@Override
public E next() {
E entry = extractor.readEntry(storageSegment, start);

Choose a reason for hiding this comment

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

Согласно спецификации у тебя должно бросаться NoSuchElementException, если следующего элемента нет. На каком этапе ты его бросаешь?

Copy link
Contributor Author

@Dalvikk Dalvikk Dec 28, 2023

Choose a reason for hiding this comment

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

Добавил

if (!hasNext()) {
    throw new NoSuchElementException();
}

в начало метода.
В принципе этот класс разработан для внутреннего использования, merge iterator для внешнего (там кидается NoSuchElementException)

@FedosOnGIT FedosOnGIT self-requested a review December 27, 2023 17:24

/**
* Вставляет значение и возвращает размер стораджа, по которому можно судить нужно ли планировать flush.
* Бросает MemoryOverflowException если достигнут порог по размеру, а предыдущий flush() еще выполняется или упал.

Choose a reason for hiding this comment

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

То есть, если у тебя на шаге n не был достигнут порог, а на шаге n+1 уже стал, то ты кинешь ошибку?

Copy link
Contributor Author

@Dalvikk Dalvikk Dec 28, 2023

Choose a reason for hiding this comment

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

Да, но если предыдущий flush() еще не выполнился.
То есть если после n+1 шага порог достигнут, начинаем флашить и очищаем мапу. Если уже до этого что то флашили и не закончили, то не успеваем.
Сделано под это условие:

В случаях, когда flush не поспевает за вставкой данных (одна полная таблица всё ещё пишется на диск, в то время как заполнилась следующая таблица), параллельные upsert()ы должны бросать исключение -- тем самым мы сигнализируем клиенту о перегрузке и избегаем падения по OutOfMemory

Если тут есть проблема, не понял в чем она

@FedosOnGIT FedosOnGIT self-requested a review December 27, 2023 17:36
}
}

@SuppressWarnings("unused")

Choose a reason for hiding this comment

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

Может добавить в него состояние, что она не запущенна? (когда не нужно делать flush). Тогда

  1. Ты сможешь создать её в конструкторе
  2. Не нужно будет вешать volatile, который может замедлить производительность
  3. Пропадут свистопляски с созданием и переводом в null

Choose a reason for hiding this comment

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

И думаю тогда компилятор перестанет ругаться

Copy link
Contributor Author

Choose a reason for hiding this comment

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

volatile вешать все равно нужно, так как это record и он immutable. По остальным пунктам согласен, сделал красивее.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image Компилятор не перестал ругаться :(

}
@Override
public void upsert(E entry) {
long size = inMemoryStorage.upsertAndGetSize(entry);

Choose a reason for hiding this comment

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

Получается, что если хранилище переполненно и кто-то в другом потоке сделал flush, то у тебя полетит ошибка? И ты никак это не отлавливаешь?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Не полетят если flush поспевает за вставкой данных.

Допустим, сейчас на диск ничего не флашится и хранилище переполнено.
Вызвали одновременно flush() и upsert().
Внутри flush() под writeLock'ом меняется state на RUNNING, очищается мапа и размер становится 0.
Внутри upsert() кидаются MemoryOverflowException, если размер превышет лимит и state RUNNING или FAILED. Размер и state читается под readLock'ом.

Благодаря локам все будет корректно.
Ошибку кидать разумно, так как в случае RUNNING flush() уже выполняется, и пока он выполняется мы успели опять накопить полную таблицу, не успеваем. В случае FAILED, произошла ошибка при flush(), из памяти мы не ничего удаляли чтобы не потерять. Тоже кидаем MemoryOverflowException, так как есть полная таблица и таблица на flush, что превывает лимит.

stateLock.writeLock().lock();
try {
if (flushingDaoState != null && flushingDaoState.isRunning()) {
return null;

Choose a reason for hiding this comment

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

А как мне понять, что у мне уже можно делать upsert? У меня же flush просто вернётся и я не смогу понять до каких моментов нельзя вставлять

Copy link
Contributor Author

@Dalvikk Dalvikk Dec 28, 2023

Choose a reason for hiding this comment

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

Можно добавить возвращаемое значение для flush(), но сейчас он void() и я не могу поменять код Dao.java, так как сломает код других студентов.
С точки зрения приложения-клиента разве что попытаться попозже. Ну или когда во 2 семестре будем делать уже полноценную бд на нодах можно задуматься. Если вся бд падает то в случае продовой ситуации там уже должно быть куча ошибок в логах и надо разбираться что не так :)

FileChannel offsets = FileChannel.open(offsetsPath, WRITE_OPTIONS);
Arena arena = Arena.ofConfined()) {

long offsetsSize = info.recordsCount() * Long.BYTES;

Choose a reason for hiding this comment

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

Long.BYTES лучше вынести в константу, иначе люди, которые не смотрели лекций от одноклассников, могут просто не понять что это и зачем

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Вынес в OFFSET_SIZE

@Dalvikk Dalvikk requested a review from FedosOnGIT December 29, 2023 14:06
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.

Окей вроде больше вопросов нет, поставлю 23/30

@FedosOnGIT FedosOnGIT merged commit 94e5d9b into polis-vk:main Jan 9, 2024
1 check 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