Skip to content

Conversation

@Tushkan
Copy link
Contributor

@Tushkan Tushkan commented Mar 3, 2017

#46 при не возможности получить лицензию делаем паузу и пробуем загрузить файл еще раз

oscript-library#46 при не возможности получить лицензию делаем паузу и пробуем загрузить файл еще раз
Copy link
Member

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.

при написании этой заглушки больше задавался вопросом: 10 сек ожидания это много или мало
А бесконечный цикл: какое количество повторов цикла считать оптимальным для ожидания лицензии?

Copy link
Member

Choose a reason for hiding this comment

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

Не знаю, не сталкивался. Надо посмотреть, за сколько повторов она всё-таки находится, и смело помножить на два :)

Copy link
Member

Choose a reason for hiding this comment

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

Надо посмотреть, за сколько повторов она всё-таки находится, и смело помножить на два :)

Эх, не умеют программисты правильно оценивать плановое время задачи.
Нужно же на 3 умножать :)

Copy link
Member

Choose a reason for hiding this comment

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

и ещё два добавить тогда уж.
прикинем: при выгрузке заданием меня бы устроило, если выгрузка зависает минут на 5-10. При задержке в 10 секунд это 30-60 повторений.

Copy link
Member

Choose a reason for hiding this comment

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

ненене! 10 секунд обратно верни! 5 минут - это я говорил в сумме :) когда с командной строки получишь задержку в 5 минут - сдуреешь :(

@artbear
Copy link
Member

artbear commented Mar 3, 2017

Такую правку предлагаю не принимать.
Часто удобно сразу получить отлуп и падение билда, чем ждать, пока лицензии подключатся.
Это важно на CI

Например, буквально на днях проводил клиенту пару настроек CI на разных машинах-агентах, ребята-инфраструктурщики каждый раз неверно настраивали файл лицензий.
Если бы не быстрый отлуп, можно было бы долго разбираться, что не так.
Даже 5 минут это много.

Максимум - предлагаю добавить параметр-флаг ком.строки, который управляет поведение из PR
При наличии этого флага можно включать указанный режим

@Tushkan
Copy link
Contributor Author

Tushkan commented Mar 3, 2017

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

@dmpas
Copy link
Member

dmpas commented Mar 3, 2017

@artbear 10 секунд и 30 повторов самое то. Если руками запускаешь - сразу багу увидишь, если заданием - тоже не зависнет и упадёт в конце концов

@artbear
Copy link
Member

artbear commented Mar 3, 2017

@Tushkan при не возможности получить лицензию делаем паузу и пробуем загрузить файл еще раз

Мне нравится именно эта исходная формулировка к PR.
небольшая пауза, еще одна попытка и отлуп в случае неудачи.
Для CI здесь сразу сможет работать оповещение заинтересованных лиц и оперативное исправление проблемы при необходимости.
Зачем лишний раз нагружать сервер, в это время другие сборки будут простаивать :(

@dmpas 10 сек и 30 раз = 5 минут. Долговато для меня :(

ИМХО нормальный вариант параметризация этого параметра.

@EvilBeaver
Copy link
Member

@artbear если все устраивает - надо влить.

@artbear
Copy link
Member

artbear commented Mar 6, 2017

@EvilBeaver Меня как раз не устраивает вариант возможной паузы в 5 минут из-за проблем с лицензией.
Выше я описал свои возражения.
Я против принятия этого PR в таком состоянии

@EvilBeaver
Copy link
Member

@Tushkan сделаешь параметром время задержки?

@Tushkan
Copy link
Contributor Author

Tushkan commented Mar 15, 2017

сделаю

@Tushkan
Copy link
Contributor Author

Tushkan commented Mar 24, 2017

@artbear @dmpas посмотрите реализацию
P.S. не пойму как посмотреть чем вызваны конфликты

@dmpas
Copy link
Member

dmpas commented Mar 24, 2017

@Tushkan там мои доработки влили. Сделай у себя мердж с девелопной веткой.

@Tushkan
Copy link
Contributor Author

Tushkan commented Mar 24, 2017

@dmpas смержил

Copy link
Member

Choose a reason for hiding this comment

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

Сделай, плиз, отдельный метод для установки получения количество циклов ожидания.
Экспортные переменные у класса это зло.
В свое время мы зря не отрефакторили экспортные ДоменПочтыДляGitПоУмолчанию и ВерсияПлатформы

А вообще классу "Менеджер синхронизации" нужен хороший пост-конструктор для правильной инициализации.

PS @EvilBeaver Конструкторы для классов!!

Copy link
Member

Choose a reason for hiding this comment

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

Врег(ТекстОшибки) дважды выполняется в 2х условиях.
Предлагаю сначала получить ВРег, а уже затем результат сравнивать.

@Tushkan
Copy link
Contributor Author

Tushkan commented Mar 24, 2017

Внес исправления
Добавил: если параметр указать равным 0, тогда получение лицензии зацикливается

Copy link
Member

Choose a reason for hiding this comment

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

1 Не увидел бесконечного цикла, реализованного в коде :(
2 Нужно документировать поведение - если 0, то бесконечность.

Copy link
Member

Choose a reason for hiding this comment

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

ИМХО странный вызов ПолучитьКоличествоЦикловОжиданияЛицензии()
проще сразу юзать КоличествоЦикловОжиданияЛицензии, как и в следующей строке :)

Copy link
Member

Choose a reason for hiding this comment

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

Вечный цикл увидел.
ИМХО здесь путаница с названиями, нужно переименовать локальную переменную КоличествоЦикловОжиданияЛицензии, чтобы было четкое различие с методом с аналогичным именем ПолучитьКоличествоЦикловОжиданияЛицензии

Copy link
Member

Choose a reason for hiding this comment

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

Ну и документировать все равно нужно :)

@artbear artbear merged commit f4aeb1d into oscript-library:develop Mar 24, 2017
@Tushkan Tushkan deleted the patch-1 branch March 25, 2017 05:33
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.

4 participants