ДЗ№4. Тяпуев Дмитрий. Магистратура. Политех #202
Conversation
lamtev
requested review from
Marashov-Alexander
and removed request for
lamtev
There was a problem hiding this comment.
10/25
Из важных замечаний:
- Нейминг InMemoryDao, в котором сидит энергонезависимое хранилище. -1 балл
- Не оптимальный get -3 балла
- Есть кейсы, при которых общая арена не будет закрыта -3 балла
- Использование названия рабочей таблицы при проведении compaction -3 балла
- Потеря арены при getWriteBufferToSsTable -3 баллов
- Мутабельная статическая переменная. Нейминги статических переменных -1 балл
- Лишние аллокации: массив лонгов для возвращения значений, стримы при подсчете количества ssTable'ов -1 балл
There was a problem hiding this comment.
Окончательный результат после второй проверки: 20/25
Из важных замечаний:
- Упускается кейс с кол-вом таблиц=1 и memTable.isEmpty - в таком случае не нужно компактить. -1 балл
- Закрытие арены в методе вручную, без try-with-resources. -1 балл
- Траблы со StorageHelper: наличие состояния, protected поля и методы вперемешку со статическими: -1 балл
- Лишняя аллокация в случае проблем или отсутствия файлов (создание пустого массива файлов). -1 балл
- Не последовательность: закрытие арены без проверки, в то время как в следующей ветке проверка осуществляется: -0.5 балла
- Метод с аргументом, который используется лишь в половине случаев в зависимости от другого аргумента-флага. -0.5 балла
| @@ -91,27 +103,47 @@ public void compact() throws IOException { | |||
| return; | |||
| } | |||
| Iterator<Entry<MemorySegment>> dataIterator = get(null, null); | |||
| Arena writeArena = Arena.ofConfined(); | |||
There was a problem hiding this comment.
Лучше создавать под try-with-resources. Иначе, в случае исключения между созданием и закрытием, будет потеря памяти
| @@ -91,27 +103,47 @@ public void compact() throws IOException { | |||
| return; | |||
There was a problem hiding this comment.
Не рассматривается еще один кейс: если количество таблиц == 1, а memTable is empty, то компактить тоже нет смысла
| StorageHelper.renameCompactedSsTable(ssTablePath); | ||
| compacted = true; | ||
| } | ||
|
|
||
| @Override | ||
| public void close() throws IOException { | ||
| if (compacted) { | ||
| readArena.close(); |
There was a problem hiding this comment.
думаю, проверка на isAlive должна быть выше всех. Иначе вы не проверяете на isAlive, а просто закрываете арену
| @@ -33,10 +33,16 @@ static MemorySegment getReadBufferFromSsTable(Path ssTablePath, Arena readArena) | |||
|
|
|||
| static MemorySegment getWriteBufferToSsTable(Long writeBytes, | |||
| Path ssTablePath, | |||
| int ssTablesQuantity) throws IOException { | |||
| int ssTablesQuantity, | |||
There was a problem hiding this comment.
Сейчас метод принимает два проблемных аргумента: ssTablesQuantity и compactionFlag. Первый аргумент лишний потому, что не используется в половине кейсов использования. А второй аргумент этому способствует)
Как насчет того, чтоб передавать в метод уже готовый Path? Тогда необходимости в тех двух аргументах отпадет, а сам метод будет выполнять единственную задачу - мапинг файла
| @@ -33,10 +33,16 @@ static MemorySegment getReadBufferFromSsTable(Path ssTablePath, Arena readArena) | |||
|
|
|||
| static MemorySegment getWriteBufferToSsTable(Long writeBytes, | |||
| Path ssTablePath, | |||
| int ssTablesQuantity) throws IOException { | |||
| int ssTablesQuantity, | |||
There was a problem hiding this comment.
Сейчас метод принимает два проблемных аргумента: ssTablesQuantity и compactionFlag. Первый аргумент лишний потому, что не используется в половине кейсов использования. А второй аргумент этому способствует)
Как насчет того, чтоб передавать в метод уже готовый Path? Тогда необходимости в тех двух аргументах отпадет, а сам метод будет выполнять единственную задачу - мапинг файла
There was a problem hiding this comment.
У меня логика построена так, что при создании sstable учитывается их количество. Поэтому надо знать- компактим мы или просто пишем. Изначально у меня для этого было 2 метода, но я решил так оптимизировать. Да и codeclimate не позволял иметь много кода в одном файле, так что пришлось разделять логику по классам.
| public void save(Iterable<Entry<MemorySegment>> memTableEntries, Path ssTablePath) throws IOException { | ||
| MemorySegment buffer = NmapBuffer.getWriteBufferToSsTable(StorageHelper.getSsTableDataByteSize(memTableEntries), | ||
| Arena writeArena = Arena.ofConfined(); |
There was a problem hiding this comment.
try-with-resources
| renamed = remainingFile.renameTo(new File(newFilePath)); | ||
| } | ||
| if (!directory.exists() || !directory.isDirectory()) { | ||
| return new File[0]; |
There was a problem hiding this comment.
лучше возвращать null, чтоб не делать лишних аллокаций
There was a problem hiding this comment.
Codeclimate на null ругался и указал так сделать
| throw new IllegalStateException("Utility class"); | ||
| } | ||
| protected static final String COMPACTED_FILE_NAME = "compact"; | ||
| protected long memTableEntriesCount; |
There was a problem hiding this comment.
если storageHelper имеет состояние, которое используется только в Storage, то лучше это состояние положить прямо в Storage
А то часть методов у хелпера статическая, а часть - protected. Это выглядит странно. protected должен использовать наследниками, а тут наследования никакого нет
No description provided.