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

g595.Manucharyan.task3 #279

Merged
merged 28 commits into from Dec 9, 2016
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
0866a60
Merge remote-tracking branch 'refs/remotes/fediq/master'
vanderwardan Nov 1, 2016
552d60a
Merge remote-tracking branch 'refs/remotes/fediq/master'
vanderwardan Nov 20, 2016
3b50057
Add third task
vanderwardan Nov 21, 2016
da7781b
Merge remote-tracking branch 'refs/remotes/fediq/master'
vanderwardan Nov 21, 2016
10c6e3d
Third task
vanderwardan Nov 21, 2016
2f58fa2
Codestyle changes
vanderwardan Nov 21, 2016
a9c4c95
add synchronised
vanderwardan Nov 21, 2016
a9f1fc8
Review changes
vanderwardan Nov 27, 2016
f530f07
code style changes
vanderwardan Nov 27, 2016
83ecff4
try do smth, that travis test my project
vanderwardan Nov 27, 2016
6446586
tried again
vanderwardan Nov 27, 2016
181536f
add cleaning storage not only in close()
vanderwardan Nov 27, 2016
af9170b
code style changes
vanderwardan Nov 27, 2016
a9a52c0
revert
vanderwardan Nov 27, 2016
f95b2d3
some bugs fixed
vanderwardan Nov 27, 2016
a85598c
try find what's wrong with travis
vanderwardan Nov 27, 2016
0ccfc8a
code style
vanderwardan Nov 27, 2016
e81d659
and here we go
vanderwardan Nov 27, 2016
81e49f0
optimise little bit
vanderwardan Nov 28, 2016
12319c8
oops
vanderwardan Nov 29, 2016
b1b8e2b
Merge remote-tracking branch 'refs/remotes/fediq/master'
vanderwardan Dec 4, 2016
e328d85
try to pass new tests
vanderwardan Dec 4, 2016
8722b58
fix problems with seek
vanderwardan Dec 5, 2016
89423e5
fix problems with files
vanderwardan Dec 5, 2016
6c53bd8
code style changes
vanderwardan Dec 5, 2016
eacb2e4
fix constructor
vanderwardan Dec 5, 2016
edcfe73
back to the right version of lector's file
vanderwardan Dec 9, 2016
47dde1d
fucking IDEA auto fixes
vanderwardan Dec 9, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,20 @@
package ru.mipt.java2016.homework.g595.manucharyan.task3;

import java.io.IOException;
import java.io.RandomAccessFile;

/**
* @author Vardan Manucharyan
* @since 30.10.16
*/
public class ConcreteStrategyDoubleRandomAccess implements SerializationStrategyRandomAccess<Double> {
@Override
public void serializeToFile(Double value, RandomAccessFile output) throws IOException {
output.writeDouble(value);
}

@Override
public Double deserializeFromFile(RandomAccessFile input) throws IOException {
return input.readDouble();
}
}
@@ -0,0 +1,20 @@
package ru.mipt.java2016.homework.g595.manucharyan.task3;

import java.io.IOException;
import java.io.RandomAccessFile;

/**
* @author Vardan Manucharyan
* @since 30.10.16
*/
public class ConcreteStrategyIntegerRandomAccess implements SerializationStrategyRandomAccess<Integer> {
@Override
public void serializeToFile(Integer value, RandomAccessFile output) throws IOException {
output.writeInt(value);
}

@Override
public Integer deserializeFromFile(RandomAccessFile input) throws IOException {
return input.readInt();
}
}
@@ -0,0 +1,33 @@
package ru.mipt.java2016.homework.g595.manucharyan.task3;

import java.io.IOException;
import java.io.RandomAccessFile;

/**
* @author Vardan Manucharyan
* @since 30.10.16
*/
public class ConcreteStrategyStringRandomAccess implements SerializationStrategyRandomAccess<String> {
@Override
public void serializeToFile(String value, RandomAccessFile output) throws IOException {
writeString(output, value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Какая клёвая реализация... А точно убрать один уровень вызовов никак нельзя? Лично мне кажется, что я вижу, как можно это сделать.
И кстати, попробуй вернуть назад writeUTF, вроде должно побыстрее начать работать :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ну, можно inline написать 😃

}

@Override
public String deserializeFromFile(RandomAccessFile input) throws IOException {
return readString(input);
}

//serializing functions
public static void writeString(RandomAccessFile output, String string) throws IOException {
output.writeInt(string.length());
output.write(string.getBytes("UTF-8"));
}

public static String readString(RandomAccessFile input) throws IOException {
int len = input.readInt();
byte[] bytes = new byte[len];
input.read(bytes);
return new String(bytes);
}
}
@@ -0,0 +1,265 @@
package ru.mipt.java2016.homework.g595.manucharyan.task3;

import java.io.File;
import java.io.IOException;
import java.io.RandomAccessFile;
import java.util.HashMap;
import java.util.Iterator;

import ru.mipt.java2016.homework.base.task2.KeyValueStorage;

import static java.lang.Math.max;
Copy link
Collaborator

@ignorer ignorer Nov 25, 2016

Choose a reason for hiding this comment

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

Здесь в последних двух абзацах довольно коротко поясняют за статик импорт, и когда его нужно/не нужно использовать.
Что касается конкретно данного случая, он не попадает ни под одно из описаний. Можешь, конечно, оставить, однако лично я удивлён, что тебе оказалось проще написать import static, нежели просто заюзать Math.max() в одном месте в коде.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

просто, когда я написал max() он предложил подключить это (там было несколько вариантов), я просто нажал enter и забил)


/**
* @author Vardan Manucharyan
* @since 20.11.2016.
*/
public class OptimisedKeyValueStorage<K, V> implements KeyValueStorage<K, V> {

private static final long MAX_CACHE_SIZE = 100L;

private SerializationStrategyRandomAccess<K> keySerializationStrategy;
Copy link
Collaborator

Choose a reason for hiding this comment

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

final же можно сделать - вряд ли ты будешь менять стратегию в процессе работы над хранилищем.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

да, врятли...

private SerializationStrategyRandomAccess<V> valueSerializationStrategy;

//consist keys and offsets(the pair of begin and length)
private HashMap<K, Long> base = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше тип переменной сделать Map, ты же внутри класса не используешь его как HashMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Странно, у меня так же было и во втором задании...

private HashMap<K, V> cache = new HashMap<>();

private long maxOffset;
private final String pathname;
private RandomAccessFile storage;
private final String storageName = "storage.txt";
private RandomAccessFile mapStorage;
private final String mapStorageName = "mapStorage.txt";

private File mutexFile; // для многопоточности
private boolean isClosed;

public OptimisedKeyValueStorage(SerializationStrategyRandomAccess<K> keySerializationStrategy,
SerializationStrategyRandomAccess<V> valueSerializaionStrategy,
String path) throws IOException {

this.keySerializationStrategy = keySerializationStrategy;
this.valueSerializationStrategy = valueSerializaionStrategy;
maxOffset = 0L;
pathname = path;

mutexFile = new File(pathname, "Mutex");
if (!mutexFile.createNewFile()) {
throw new RuntimeException("Can't synchronize!");
}

File directory = new File(pathname);
if (!directory.isDirectory()) {
throw new RuntimeException("wrong path");
}

File file = new File(pathname, storageName);
storage = new RandomAccessFile(file, "rw");

File file2 = new File(path, mapStorageName);
mapStorage = new RandomAccessFile(file2, "rw");

if (file.exists() && file2.exists()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

При создании RandomAccessFile в режиме rw создаётся и файл, который ты подал в качестве аргумента. Получается, ты только что создал файлы (а если не создал, то вылететл IOException), а потом ещё специально что-то проверяешь?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Поправил

uploadDataFromStorage();
} else if (!file.createNewFile() || file2.createNewFile()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

createNewFile() вернёт false только когда файл уже существет, то есть exist() вернул true. Такая проверка есть двумя строчками выше, так что по-моему лучше здесь if убрать, да и вообще, наверное, весь if-else блок, оставить только вызов функции. Объясни мне, зачем это ветвление здесь вообще.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true if the named file does not exist and was successfully created; false if the named file already exists. Если файл не удалось создать, то должно вернуться false, нет? Ветвление здесь вот зачем: есть два варианта, файлы уже есть, тогда загружаем их. Если же их нет, но и создать их не получилось, то что-то пошло не так

Copy link
Collaborator

Choose a reason for hiding this comment

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

Давай так.
exist возвращает true тогда и только тогда, когда файл есть.
createNewFile возвращает false тогда и только тогда, когда файл есть (ты привёл кусок документации).
[Тут чудеса матлогики]
В итоге получаем, что эти два условия эквивалентны.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ок

throw new RuntimeException("Can't create a storage!");
}

isClosed = false;
}

/**
* Возвращает значение для данного ключа, если оно есть в хранилище.
* Иначе возвращает null.
*/
@Override
public synchronized V read(K key) {
if (!exists(key)) {
return null;
} else {
try {
if (cache.get(key) != null) {
return cache.get(key);
}

Long offset = base.get(key);
storage.seek(offset);
return valueSerializationStrategy.deserializeFromFile(storage);
} catch (Exception exception) {
throw new RuntimeException("Can't read from storage");
}
}
}

/**
* Возвращает true, если данный ключ есть в хранилище
*/
@Override
public boolean exists(K key) {
if (isClosed) {
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут бы IllegalStateException был кстати. А вообще эту проверку лучше в отдельную функцию вынести. А вот в close() не надо, он при повторном закрытии ничего не должен делать.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

У меня в close() и не было ничего) А так да, мне приходила эта идея в голову, но я забил)

}
if (cache.containsKey(key)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Идея предложила тебе заменить на return cache.containsKey(key) || base.containsKey(key), а ты её игноришь. Не надо так.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ну так же понятней, нет? Типа тут односложные ифы, и сразу понятно, что происходит, иначе надо думать(

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ну, не знаю, по мне так два однотипных условия в одном выглядят читабельнее. Вообще не суть.

return true;
}
return base.containsKey(key);
}

/**
* Записывает в хранилище пару ключ-значение.
*/
@Override
public synchronized void write(K key, V value) {
if (isClosed) {
throw new RuntimeException("Can't write: storage is closed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

А в read() такой проверки нет, кстати. И да, здесь лучше IllegalStateException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Поправил

Copy link
Collaborator

Choose a reason for hiding this comment

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

Нет, не поправил. В read() проверки всё ещё нет.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ааа, пропустил

} else {
cache.put(key, value);
base.put(key, 0L);

if (cache.size() > MAX_CACHE_SIZE) {
writeCacheToStorage();
}
}
}

/**
* Удаляет пару ключ-значение из хранилища.
*/
@Override
public synchronized void delete(K key) {
if (isClosed) {
throw new RuntimeException("Can't delete: storage is closed");
} else {
cache.remove(key);
base.remove(key);
}
}

/**
* Читает все ключи в хранилище.
* <p>
* Итератор должен бросать {@link java.util.ConcurrentModificationException},
* если данные в хранилище были изменены в процессе итерирования.
*/
@Override
public Iterator<K> readKeys() {
if (isClosed) {
throw new RuntimeException("Can't iterate: storage is closed");
} else {
return base.keySet().iterator();
}
}

/**
* Возвращает число ключей, которые сейчас в хранилище.
*/
@Override
public int size() {
if (isClosed) {
throw new RuntimeException("Can't know size: storage is closed");
} else {
return base.size();
}
}

@Override
public synchronized void close() {
Copy link
Collaborator

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.

Да, точно

writeCacheToStorage();
reorganiseStorage();
downnloadDataToStorage();
try {
mapStorage.close();
storage.close();
} catch (IOException excetion) {
throw new RuntimeException("Can't close storage");
}
isClosed = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

В finally лучше запихнуть.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Сделано!

cache.clear();
base.clear();
mutexFile.delete();
}

private void uploadDataFromStorage() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

В моём понимании upload - это когда что-то загружают на крупный ресурс, а download - скачивают с него. И тогда получается, что upload to, download from. А в данном случае ещё и вышеприведённая логика инвертирована.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ок, хотя мне кажется это немного странным

try {

int count = -1;
count = mapStorage.readInt();

for (int i = 0; i < count; i++) {
K key = keySerializationStrategy.deserializeFromFile(mapStorage);
Long tmp = mapStorage.readLong();
base.put(key, tmp);
maxOffset = max(maxOffset, tmp);
}
} catch (IOException exception) {
base.clear();
//throw new RuntimeException("Trouble with storage.db");
}
}

private void downnloadDataToStorage() {
try {
mapStorage.close();
File file = new File(pathname, mapStorageName);
assert (file.delete());
file = new File(pathname, mapStorageName);
mapStorage = new RandomAccessFile(file, "rw");

mapStorage.writeInt(size());
for (HashMap.Entry<K, Long> entry : base.entrySet()) {
keySerializationStrategy.serializeToFile(entry.getKey(), mapStorage);
mapStorage.writeLong(entry.getValue());
}
} catch (IOException exception) {
throw new RuntimeException("Trouble with storage.db");
}
}

private void writeCacheToStorage() {
if (isClosed) {
throw new RuntimeException("Can't write: storage is closed");
} else {
try {
for (HashMap.Entry<K, V> entry : cache.entrySet()) {
storage.seek(maxOffset);
valueSerializationStrategy.serializeToFile(entry.getValue(), storage);
long curOffset = storage.getFilePointer();
base.put(entry.getKey(), maxOffset);
maxOffset = curOffset;
}
cache.clear();

} catch (IOException exception) {
throw new RuntimeException("Can't write cache on the disk");
}
}
}

private void reorganiseStorage() {
try {
File file = new File(pathname, "newStorage.txt");
RandomAccessFile newStorage = new RandomAccessFile(file, "rw");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Через DataOutputStream можно было бы наклепать, обернув BufferedOutputStream. Побыстрее должно работать.

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 для каждого элемента, а у DataOutputStream нет такого функционала, или я что-то не понимаю?

Copy link
Collaborator

Choose a reason for hiding this comment

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

size()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Да, стало работать быстрее, спасибо)


assert (cache.isEmpty());

for (HashMap.Entry<K, Long> entry : base.entrySet()) {
storage.seek(entry.getValue());
Long tmp = newStorage.getFilePointer();
valueSerializationStrategy.serializeToFile(read(entry.getKey()), newStorage);
base.put(entry.getKey(), tmp);
}

storage.close();
File file1 = new File(pathname, storageName);
assert (file1.delete());

newStorage.close();
assert (file.renameTo(file1));

} catch (IOException exception) {
throw new RuntimeException("Can't reorganise storage!");
}
}
}

@@ -0,0 +1,15 @@
package ru.mipt.java2016.homework.g595.manucharyan.task3;

import java.io.IOException;
import java.io.RandomAccessFile;

/**
* @author Vardan Manucharyan
* @since 30.10.16
*/
public interface SerializationStrategyRandomAccess<Value> {

void serializeToFile(Value value, RandomAccessFile output) throws IOException;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вместо RandomAccessFile лучше сделать DataOutput здесь - он является родителем RandomAccessFile, но при этом код становится гибче.


Value deserializeFromFile(RandomAccessFile input) throws IOException;
Copy link
Collaborator

Choose a reason for hiding this comment

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

То же, но про DataInput. Как я и говорил на семинаре (и не на одном), лучше не цепляться за конкретную реализацию.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Исравил

}
@@ -0,0 +1,24 @@
package ru.mipt.java2016.homework.g595.manucharyan.task3;

import java.io.IOException;
import java.io.RandomAccessFile;

import ru.mipt.java2016.homework.tests.task2.StudentKey;

/**
* @author Vardan Manucharyan
* @since 30.10.16
*/
public class ConcreteStrategyStudentKeyRandomAccess implements SerializationStrategyRandomAccess<StudentKey> {

@Override
public void serializeToFile(StudentKey value, RandomAccessFile output) throws IOException {
output.writeInt(value.getGroupId());
ConcreteStrategyStringRandomAccess.writeString(output, value.getName());
}

@Override
public StudentKey deserializeFromFile(RandomAccessFile input) throws IOException {
return new StudentKey(input.readInt(), ConcreteStrategyStringRandomAccess.readString(input));
}
}