ED-305: withdrawal repair tools - #411
Conversation
…outes and to make sure of withdrawal succeeded
keynslug
left a comment
keynslug
left a comment
There was a problem hiding this comment.
Тесты не смотрел ещё, сорямба.
|
|
||
| -spec checkout_object(revision(), object_ref()) -> object() | no_return(). | ||
| checkout_object(Revision, ObjectRef) -> | ||
| #'Snapshot'{domain = Domain} = dmt_client:checkout(Revision), |
There was a problem hiding this comment.
В dmt_client теперь же есть оптимальный checkout_object.
| ] | ||
| ], | ||
| Result |
There was a problem hiding this comment.
Какое-то немного бессмысленное изменение, не?
There was a problem hiding this comment.
Плохо почистил всё после того как вставлял printf'ы, еще приберусь.
|
|
||
| -spec process_repair(repair_scenario(), withdrawal_state()) -> process_result(). | ||
| process_repair({routing, {route_changed, Route}}, _Withdrawal) -> | ||
| {{set_timer, 0}, [{route_changed, Route}]}; |
There was a problem hiding this comment.
А зачем новый action? Чем это от continue отличается?
| -spec do_process_repair(repair_scenario(), withdrawal_state()) -> process_result(). | ||
| do_process_repair(Scenario, Withdrawal) -> | ||
| process_repair(Scenario, Withdrawal). |
There was a problem hiding this comment.
Нахожу эту функцию немного бессмысленной. 🤔 Зачем она тут?
There was a problem hiding this comment.
Это небольшой артефакт, который остался после того как я пробовал разные варианты запуска сценария починки. Сначала я эту функцию завел для единообразия с do_process_transfer, а потом оказалось что здесь делать нечего, но пока я это оставил. Выпилю, если она пустой останется.
| build_failure({route_not_found, Reason}, _Withdrawal) -> | ||
| #{code => Reason}; |
There was a problem hiding this comment.
Вообще выглядит немного странно: сценарий называется route_not_found, но при этом withdrawal просто завершится с произвольной ошибкой. Причём тут тогда route_not_found? 🤔
There was a problem hiding this comment.
Задумка была в том, что тот кто запустит восстановление по сценарию route_not_found напишет Reason, и это можно будет легко найти в логах, либо спустя время вспомнить что это был не просто "no_route_found", а что-то типа "Hand repair with no_route_found". Я упустил, что в этом случае в Reason'е может не оказаться слов про no_route_found (по умолчанию если инициатор поленится указать Reason, то будет дефолтное <<"no_route_found">>).
Тут два варианта, либо вообще Reason выпилить, либо вместо #{code => Reason} делать что-то типа #{code => <<"no_route_found, reason: ", Reason">>}. Вопрос в полезности этой фичи. Как считаешь?
There was a problem hiding this comment.
(подозреваю, что поле 'code' задумывалось как нехудожественное короткое обозначение ошибки, и значит не надо там писать никаких ризонов, короче наверное надо просто выпилить Reason и всё)
There was a problem hiding this comment.
Тут два варианта, либо вообще Reason выпилить, либо вместо
#{code => Reason}делать что-то типа#{code => <<"no_route_found, reason: ", Reason">>}. Вопрос в полезности этой фичи. Как считаешь?
Я думаю, что для такого сценарий должен по-другому называться, типа MakeFailed.
There was a problem hiding this comment.
надо просто выпилить Reason и всё)
Для текущего сценария определённо.
| -spec process_repair(ff_repair:scenario(), machine(), handler_args(), handler_opts()) -> | ||
| {ok, {repair_response(), result()}} | {error, repair_error()}. | ||
| process_repair({add_events, _} = Scenario, Machine, _Args, _Opts) -> | ||
| ff_repair:apply_scenario(ff_withdrawal, Machine, Scenario); |
There was a problem hiding this comment.
Кажется по задумке автора ff_repair ты просто процессор должен был добавить для этого сценария и передать его в ff_repair:apply_scenario/4.
| -include_lib("damsel/include/dmsl_payment_processing_thrift.hrl"). | ||
| -include_lib("damsel/include/dmsl_withdrawals_provider_adapter_thrift.hrl"). | ||
|
|
||
| -include_lib("fistful_proto/include/ff_proto_withdrawal_thrift.hrl"). |
There was a problem hiding this comment.
А зачем тут новый include? Чё-то не вижу сходу потребности.(как кстати и в предыдущем include)
| {invalid_withdrawal_status, status()}. | ||
|
|
||
| -type action() :: sleep | continue | undefined. | ||
| -type action() :: sleep | continue | undefined | {set_timer, integer()}. |
There was a problem hiding this comment.
Тогда это изменение тоже можно убрать.
…tc (but there is problem - machine Ctx not cleared by repair_scenario and it running every process_timeout, need purge ctx in process_timeout ends)
…rk some imperative messy code
2 participants
No description provided.