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

Opt amir khasanov #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

aworldx
Copy link

@aworldx aworldx commented Aug 22, 2020

No description provided.

Copy link
Collaborator

@spajic spajic left a comment

Choose a reason for hiding this comment

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

Отличная работа, спасибо 🚌

@@ -0,0 +1,17 @@
FROM ruby:2.6.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Плюсик за докер 👍

@@ -0,0 +1,89 @@
require 'byebug'

class Seed::ReloadDataService
Copy link
Collaborator

Choose a reason for hiding this comment

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

Плюсик за вынос в отдельный процесс и согласен, что это похоже на seed

@@ -7,10 +7,19 @@

<% @trips.each do |trip| %>
<ul>
<%= render "trip", trip: trip %>
<li><%= "Отправление: #{trip.start_time}" %></li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут лучше бы оставить паршлы и рендерить коллекцией, а не через цикл @trips.each

# Проблема с загрузкой json

Основная метрика - время обработки и загрузки в бд данных из json файла.
Выделил код для загрузки в сервис (просто для удобства), написал тест на регресс, и на быстродействие.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

В первоначальном виде скрипт, загружающий данные, работает не более 20 секунд.
Это значение (20 сек) было выставлено в качестве нижнего порога для теста быстродействия.

Можно переходить к оптимизации.
Copy link
Collaborator

Choose a reason for hiding this comment

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

чёткая подготовка 👍

-> Seq Scan on trips (cost=0.00..234.00 rows=96 width=34)
Filter: ((from_id = 10) AND (to_id = 7))
```
что планировщик запроса использует перебор строк таблицы (Seq Scan) при фильтрации по городу. Поэтому необходимо добавить индексы.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


#### Как оптимизировал

- Добавил в таблицу trips составной индекс [from_id, to_id]
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
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,5 @@
class AddCityIndexesToTrips < ActiveRecord::Migration[5.2]
def change
add_index :trips, [:from_id, :to_id]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше не забывать про concurrently при создании индексов
Тут конечно не важно, но нагруженный проект можно уронить

@@ -7,4 +7,11 @@ class ActiveSupport::TestCase
fixtures :all

# Add more helper methods to be used by all tests here...
#
def benchmark_with_limit(timeout = 150, &block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants