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.

Sign up for GitHub

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

Jump to bottom

add migration archive old vacancies #483

Merged
merged 5 commits into from Mar 13, 2023
Merged

add migration archive old vacancies #483

merged 5 commits into from Mar 13, 2023

Conversation

usernaimandrey
Copy link
Contributor

No description provided.

@usernaimandrey
Copy link
Contributor Author

#480

@usernaimandrey
Copy link
Contributor Author

Написал пока мграцию которая архивирует вакансии старше 2-х недель, если двигаться в сторону автоматизации этого процесса то тогда нужен Redis и sidekick, ну и какой ни будь гем чтобы запускать задачу по крону например https://github.com/collectiveidea/delayed_job

grozwalker
grozwalker requested changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@grozwalker grozwalker left a comment

Choose a reason for hiding this comment

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

Давай переделаем миграцию примерно так:

vacancies = Vacancs.where(...)

vacancies.find_each do |vacancy|
   vacancy.archive!
end

db/migrate/20230221221505_archive_vacancies_older_two_weeks.rb Outdated

class ArchiveVacanciesOlderTwoWeeks < ActiveRecord::Migration[7.0]
def change
Vacancy.find_each do |vacancy|
Copy link
Contributor

Choose a reason for hiding this comment

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

Наконец-то ты дал повод что-то поправить у тебя!)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Конкретно в этой задаче у нас всего около 600 вакансий, поэтому в целом пофиг, но вообще лучше делать сразу правильнее

Copy link
Contributor

Choose a reason for hiding this comment

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

Еще момент о котором надо помнить, что вакансия может быть невалидной, поэтому в момент архивации будет выброщено исключение, поэтому этот момент надо проверять отдельно, либо убирать валидацию (но я проверил, вакансии валидные)

Copy link
Contributor

Choose a reason for hiding this comment

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

И еще одна штука о которой надо помнить, что если кол-во обрабатываемых записей слишком велико, то за время выполнения миграции (у нас в кубе это около 30 сек), то под убивается быстрее чем миграция выполняется, все откатывается и так происходит бесконечно (опять же не этот случай, но просто для справки)

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
Contributor Author

Choose a reason for hiding this comment

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

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

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

Об этом что-то не подумал, не оптимально поправлю)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Еще момент о котором надо помнить, что вакансия может быть невалидной, поэтому в момент архивации будет выброщено исключение, поэтому этот момент надо проверять отдельно, либо убирать валидацию (но я проверил, вакансии валидные)

А вроде же все данные валидируются через activeformmodel в каком случае они могут быть не валидны, если только миграцию накатили и данные стали не консистентны?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

И еще одна штука о которой надо помнить, что если кол-во обрабатываемых записей слишком велико, то за время выполнения миграции (у нас в кубе это около 30 сек), то под убивается быстрее чем миграция выполняется, все откатывается и так происходит бесконечно (опять же не этот случай, но просто для справки)

Это конечно прям новое знание, я не знал, запомню! Ну я с кубом не работал ешо)

Copy link
Contributor

Choose a reason for hiding this comment

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

А вроде же все данные валидируются через activeformmodel в каком случае они могут быть не валидны, если только миграцию накатили и данные стали не консистентны?

Ну бывает, что какие-то правила валидации добавляются уже позже и некоторые записи становятся невалидными

@grozwalker
Copy link
Contributor

ПС по поводу автоматизации, давай напишем рейк таску, которая будет архивировать старые вакансии, а я потом настрою крон джобу в хероку

https://devcenter.heroku.com/articles/scheduler

@usernaimandrey
Copy link
Contributor Author

ПС по поводу автоматизации, давай напишем рейк таску, которая будет архивировать старые вакансии, а я потом настрою крон джобу в хероку

https://devcenter.heroku.com/articles/scheduler

Хорошо

@ashikov
Copy link
Contributor

ashikov commented Feb 28, 2023

@grozwalker тыц

grozwalker
grozwalker reviewed Mar 1, 2023
View reviewed changes
db/schema.rb
@@ -10,275 +12,275 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2022_11_27_083950) do
Copy link
Contributor

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.

да уменя стал запускатся по коммиту и он почемуто игнорирует правила

@usernaimandrey
Copy link
Contributor Author

fixed

@grozwalker grozwalker merged commit 21b58dd into Hexlet:main Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grozwalker grozwalker Awaiting requested review from grozwalker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@usernaimandrey @grozwalker @ashikov