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

Guess ExchangeType if it is not set in RequestOptions #5517

Merged
merged 3 commits into from Mar 26, 2024

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Mar 19, 2024

Motivation:

ExchangeType is a hint for performance optimization. If no RequestOptions is set, it is inferred from the type of HttpRequest. However, a custom RequestOptions is set, ExchangeType is not inferred even if the RequestOptions.exchangeType() is null.

Modifications:

  • Use ExchangeType.RESPONSE_STREAMING if RequestOptions.exchangeType() is null and HttpRequest is an instance of FixedStreamMessage.
  • If none of the above conditions are met, the method defaults to returning ExchangeType.BIDI_STREAMING.

Result:

ExchangeType is guessed correctly even if RequestOptions is set.

Motivation:

`ExchangeType` is a hint for performance optimization.
If no `RequestOptions` is set, it is inferred from the type of
`HttpRequest`. However, a custom `RequestOptions` is set, `ExchangeType`
is not inferred even if the `RequestOptions.exchangeType()` is null.

Modifications:

- Use `ExchangeType.RESPONSE_STREAMING` if `RequestOptions.exchangeType()`
  is null and `HttpRequest` is an instance of `FixedStreamMessage`.

Result:

`ExchangeType` is guessed correctly even if `RequestOptions` is set.
@ikhoon ikhoon added the defect label Mar 19, 2024
@ikhoon ikhoon added this to the 1.28.0 milestone Mar 19, 2024
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.04%. Comparing base (ccb29fe) to head (8790698).
Report is 1 commits behind head on main.

❗ Current head 8790698 differs from pull request most recent head ff98952. Consider uploading reports for the commit ff98952 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5517       +/-   ##
===========================================
+ Coverage        0   74.04%   +74.04%     
- Complexity      0    20798    +20798     
===========================================
  Files           0     1801     +1801     
  Lines           0    76576    +76576     
  Branches        0     9761     +9761     
===========================================
+ Hits            0    56701    +56701     
- Misses          0    15255    +15255     
- Partials        0     4620     +4620     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Looks straightforward enough 👍 Thanks @ikhoon 🙇 👍 🙇

@jrhee17
Copy link
Contributor

jrhee17 commented Mar 19, 2024

+ Please check the lint failures 🙇

Copy link
Member

@thachlp thachlp left a comment

Choose a reason for hiding this comment

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

Updated to modification:

  • If none of the above conditions are met, the method defaults to returning ExchangeType.BIDI_STREAMING.

Could you explain this default meaning?

@ikhoon
Copy link
Contributor Author

ikhoon commented Mar 20, 2024

Good question.
Armeria requests and responses are a stream that does not hold all messages. One variant is FixedStreamMessage which contains all objects at the creation time. Reactive stream processing for backpressure is not required for the unary message type.

Otherwise, we use a streaming ExchangeType to subscribe to the message one by one when a message is written to the wire.

My answer may not be sufficient to explain Armeria's stream processors. If you want to know more about that, please let us know.

@thachlp
Copy link
Member

thachlp commented Mar 20, 2024

Good question. Armeria requests and responses are a stream that does not hold all messages. One variant is FixedStreamMessage which contains all objects at the creation time. Reactive stream processing for backpressure is not required for the unary message type.

Otherwise, we use a streaming ExchangeType to subscribe to the message one by one when a message is written to the wire.

My answer may not be sufficient to explain Armeria's stream processors. If you want to know more about that, please let us know.

Thanks for explaining to me 🙇
I just started learning Armeria :)

Copy link
Member

@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.

Great, thanks!

@jrhee17 jrhee17 merged commit 5df7f16 into line:main Mar 26, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants