Осокин Дмитрий, Политех, hw 1 #22

osokindm · 2024-02-20T10:30:52Z

No description provided.

codeclimate · 2024-02-20T10:31:41Z

src/main/java/ru/vk/itmo/test/osokindm/Converter.java

+import java.lang.foreign.ValueLayout;
+import java.nio.charset.StandardCharsets;
+
+public class Converter {


Add a private constructor to hide the implicit public one.

codeclimate · 2024-02-20T10:31:41Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

@@ -0,0 +1,109 @@
+package ru.vk.itmo.test.osokindm;
+
+import one.nio.http.*;


Avoid unused imports such as 'one.nio.http'

codeclimate · 2024-02-20T10:31:41Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+    }
+
+
+    @Path("/v0/entity")


'METHOD_DEF' has more than 1 empty lines before.

codeclimate · 2024-02-20T10:31:41Z

src/main/java/ru/vk/itmo/test/osokindm/dao/MergingEntryIterator.java

+import ru.vk.itmo.dao.Entry;
+
+import java.lang.foreign.MemorySegment;
+import java.util.*;


Using the '.' form of import should be avoided - java.util..

codeclimate · 2024-02-20T10:31:41Z

src/main/java/ru/vk/itmo/test/osokindm/dao/TableSet.java

+import ru.vk.itmo.dao.Entry;
+
+import java.lang.foreign.MemorySegment;
+import java.util.*;


Using the '.' form of import should be avoided - java.util..

codeclimate · 2024-02-20T10:31:42Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

@@ -0,0 +1,109 @@
+package ru.vk.itmo.test.osokindm;
+
+import one.nio.http.*;


Avoid unused imports such as 'one.nio.http'

codeclimate · 2024-02-20T10:31:42Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+        super.stop();
+    }
+
+    @Path("/v0/entity")


The String literal '/v0/entity' appears 4 times in this file; the first occurrence is on line 62

codeclimate · 2024-02-20T10:31:42Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+
+    @Path("/v0/entity")
+    @RequestMethod(Request.METHOD_GET)
+    public Response get(@Param(value = "id=", required = true) String id) {


The String literal 'id=' appears 4 times in this file; the first occurrence is on line 64

codeclimate · 2024-02-20T10:31:42Z

src/main/java/ru/vk/itmo/test/osokindm/TestServer.java

+import java.nio.file.Path;
+import java.util.List;
+
+public class TestServer {


All methods are static. Consider using a utility class instead. Alternatively, you could add a private constructor or make the class abstract to silence this warning.

codeclimate · 2024-02-20T10:31:42Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

@@ -0,0 +1,109 @@
+package ru.vk.itmo.test.osokindm;
+
+import one.nio.http.*;


Using the '.' form of import should be avoided - one.nio.http..

codeclimate · 2024-02-20T11:50:21Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+public class HttpServerImpl extends HttpServer {
+
+    private static final int MEMORY_LIMIT = 8388608;
+//    private static final int MEMORY_LIMIT = 1024;


This block of commented-out lines of code should be removed.

codeclimate · 2024-02-20T11:50:21Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+public class HttpServerImpl extends HttpServer {
+
+    private static final int MEMORY_LIMIT = 8388608;
+//    private static final int MEMORY_LIMIT = 1024;


Comment has incorrect indentation level 0, expected is 4, indentation should be the same level as line 26.

Marashov-Alexander

Провел ревью кода, чуть позже вернусь к ревью нагрузочного тестирования)

Marashov-Alexander · 2024-02-25T09:15:12Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+
+public class HttpServerImpl extends HttpServer {
+
+    private static final int MEMORY_LIMIT = 8388608;


Константа всё равно магическая получается) Лучше объяснять, откуда это число взялось и что оно означает. Для того, чтоб объяснить, откуда оно взялось, можно использовать выражение: 8 * 1024 * 1024. Оно всё равно посчитается единожды, зато читающему код будет понятно его происхождение. И в название константы лучше добавить единицы измерения. Например, MEMORY_LIMIT_BYTES

Marashov-Alexander · 2024-02-25T09:19:16Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+    public HttpServerImpl(ServiceConfig config) throws IOException {
+        super(createServerConfig(config));
+        workingDir = config.workingDir();
+        daoWrapper = new DaoWrapper(new Config(config.workingDir(), MEMORY_LIMIT));


Для чего здесь создается объект DaoWrapper, если он же создается в методе start?
Созданное в конструкторе дао перетирается объектом из start

Marashov-Alexander · 2024-02-25T09:22:07Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+    private DaoWrapper daoWrapper;
+    private final java.nio.file.Path workingDir;
+
+    private final Logger logger = LoggerFactory.getLogger(HttpServerImpl.class);


logger - объект класса, однако создается вне конструктора. Если создается единственный конструктор для инициализации полей, то по возможности лучше все поля в нём объявлять. Однако logger думаю вообще можно сделать статическим полем. Здесь и во всех других классах

Marashov-Alexander · 2024-02-25T09:23:54Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+    private static final int MEMORY_LIMIT = 8388608;
+    private static final String PATH = "/v0/entity";
+    private static final String ID_REQUEST = "id=";
+    private DaoWrapper daoWrapper;


Мутабельные поля лучше указывать после иммутабельных) Чисто по стилю удобнее понимать код

Marashov-Alexander · 2024-02-25T09:31:44Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+    @Override
+    public synchronized void stop() {
+        try {
+            daoWrapper.stop();


Получается, что сервер отвечает за жизненный цикл dao: он и создает его, и останавливает, и использует. Кажется, слишком много ответственности. Может сделать так, чтоб ServiceImpl управлял не только жизненным циклом сервера, но еще и dao? А объект dao передавать в server в качестве зависимости. Тогда и принцип единой ответственности будет лучше соблюдаться, и класс service будет более полезным) И в TestServer тоже можно будет использовать Service

Marashov-Alexander · 2024-02-25T09:33:04Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+        daoWrapper = new DaoWrapper(new Config(config.workingDir(), MEMORY_LIMIT));
+    }
+
+    private static HttpServerConfig createServerConfig(ServiceConfig serviceConfig) {


приватные методы лучше указывать после публичных. Приватные методы - это обычно детали реализации, а читающему код чаще всего интересны в первую очередь публичные методы

Marashov-Alexander · 2024-02-25T09:34:40Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+public class HttpServerImpl extends HttpServer {
+
+    private static final int MEMORY_LIMIT = 8388608;
+    private static final String PATH = "/v0/entity";


думаю есть смысл здесь тоже конкретизировать, что это за PATH. А то это слово встречается в разных контекстах в этом классе, поэтому добавить постфикс лишним не будет

Marashov-Alexander · 2024-02-25T09:40:10Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+    }
+
+    @Path(PATH)
+    @RequestMethod(Request.METHOD_POST)


Тогда уж стоило поддержать и остальные http-методы)) Не post'ом единым

Может удобнее для этой цели было бы сделать один метод, в котором в зависимости от http-метода выполнять ту или иную логику? А в случае неподдерживаемого типа возвращать то, что возвращается в этом методе

Marashov-Alexander · 2024-02-25T09:51:11Z

src/main/java/ru/vk/itmo/test/osokindm/HttpServerImpl.java

+
+    @Override
+    public void handleRequest(Request request, HttpSession session) throws IOException {
+        String id = request.getParameter(ID_REQUEST);


похоже, что этот метод уже вызывается за нас в super.handleRequest. Возможно имеет смысл не делать двойной пробег, а сразу сделать, что нужно? Например, здесь и на null проверить, и может метод тоже проверить да разроутить...)

Marashov-Alexander

В связи с недочетами в коде и некоторыми вопросами по профилированию готов поставить 12 баллов

Marashov-Alexander · 2024-02-25T19:49:47Z

src/main/java/ru/vk/itmo/test/osokindm/wrk_scripts/result_hw1.md

+## Нахождение точки разладки
+### GET
+Размер БД = 2.1gb
+


Похоже, что в этом месте не хватает вывода wrk

Marashov-Alexander · 2024-02-25T19:52:05Z

src/main/java/ru/vk/itmo/test/osokindm/wrk_scripts/result_hw1.md

+Transfer/sec:    523.42KB
+```
+
+Повторное выполнение запроса на пустой БД не приводит к существенному изменению результатов, в связи с тем, что созданные записи не просматриваются при вставке.


Не очень понял, что тут имелось в виду

Marashov-Alexander · 2024-02-25T19:53:24Z

src/main/java/ru/vk/itmo/test/osokindm/wrk_scripts/result_hw1.md

+99.999%   33.95ms
+100.000%   33.98ms
+```
+Если изменить скрипт lua и наполнять БД, обращаясь по ключам с уже существующими значениями, то можно заметить, что время работы также не изменилось  (1.19ms vs 1.17ms).


Лучше сравнивать не средние значения, а перцентили с девятками. Так точность сравнения будет существенно выше

Marashov-Alexander · 2024-02-25T19:59:11Z

src/main/java/ru/vk/itmo/test/osokindm/wrk_scripts/result_hw1.md

+
+Больше всего выделений памяти (8%) уходило на Mapped MemorySegment
+
+На данный момент все запросы выполняются внутри SelectorThread, чего быть по идее не должно, поэтому возможна оптимизация путем делегирования задачи WorkerPool'у


Да, создается ощущение, что SelectorThread должен другими делами заниматься) Посмотрим, что получится сделать на следующем этапе

Marashov-Alexander · 2024-02-25T19:59:54Z

src/main/java/ru/vk/itmo/test/osokindm/wrk_scripts/profile_get_8000.svg

Думаю, лучше прикреплять флеймграфы в виде html,чтобы проверяющие могли потыкаться в результаты

Marashov-Alexander · 2024-02-25T20:06:20Z

src/main/java/ru/vk/itmo/test/osokindm/wrk_scripts/put.lua

@@ -0,0 +1,33 @@
+minId = 1


тут и в get - кажется, что маленький диапазон допустимых рандомных айдишек, чтоб говорить о точных результатах профилирования

* hw1 first try

hw1 first try

0ab3565

codeclimate bot reviewed Feb 20, 2024

View reviewed changes

incubos requested a review from lamtev February 20, 2024 10:40

incubos assigned lamtev Feb 20, 2024

hw1 first try

cdd1344

codeclimate bot reviewed Feb 20, 2024

View reviewed changes

lamtev requested review from Marashov-Alexander and removed request for lamtev February 21, 2024 19:19

lamtev assigned Marashov-Alexander and unassigned lamtev Feb 21, 2024

hw1 first try

0bc4a3c

Marashov-Alexander reviewed Feb 25, 2024

View reviewed changes

Marashov-Alexander approved these changes Feb 25, 2024

View reviewed changes

Marashov-Alexander added 3 commits February 29, 2024 14:45

Merge branch 'main' into hw1

a886609

Merge branch 'main' into hw1

0e8b54c

Merge branch 'main' into hw1

014ffc3

Marashov-Alexander merged commit f10fd34 into polis-vk:main Mar 1, 2024
2 checks passed

osokindm added a commit to osokindm/2024-highload-dht that referenced this pull request Mar 6, 2024

Осокин Дмитрий, Политех, hw 1 (polis-vk#22)

db179ee

* hw1 first try

axothy pushed a commit to axothy/2024-highload-dht that referenced this pull request Mar 13, 2024

Осокин Дмитрий, Политех, hw 1 (polis-vk#22)

cbf9210

* hw1 first try

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Осокин Дмитрий, Политех, hw 1 #22

Осокин Дмитрий, Политех, hw 1 #22

osokindm commented Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

codeclimate bot Feb 20, 2024

Marashov-Alexander left a comment

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander left a comment

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

Marashov-Alexander Feb 25, 2024

		@@ -0,0 +1,109 @@
		package ru.vk.itmo.test.osokindm;

		import one.nio.http.*;


		public class HttpServerImpl extends HttpServer {

		private static final int MEMORY_LIMIT = 8388608;


		Больше всего выделений памяти (8%) уходило на Mapped MemorySegment

		На данный момент все запросы выполняются внутри SelectorThread, чего быть по идее не должно, поэтому возможна оптимизация путем делегирования задачи WorkerPool'у

Осокин Дмитрий, Политех, hw 1 #22

Осокин Дмитрий, Политех, hw 1 #22

Conversation

osokindm commented Feb 20, 2024

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

codeclimate bot Feb 20, 2024

Choose a reason for hiding this comment

Marashov-Alexander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marashov-Alexander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment