Комментарии 33
Как же сложно. Это решается одним статическим методом в утиле. Строчек на 20-30 примерно.
result = MyHttpUtils.callWithRetry(() -> originalCall(...), retryCount, delay) или прямо в него оригинальные параметры вызова передать, как больше нравится. Или экспоненциальная задержка, тоже как больше нравится.
Здравствуйте, спасибо за комментарий. Да, так тоже можно. Это решение приведет к тому, что постепенно мы скопируем всю расписанную тут логику в предлагаемые Вами метод. А когда решим масштабироваться, начнем делать копипаст. Для меня было важно донести технологию решения проблемы. Решить ее можно многими способами.
Код делающий примерно тоже самое из документации https://docs.oracle.com/en/cloud/paas/app-container-cloud/cache/handle-connection-exceptions-retries.html И даже он переусложнен. Объект тут не нужен, да и класс не нужен. Одного метода хватит. Сделаем скидку что там Джава слишком старая. Даже без лямбд.
У вас Спринг, тесты, Спринг стартер, Спринг магия. А потом что-то код медленно работает, что-то ничего не понятно что на самом деле происходит. Вот из-за такого оверинжиниринга непонятно и медленно.
Сложно стоит писать сложные штуки. А простые штуки надо писать максимально просто. Ретраи это просто. Как обычно с подводными камнями вроде добавки джиттера, не забыть про возможность экспоненциальных ретраев и вообще помнить про то чтобы не убить запросами пошатнувшегося соседа. Но все равно просто.
Чем описанная Вами простота проще, чем описанная мной?
Когда ты работаешь с фреймворком, тебе следует использовать возможности Фреймворка, потому что - просто сопровождать, просто обновлять, просто масштабировать.
Назначение сложного продукта, понятно решать сложные задачи.
У нас нет проблем с медленной работой, но за ссылку спасибо, обязательно посмотрю )
Чем описанная Вами простота проще, чем описанная мной?
У меня 20 строк кода. Который явно вызывается и явно работает. Что он делает и как он это делает поймет любой джун минут за 10.
У вас космолет какой-то. Сроки на понять а что там происходит резко растут. Начать использовать мой код можно секунд за 10. Просто вызвать функцию. Как ваш код использовать надо думать. Я не хочу думать над тем как вызывать функцию.
Мой код отлично сочетается как с функциональным стилем, так и с императивным. Хотите в Either обернуть и в стримах гонять? Легко! Хотите в hot path запихнуть и делать все с минимумом аллокаций? Тоже легко. У вас кажется с этим будут проблемы.
просто сопровождать, просто обновлять, просто масштабировать
Этот код стейтлесс, его не надо масштабировать. Сколько раз вызывали столько и отработает.
Сопровождать его в общем тоже не надо, он просто работает десятилетиями. Но если что он простой и любой джун внесет в него нужные правки.
Назначение сложного продукта, понятно решать сложные задачи.
Продукт может быть сколько угодно сложен. Это нормально. А вот ретраи http просты. И их стоит написать просто. Сложно надо писать только сложные вещи.
Когда ты работаешь с фреймворком, тебе следует использовать возможности Фреймворка
Когда у тебя в руках есть только молоток, тогда все вокруг превращается в гвозди.
У нас нет проблем с медленной работой
Это глобально про такой подход. Если все в таком стиле написать, то возникают разные проблем. Проблема с производительностью это одна из них.
Используя Фреймворк и все его возможности Вы уходите от того, чтобы в руках был только молоток.
Предлагаемое решение раскладывает по полкам разные уровни решения и представления задачи.
Каждый уровень имеет возможность для расширений и дополнений.
Каждый уровень имеет понятный тест, который просто поддерживать и развивать.
Перед глазами понятно, на что ретраим, понятно, как ретраим.
Если мы все заворачиваем в единую реализацию и отдаем джуну, то мы формируем ком функциональности, который начнет жить собственной жизнью через какое-то время.
Да, такой подход предполагает, что в нем нужно разобраться, но в любом коде нужно разобраться, чтобы применять его к месту.
Вы мой ненаписанный код обвинили в серьезных преступлениях. Давайте я его напишу для справедливости
private static HttpClient client = HttpClient.newHttpClient();
private static Set<Integer> retryableCodes = Set.of(HTTP_BAD_GATEWAY, HTTP_UNAVAILABLE, HTTP_GATEWAY_TIMEOUT); //расширьте по вкусу
/*
Дописывайте свои специфичные реализации, кому что надо
*/
public static @Nullable String httpGetStringWithRetries(HttpRequest request, int retryCount, int delay) {
Optional<HttpResponse<String>> result = httpGetWithRetries(request, retryCount, delay, false, HttpResponse.BodyHandlers.ofString());
return result.map(HttpResponse::body).orElse(null);
}
public static <T> Optional<HttpResponse<T>> httpGetWithRetries(HttpRequest request,
int tryCount,
int delay,
boolean isExponentDelay,
HttpResponse.BodyHandler<T> bodyHandler) {
if (tryCount < 1) {
throw new RuntimeException("Нельзя меньше 1 попытки");
}
if (delay < 0) {
throw new RuntimeException("Машину времени еще не придумали. Задержка между попытками должна быть неотрицательной.");
}
int retryDelay = delay;
for (int i = 0; i < tryCount; ++i) {
if (i > 0 && delay > 0) {
try {
int jitter = (int) (ThreadLocalRandom.current().nextInt() % (retryDelay * 0.2)) - (int) (retryDelay * 0.1);
Thread.sleep(retryDelay + jitter);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
}
HttpResponse<T> result = null;
try {
result = client.send(request, bodyHandler);
} catch (Exception e) {
if (!(e instanceof IOException))
throw new RuntimeException(e);
}
if (result == null || retryableCodes.contains(result.statusCode())) {
if (isExponentDelay)
retryDelay = retryDelay * 2;
continue;
}
return Optional.of(result);
}
return Optional.empty();
}Смыслового кода около 20 строк, как я и обещал. И он поддерживает все основные фичи любых ретраев, без них еще меньше будет. Джун за 10 минут разберется, тут я тоже не соврал.
Плюс у моего кода нет зависимостей. Вообще. Его можно использовать откуда угодно и он вообще ничего не тянет за собой. В отличии от вашего. Что я считаю большим плюсом.
Если нужна версия с еще меньшим количеством аллокаций, то как ее написать понятно. Не знаю зачем, но вдруг.
А теперь попробуйте в моем коде показать все те ужасы которые вы перечислили.
Что вы хотите расширять и дополнять? Ну серьезно.
Где тут уровни, полки и прочий буллшит? Тут 20 строк несложного кода, ничего другого я не вижу.
Что и как ретраим отлично видно. В том числе из вызывающего кода понятно что происходит. Прям с одного взгляда.
Ком функциональности покажите. Ну или опишите путь как он появится. С учетом что когда джун правит коммоны его ревьюят.
Разбираться в 20 строчках? Ну, блин.
Можете писать смело. Считайте что я на ревью вам принес ПР с удалением вашего кода из проекта и его заменой на этот кусок. С обоснованием "я тут хорошо сделал". Отморозиться и закрыть ПР мой кто я такой у вас варианта нет. Я уверен в себе и своем коде и готов эскалировать к руководству.
Если мы посчитаем строчки кода в моем решении и строчки кода в Вашем решении, то в Вашем решении их не окажется меньше с учетом структурированности и форматирования. Да и с
Я сильно сомневаюсь, что джун с ходу (10 минут) разберется в чем-то типа
ThreadLocalRandom.current()
Разберётся так, что сможет протестировать и взять на поддержку.
Вложенность некоторых try и if до 3 уровня и все в одном модуле, что сильно затрудняет читаемость. Чисто стилистически я бы предпочел так не делать. Но это стилистические предпочтения.
Каждый if - своя ответственность, которая потенциально обрастает своим окружением. Например.
Если Вам нужно будет какой-то код обработать особо. Например 401, то вот тут
if (result == null || retryableCodes.contains(result.statusCode())) ->
Появятся дополнения и все это прямо тут ...
ThreadLocalRandom.current()
Это уже много лет как рекомендованный способ получения случайного числа. Это прямо в комментариях к методу написано.
Даже если джун не в курсе поймет по клику провалившись в метод.
Разберётся так, что сможет протестировать и взять на поддержку.
Этот код не надо поддерживать. Он реально способен работать десятилетиями без изменений. Пока что-то глобальное в мире не изменится, вроде кодов ответа http или http клиента.
Разберется так что поймет что и как там делается и сможет осознанно это использовать.
Если Вам нужно будет какой-то код обработать особо. Например 401, то вот тут
Никаких особых обработок. Это утиль метод. Только ретраи того что мы хотим ретраить всегда в любом вызывающем коде.
На ревью такое изменение зарубать надо. Ему место где-то в другом методе.
Cчитайте что я на ревью вам принес ПР с удалением вашего кода из проекта и его заменой на этот кусок. С обоснованием "я тут хорошо сделал".
В общем я мерджу. Возражений нет.
Даже если джун не в курсе поймет по клику провалившись в метод.
Вот уже мы начинаем с Вами уходит в дискуссию по вопросу, который и для Вас, очевидно, понятен, что "джун может не разобраться". Пограничное состояние между очевижно/сложно/требует погружения пройдено и Вы так же подтверждаете, что код требует того, чтобы в нем разобрались. А с учетом того, что у Вас кода много, он в одном месте, веток развития много, логика не инкапсулирована и каждый if отвечает за свое, цикломатичность возрастает.
На ревью такое изменение зарубать надо. Ему место где-то в другом методе.
Почему? Логика работы со многими авторизационными механизмами предполагает перехват 401 сообщения. У Вас секреты устарели и их надо обновить. Вы поймаете 401 и обновляйте. Если это предлагаемый подход - это ком. Если все разложено по полочкам, то каждая полка дает возможность в рамках ответственности полки сделать необходимые действия и не погружаться дальше
В общем я мерджу. Возражений нет.
Конечно, мерджите. Ваш мердж, Ваша поддержка и развитие сделанного )
Вот уже мы начинаем с Вами уходит в дискуссию по вопросу, который и для Вас, очевидно, понятен, что "джун может не разобраться".
Ну блин. Мои джуны точно способны провалиться в метод стандартной либы и прочитать 3 строчки о том что он делает. Тем более что метод стандартнее некуда.
логика не инкапсулирована и каждый if отвечает за свое, цикломатичность возрастает
20 строк. Это точно много? Ифчики под которыми одна строка это точно сложно?
Не забывайте что у вас спрингмагия во все поля. В которой сеньор рискует на день закопаться.
Почему? Логика работы со многими авторизационными механизмами предполагает перехват 401 сообщения.
Ретраи не для авторизации. Ретраи не для секретов. Ретраи не для ловли четырехсоток. Это просто ретраи флапающих ошибок сети или удаленного сервера. Зачем вы пытаетесь туда запихать что-то не то?
Сделать рядом метод httpGetWithAuth(..) с вашей типовой авторизацией и секретами это хорошая и полезная идея. Но это отдельная функция. Которая внутри себя имеет право вызывать httpGet с ретраями. И httpGetWithAuth тоже делается без спингмагии. Просто статический метод, просто параметры.
Конечно, мерджите. Ваш мердж, Ваша поддержка и развитие сделанного )
Это не надо развивать. Это не надо поддерживать. У этого не надо обновлять зависимости. Это не зависит от внешнего кода. Один раз написали и оно 10-20 лет просто работает. Возможно и дольше. С нулем затрат на поддержку. Представляете как круто?
Попробуйте писать с минимумом Спринга там где его польза неочевидна. Можно передать параметром? Передавайте параметров. Можно без бина? Значит без бина. Можно просто отдельную функцию? Значит просто отдельная функция. Саплаеры и подобное хорошо работают для явного вызова функций в обратную сторону. И даже страшное: через new объекты можно создавать. Это часто имеет смысл. Спрингом заинжектили всякие сервисы и даошки и хватит, остальное вероятно не нужно. Все связи сразу становятся очевидны, а где искать реально исполняющийся код понятно. Нет всех этих наслоений ничего не делающих классов, которые нужны только чтобы Спрингу понравиться. Код от этого становится заметно проще и удобнее в эксплуатации. У вас виден сильный оверинжинеринг, там где он совсем не нужен.
Не нравятся мне ваши три параметра tryCount, delay и isExponentDelay. Их придётся либо выдумывать из головы каждый раз при обращении к этому методу, либо брать из какого-то конфига, снова каждый раз. Если их сложить в объект, то пользоваться таким методом будет куда проще, при этом код менее понятным не станет.
Ещё что-то мне статический HttpClient не нравится. Была же какая-то причина, по которой метод send не статический?
Напишите рядом реализацию с дефолтами. Ну там 3, 200, false. Эти параметры специально параметры. Нормально когда они разные в разных случаях. Внешние сервисы разные бывают, с разными ограничениями и разными ситуациями.
В клиент можно передать кое-какие параметры. Вроде прокси и тому подобного. Вероятность того что они нужны разные в одном проекте минимальна. Если такое есть, то тоже можно написать. Send threadsafe нет проблем.
Напишите рядом реализацию с дефолтами. Ну там 3, 200, false. Эти параметры специально параметры.
Вы вообще мне отвечаете или кому? Я разве предлагал убрать параметры?
Эти параметры специально параметры. Нормально когда они разные в разных случаях.
Но ненормально когда они все настраиваются по-разному.
Все одинаково. Пара чиселок.
В жизни ситуации бывают разные. Есть у вас два стула три сервиса. Один сервис авторизации. Быстрый, хайлоад, запас железа под ним х20, вызывается на каждом запросе. Работать без него вы не можете, кешировать его запрещено. Как будете ретраить? Я бы поставил 3 запроса с задержкой 5-10мс. Флап сети пережить.
Рядом есть у вас новенький небольшой ML сервис обогащающий ваши ответы. Включен экспериментом на 5 процентов юзеров. Владельцы сервиса слезно просят их не дудосить железа уних мало и обещают прикрутить квоты завтра (с). Как будете ретраить? Я бы ретраил 1 раз с задержкой 50-100мс.
И еще более рядом есть сервис раздающий огромный кеш для вас. Оттуда качаете какой-то стейт размеров в сотни мегабайт. Качаете асинхронно конечно. По крону условному раз в час. Как будете ретраить? Я бы ретраил экспоненциально раз 10-20, сроком этак до 30 минут.
И это далеко не все случаи. Две циферки и один булин помогают их все разрулить. Для типовых случаев можно сделать типовые параметры.
Ну и что мешает их разрулить сгруппировав? Группировка трёх параметров в объект никоим образом не помешает иметь разные настройки для разных сервисов.
Вы же сами писали: иногда объекты и создавать можно, даже через new.
А ваш подход однажды приведёт к вот этому, и удачной отладки:
HttpRequest authRequest = …;
HttpHelpers.httpGetWithRetries(authRequest, 10, 3, false, …);Эта ошибка будет "спать" до тех пор пока с сервисом авторизации что-то не случится, после чего она "добьёт" его.
Советую к шаблону докрутить callback, который будет вызван при повторе/падении запроса.
Так метрики можно будет собирать не только на уровне API (только коды ответов и тд), но и этой обертки (сколько из этих запросов были повторены).
Реализовали у себя очень похожую штуку.
А зачем нужна аннотация Configurable над SomeServiceConfiguration? Это ведь не обычный класс, а управляемый Spring'ом бин. К тому же, для инициализации бинов, помеченный аннотацией Bean необходима аннотация Configuration (которая как раз и решит проблему инжекта, т.е. надобность в Configurable отпадет).
Доброе утро,
А зачем нужна аннотация Configurable над SomeServiceConfiguration
Это именно обычный класс. Не bean, а обычный класс. Делать его bean излишне. Он не нужен в контексте до момента инициализации обработки. Именно для этих целей, с точки зрения Spring, туда будет корректно повесить Configurable.
Подробнее можно посмотреть тут - https://www.baeldung.com/spring-inject-bean-into-unmanaged-objects
Спасибо за ответ.
Да, про Configurable знаю. Просто не совсем понимал, зачем это в данном случае применяется. Но как я понимаю, если мы весь SomeServiceConfiguration пометим как Configuration, то объявленные бины применятся ко всем FeignClient. А в описанном кейсе из статьи мы хотим сделать данную конфигурацию к определенным клиентам (например, указав в FeignClient(configuration = SomeServiceConfiguration)). Но в таком кейсе Configurable не особо нужен, т.к. все зависимости успешно зарезолвятся и без него (при этом можно заменить Autowired на constructor-injection)
А вы не смотрели spring-retry? Может был смысл писать решение на базе него? Если не подошёл, напишите пожалуйста в чём причина.
Здравствуйте, если Вы про это решение - https://www.baeldung.com/spring-retry, то не смотрели. Для нас акцент был сделан именно на Feign, как используемом клиенте
В тесте shouldThrownRetryException, если на вход передать параметр успешного статуса, например, 200, то тест завершится успешно, что неправильно.
Здраствуйте, да, Вы правы. Этот тест задуман для другого. Он проверяет механизм retry. Проверка статусов на предмет соответствия тех, которые внесены в List тут не описан.
Тогда зачем там вообще статусы этому тесту, если ни на что не влияют?
Если уж делать это тест правильно, то блок
doThrow(RetryableException.class)
.when(CheckRetry5xxStatuses).retry(any(Response.class), anyString());
вообще надо убрать, и прогонять сенды для всех возможных HTTP статусов и проверять, что только для шестерых "избранных" будут ретраи, а для остальных - ретраев быть не должно.
А иначе тестируется, что метод возвращает то, что "должен вернуть" (замоканное в тесте же поведение).
И еще один момент.
Использовать в тесте провайдер тестовых данных в том виде, как это сделано в ProvideRetryableStatuses, то есть, извлекая данные из тестируемого класса - плохая практика (и не зря в самом начале метод retryableStatuses() был у вас объявлен как private).
Потому что, если кто-то изменит список (не важно по каким причинам и каким способом), который этот метод возвращает, тем самым нарушив контракт работы класса, то тест это никак не отследит.
В тесте лучше явно прописывать те выходные данные, которые ожидаются от тестируемой функциональности, а не путем запроса этих данных у самой функциональности. Да, это выглядит не так лаконично и изящно, но для этого тесты и нужны.
А иначе снова тестируется, что сервис возвращает то, что "должен вернуть" (заданное в самом сервисе).
Вот тут вопрос спорный. Точка зрения понятна. Если есть более аргументированное обоснование того, какие данные проверять, то буду рад за ссылки. Дублировать в тесте полный набор данных для проверки корректности текущего состояния - правильно, но долго/трудозатратно. Этот список будет расти вместе с кодом. Проверить в юните корректность поведения при существующих данных на мой взгляд неплохо. Поинт о том, что кто-то/что-то/когда-то изменит и не проверит то, что он сделал соответствует парадигме того, что код нужно писать так, как будто его будет поддерживать психопат, но я не уверен(не созрел) к тому, чтобы соответствовать этой парадигме. Написать тест на то, что если по коду ошибка не соответствует тому, что есть в списке -> покажи поведение, да надо, это сделает тест покрытие более полным.
А как проверяются сетевые ошибки, например когда вообще не удалось подключиться к сервису?
Здраствуйте, чтобы ответить на Ваш вопрос надо сделать небольшой рефрен в сторону того, как осуществляется сетевой обмен и что такое сетевая устойчивость, ориентированная на обработку статусов http.
Шаблоны поддержки сетевой устойчивости предполагают, что Вы установили соединение по url и с отвечающий стороны есть компонент, который орбрабатывает Ваши соединения. Если компоненты (nginx/tomcat/...), обрабатывающей Ваши запросы нет, то Вам и отвечать некому. Это можно воспроизвести с помощью вызова через любой доступный инструмент curl/postman/insonmnia/... по произвольному адресу, произвольного запроса.
Если же Вы находитесь в экзотической ситуации, и соединение то есть, то его просто нет (исчез/удален/перемещен), но при этом Вы уверены в том, что он там должен быть/появится, то попробуйте обработать свой вызов через ->
...
try
{вызов ресурса}
catch
(отлов ошибки)
{логика повторных вызовов}
...
Чего-то более интеллектуального по тому контексту, что Вы написали и предложить/подсказать не могу )
Retry or No?