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

mat/rpc_queue_connection_error_handling #61

Open
wants to merge 17 commits into
base: master
from

Conversation

@mat-fs
Copy link

commented Aug 18, 2019

No description provided.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 18, 2019

Codecov Report

Merging #61 into master will decrease coverage by 0.39%.
The diff coverage is 69.23%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #61     +/-   ##
=========================================
- Coverage   74.38%   73.99%   -0.4%     
=========================================
  Files           7        7             
  Lines         367      373      +6     
  Branches       59       63      +4     
=========================================
+ Hits          273      276      +3     
- Misses         55       56      +1     
- Partials       39       41      +2
Impacted Files Coverage Δ
lib/Mojo/WebSocketProxy/Backend/JobAsync.pm 79.31% <69.23%> (-0.69%) ⬇️
lib/Mojo/WebSocketProxy/Backend/JSONRPC.pm 62.74% <0%> (-1.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d7642b...2e88e30. Read the comment docs.

mat-fs added 4 commits Aug 22, 2019
t/25-job-async-backend.t Outdated Show resolved Hide resolved
t/25-job-async-backend.t Outdated Show resolved Hide resolved
t/25-job-async-backend.t Outdated Show resolved Hide resolved
mat-fs and others added 4 commits Sep 8, 2019
Update lib/Mojo/WebSocketProxy/Backend/JobAsync.pm
Co-Authored-By: Raunak Kathuria <raunakkathuria@gmail.com>
Update t/25-job-async-backend.t
Co-Authored-By: Raunak Kathuria <raunakkathuria@gmail.com>
Update t/25-job-async-backend.t
Co-Authored-By: Zak B. Elep <zakame@zakame.net>
t/25-job-async-backend.t Outdated Show resolved Hide resolved
} else {
my ($failure) = $f->failure;
$log->warnf("method %s failed: %s", $method, $failure);
my $failure = $f->is_failed ? $f->failure // '' : 'Rpc request was cancelled. Subscription is not ready.';

This comment has been minimized.

Copy link
@tom-binary

tom-binary Sep 9, 2019

Contributor
Suggested change
my $failure = $f->is_failed ? $f->failure // '' : 'Rpc request was cancelled. Subscription is not ready.';
my $failure = $f->is_failed ? $f->failure // '' : 'RPC request was cancelled. Subscription is not ready.';

although I'd suggest splitting these out into separate done/failed/cancelled mappings, rather than putting it all under ->on_ready

This comment has been minimized.

Copy link
@mat-fs

mat-fs Sep 11, 2019

Author

@tom-binary I agree that splitting into done/failed/cancel is better, but doing so the lines of code shared among them should be duplicated which doesn't seem reasonable to me.

@@ -37,11 +38,11 @@ Returns a new instance. Required params:
=over 4
=item loop => IO::Async::Loop
=item loop => IO::Async::Loop (deprecate)

This comment has been minimized.

Copy link
@tom-binary

tom-binary Sep 9, 2019

Contributor

why deprecated? shouldn't take any extra work to support these as parameters...

This comment has been minimized.

Copy link
@mat-fs

mat-fs Sep 11, 2019

Author

As I remember @nael-binary removed these args because of their harmful effect on queue client shutdown. I'll try to test service shutdown by restoring these args and push changes if there is not any issue. @nael-binary Would you please give details about the reason behind these args and the test procedure if want to restore them?

mat-fs added 5 commits Sep 11, 2019
my ($f) = @_;
$log->debugf('->submit completion: ', $f->state);

$_->($c, $req_storage) for @$before_get_rpc_response_hook;
foreach my $hook (@$before_get_rpc_response_hooks) { $hook->($c, $req_storage) };

This comment has been minimized.

Copy link
@nael-binary

nael-binary Sep 18, 2019

Contributor

i think it was fine how it was. as in there is no need for this change

This comment has been minimized.

Copy link
@mat-fs

mat-fs Sep 18, 2019

Author

@nael-binary There is nothing wrong with using $_ here, but since there are a number of constructs around setting the default variable in different ways, I've accepted @michaelmueller-binary 's suggestion to use named variables for more readability.

try {
$result = MojoX::JSON::RPC::Client::ReturnObject->new(rpc_response => decode_json_utf8($f->get));

foreach my $hook (@$before_get_rpc_response_hooks) { $hook->($c, $req_storage, $result) };

This comment has been minimized.

Copy link
@nael-binary

nael-binary Sep 18, 2019

Contributor

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.