Коротких Виктор / ИТМО DWS / Stage 5 #263
Conversation
This reverts commit 4ae0bff.
# Conflicts: # src/main/java/ru/vk/itmo/test/viktorkorotkikh/FactoryImpl.java # src/main/java/ru/vk/itmo/viktorkorotkikh/LSMDaoImpl.java # src/main/java/ru/vk/itmo/viktorkorotkikh/SSTable.java
This reverts commit ad1e904.
| // entry already was in memTable, so we need to substructure subtract size of previous entry | ||
| memTableByteSize.addAndGet(-Utils.getEntrySize(previous)); | ||
| } | ||
| memTableByteSize.addAndGet(Utils.getEntrySize(entry)); |
There was a problem hiding this comment.
лучше было бы локально посчитать разницу и один раза прибвать в атомик. Добавление в атомик не дешевая оператция
| if (memTableByteSize.get() >= flushThresholdBytes) { | ||
| throw new LSMDaoOutOfMemoryException(); | ||
| } | ||
| Entry<MemorySegment> previous = storage.put(entry.key(), entry); |
There was a problem hiding this comment.
тут есть небольшая проблема: мы можем успеть вставить в мапу больше чем можно, потому что параллельные зпросы решат что место еще есть, хотя место будет только на один запрос
There was a problem hiding this comment.
Согласен. Но я не могу придумать хорошего решения, чтобы удовлетворить это требование и то, которое выше:
лучше было бы локально посчитать разницу и один раза прибвать в атомик. Добавление в атомик не дешевая оператция
Наивное решение - сделать другое условие:
if (memTableByteSize.addAndGet(newEntrySize) - newEntrySize >= flushThresholdBytes) {
memTableByteSize.addAndGet(-newEntrySize);
throw new LSMDaoOutOfMemoryException();
}
Тут мы решим проблему с тем, что какие-то потоки успевают вставить, когда на самом деле не должны. Но тут возникает проблема с многочисленными вызовами addAndGet, потому что ниже на 62 строчке нам всё равно придётся проверить previous на null и вычесть (ну и вычитание при выполнении самого условия).
Вычитание убирать нехорошо, потому что тогда мы будем нечестно считать размер memTable:
Допустим, memTable заполнена под завязку и остался лишь свободный 1mb и фоново у нас идёт флаш другой полной memTable. Мы попытались сделать проверку для entry 5mb и получили эксепшн. Тогда поток, который после нас попытается вставить <1mb, тоже получит экспешн, хотя по идее он должен смочь вставить.
К тому же этот размер я использую при флаше, чтобы не итерироваться заново по всем записям в мапе и считать размер всех entity
| } | ||
|
|
||
| public boolean isEmpty() { | ||
| return memTableByteSize.compareAndSet(0, 0); |
There was a problem hiding this comment.
зачем через CAS это делать? почему бы просто не get() == 0?
| } | ||
| SSTable.save(memTable, ssTables.size(), storagePath); |
There was a problem hiding this comment.
если параллельно вызвать close() будут проблемы, все потоки как раз почти одноверменно начнут выполнять SSTable.save() - скорее всего будет ошибка и мы потеряем данные
There was a problem hiding this comment.
Исправил - добавил AtomicBoolean closed
| return newSSTables; | ||
| private void compactInBackground() { | ||
| try { | ||
| compactionLock.writeLock().lock(); |
There was a problem hiding this comment.
а зачем тут compactionLock ну и во флаше получается?
There was a problem hiding this comment.
Это нужно для того, чтобы параллельно не могли выполняться flush и compaction. Объективно, решение не самое лучше получилось - можно было сделать как Артём и выполнить это условие путём создания ExecutorService с одним рабочим потоком.
Ну либо разделить compaction на 2 стадии и залочить только стадию удаления sstable с диска и записью нового файла index.idx. Но это решение имеет много подводных камней - нужно не удалить возможно появившиеся после параллельного флаша новые sstable, нужно пределать механизм выдачи нового имени для sstable (сейчас это просто "sstable" + sstablesList.size() + ".db". Очевидно, если был параллельный флаш, то в будущем это приведёт к коллизии и мы этот параллельный флаш случайно перезапишем)
Я могу переделать это замечание
There was a problem hiding this comment.
так у нас флаш и компакт идет в одном потоке, они не будут параллельно вызываться
There was a problem hiding this comment.
Удивительно, но почему-то я считал, что у меня там не singleThreadExecutor, а newFixedThreadPool с 2-мя потоками. Я точно помню, что в какой-то момент у меня был именно private final ExecutorService bgExecutor = Executors.newFixedThreadPool(2);. Там как раз нужен был лок.
Видимо, из-за того, что я доделывал всё в последний момент, упустил из виду, что я в какой-то момент отказался от 2х потоков и указал singleThreadExecutor, и забыл лок убрать.
Так что да, вы абсолютно правы
| if (iterator.hasNext()) { | ||
| current = iterator.next(); | ||
| private void tryToFlush(boolean tolerateToBackgroundFlushing) { | ||
| upsertLock.writeLock().lock(); |
There was a problem hiding this comment.
для чего тут upsertLock.writeLock() берем?
There was a problem hiding this comment.
Если у нас 2 потока одновременно вызывают флаш, то получится, что будет 2 параллельных фоновых флаша и мы потеряем данные на некоторое время:
memTable: [entity1, entity2,....]
flushingMemTable: []
↓
memTable: []
flushingMemTable: [entity1, entity2,....]
↓
memTable: []
flushingMemTable: []
То есть одна memTable будет вне зоны видимости dao во время записи данных на диск, после записи они появятся, но это плохая база данных получается.
Но моё решение тоже не идеально. Замена таблиц тут происходит в методе prepareFlush, который вызывается после того, как был отпущен лок в методе tryToFlush. В теории может случиться та ситуация, о которой я писал выше.
Тут виной всему суровый и беспощадный рефакторинг, который заменил atomicBoolean на lock
There was a problem hiding this comment.
Ну или надо не atomicBoolean использовать, а просто делать вызов prepareFlush тут же сразу под локом, а не в runFlushInBackground
There was a problem hiding this comment.
Исправил на последний вариант - вызов prepareFlush осуществляется в tryToFlush
| if (compactionFuture != null) { | ||
| await(compactionFuture); | ||
| } | ||
| bgExecutor.awaitTermination(1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
маловатисто ждем, у нас бесконечная очередь для флаша и компакта, туда могло много тасок попасть, и секунды будет мало - потеря данных
There was a problem hiding this comment.
туда могло много тасок попасть, и секунды будет мало - потеря данных
Туда не может попасть больше 2х заданий. Флаши у нас заданы логикой - автофлаши бросают эксепшн, ручные флаши просто делают return, если есть уже запущенный фалш. Компакшнов у нас тоже не может быть больше одного - при вызове метода мы отменяем предыдущий фьючер и запускаем задание на компакшн заново.
Причём Future для обоих видов заданий мы храним во flushFuture и compactionFuture, и мы дожидаемся выше их завершения
https://github.com/polis-vk/2023-nosql-lsm/pull/263/files#diff-79201a9f5547826a664aaebf9109694f27b9f047da9deda3ae6bc1800e3e5aeeR210
https://github.com/polis-vk/2023-nosql-lsm/pull/263/files#diff-79201a9f5547826a664aaebf9109694f27b9f047da9deda3ae6bc1800e3e5aeeR213
Так что потери данных тут нет
There was a problem hiding this comment.
да, не заметил что фьючерсов дожидаемся, но все равно чего жалеть то, если все равно ожидаем что работы никакой нет, можно поставить и больше, может там на ГЦ втупим на 3 сек
|
20 баллов |
|
Проставлено в ведомость. |
No description provided.