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

Specify why on Closed{Session,Stream}Exception #2580

Merged
merged 4 commits into from
Mar 13, 2020
Merged

Specify why on Closed{Session,Stream}Exception #2580

merged 4 commits into from
Mar 13, 2020

Conversation

trustin
Copy link
Member

@trustin trustin commented Mar 13, 2020

Motivation:

It is sometimes hard to know why a connection or a stream has been
closed or reset.

Modifications:

  • Allow a user specify a message or a cause when creating a
    Closed{Session,Stream}Exception.
  • Specify a message or a cause when creating Closed{Session,Stream}Exception
    when applicable.
  • Removed redundant instantiation of ClosedSessionExceptions.
  • Fixed a bug where ClosedSessionException is raised when
    ClosedStreamException is expected.

Result:

  • More information when diagnosing a Closed{Session,Stream}Exception
  • Fixed a bug where ClosedSessionException is raised when
    ClosedStreamException is expected.

@trustin
Specify why on Closed{Session,Stream}Exception
b589be0 
Motivation:

It is sometimes hard to know why a connection or a stream has been
closed or reset.

Modifications:

- Allow a user specify a message or a cause when creating a
  `Closed{Session,Stream}Exception`.
- Specify a message or a cause when creating `Closed{Session,Stream}Exception`
  when applicable.
- Removed redundant instantiation of `ClosedSessionException`s.
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.

Result:

- More information when diagnosing a `Closed{Session,Stream}Exception`
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
@trustin trustin added this to the 0.98.5 milestone Mar 13, 2020
@trustin trustin requested review from ikhoon and minwoox as code owners March 13, 2020 09:18
ikhoon
ikhoon approved these changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @trustin

minwoox
minwoox approved these changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks!

@codecov
Copy link

codecov bot commented Mar 13, 2020 •
edited
Loading

Codecov Report

Merging #2580 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2580      +/-   ##
============================================
- Coverage     73.44%   73.42%   -0.02%     
- Complexity    10941    10962      +21     
============================================
  Files           958      958              
  Lines         42098    42157      +59     
  Branches       5263     5273      +10     
============================================
+ Hits          30919    30954      +35     
- Misses         8480     8505      +25     
+ Partials       2699     2698       -1
Impacted Files Coverage Δ Complexity Δ
...a/com/linecorp/armeria/client/HttpChannelPool.java 80.23% <ø> (+2.28%) 65 <0> (+2) ⬆️
...rp/armeria/internal/common/Http1ObjectEncoder.java 72.98% <0%> (-0.7%) 50 <0> (ø)
...com/linecorp/armeria/server/HttpServerHandler.java 81.44% <100%> (-1.26%) 79 <1> (-1)
...armeria/client/HttpClientPipelineConfigurator.java 73.35% <100%> (ø) 35 <0> (ø) ⬇️
...p/armeria/client/retrofit2/AbstractSubscriber.java 82.92% <100%> (ø) 22 <0> (ø) ⬇️
...m/linecorp/armeria/server/Http2RequestDecoder.java 69.01% <100%> (+0.21%) 28 <0> (ø) ⬇️
...p/armeria/common/stream/ClosedStreamException.java 54.54% <55.55%> (-25.46%) 5 <5> (+2)
.../linecorp/armeria/client/Http2ResponseDecoder.java 62.01% <66.66%> (ø) 34 <0> (ø) ⬇️
...om/linecorp/armeria/client/HttpSessionHandler.java 60.6% <76.47%> (-12.63%) 33 <4> (-1)
...inecorp/armeria/common/ClosedSessionException.java 72.72% <77.77%> (-7.28%) 6 <6> (+3)
... and 18 more

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 9b8a182...a7ffa5c. Read the comment docs.

@trustin trustin merged commit c0d0f01 into line:master Mar 13, 2020
@trustin trustin deleted the detailed_close_cause branch March 13, 2020 12:08
trustin added a commit to trustin/armeria that referenced this pull request Mar 13, 2020
@trustin
Specify why on Closed{Session,Stream}Exception (line#2580)
aa664cd 
Motivation:

It is sometimes hard to know why a connection or a stream has been
closed or reset.

Modifications:

- Allow a user specify a message or a cause when creating a
  `Closed{Session,Stream}Exception`.
- Specify a message or a cause when creating `Closed{Session,Stream}Exception`
  when applicable.
- Removed redundant instantiation of `ClosedSessionException`s.
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
- Miscellaneous:
  - Use `toString()` instead of `getMessage()` to show the cause type in
    `armeria-retrofit2`

Result:

- More information when diagnosing a `Closed{Session,Stream}Exception`
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
trustin added a commit to trustin/armeria that referenced this pull request Mar 13, 2020
@trustin
Specify why on Closed{Session,Stream}Exception (line#2580)
9869df5 
Motivation:

It is sometimes hard to know why a connection or a stream has been
closed or reset.

Modifications:

- Allow a user specify a message or a cause when creating a
  `Closed{Session,Stream}Exception`.
- Specify a message or a cause when creating `Closed{Session,Stream}Exception`
  when applicable.
- Removed redundant instantiation of `ClosedSessionException`s.
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
- Miscellaneous:
  - Use `toString()` instead of `getMessage()` to show the cause type in
    `armeria-retrofit2`

Result:

- More information when diagnosing a `Closed{Session,Stream}Exception`
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
trustin added a commit to trustin/armeria that referenced this pull request Mar 13, 2020
@trustin
Specify why on Closed{Session,Stream}Exception (line#2580)
2301b1a 
Motivation:

It is sometimes hard to know why a connection or a stream has been
closed or reset.

Modifications:

- Allow a user specify a message or a cause when creating a
  `Closed{Session,Stream}Exception`.
- Specify a message or a cause when creating `Closed{Session,Stream}Exception`
  when applicable.
- Removed redundant instantiation of `ClosedSessionException`s.
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
- Miscellaneous:
  - Use `toString()` instead of `getMessage()` to show the cause type in
    `armeria-retrofit2`

Result:

- More information when diagnosing a `Closed{Session,Stream}Exception`
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
@trustin
Specify why on Closed{Session,Stream}Exception (line#2580)
abf6415 
Motivation:

It is sometimes hard to know why a connection or a stream has been
closed or reset.

Modifications:

- Allow a user specify a message or a cause when creating a
  `Closed{Session,Stream}Exception`.
- Specify a message or a cause when creating `Closed{Session,Stream}Exception`
  when applicable.
- Removed redundant instantiation of `ClosedSessionException`s.
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
- Miscellaneous:
  - Use `toString()` instead of `getMessage()` to show the cause type in
    `armeria-retrofit2`

Result:

- More information when diagnosing a `Closed{Session,Stream}Exception`
- Fixed a bug where `ClosedSessionException` is raised when
  `ClosedStreamException` is expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon ikhoon approved these changes

@minwoox minwoox minwoox approved these changes

Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
0.98.5
Development

Successfully merging this pull request may close these issues.
3 participants
@trustin @ikhoon @minwoox