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

Add RequestLogAccess.getIfAvailable(RequestLogProperty) #4956

Closed
ikhoon opened this issue Jun 14, 2023 · 2 comments · Fixed by #4966
Closed

Add RequestLogAccess.getIfAvailable(RequestLogProperty) #4956

ikhoon opened this issue Jun 14, 2023 · 2 comments · Fixed by #4966
Labels
good first issue new feature
Milestone
1.25.0

Comments

@ikhoon
Copy link
Contributor

ikhoon commented Jun 14, 2023 •
edited

Both RequestLogAccess.isAvailable() and RequestLogAccess.ensureAvailable() should be used to safely get a RequestLog when the given RequestLogProperty.

armeria/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseHandler.java

Lines 116 to 118 in 0ce077e

if (reqCtx.log().isAvailable(RequestLogProperty.RESPONSE_CAUSE)) {
cause = reqCtx.log().ensureAvailable(RequestLogProperty.RESPONSE_CAUSE).responseCause();
}

The code style looks verbose and is not efficient because each operation accesses a volatile variable and computes the available flags.

If we provide an API that returns RequestLog only when the given flags are available, it is performant and Kotlin users may take advantage of null safe calls.

interface RequestLogAccess {
  @Nullable
  RequestLog getIfAvailable(RequestLogProperty... properties);

  @Nullable
  RequestLog getIfAvailable(Iterable<RequestLogProperty> properties);
}

val cause: Throwable? = 
 ctx.log()
    .getIfAvailable(RequestLogProperty.RESPONSE_CAUSE)?.responseCause()
@Kyoungwoong
Copy link
Contributor

Can i take this one?

@ikhoon
Copy link
Contributor Author

ikhoon commented Jun 15, 2023

For sure. Thanks in advance. 🙇‍♂️

Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 18, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
72eb408 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 18, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
c591723 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
@trustin trustin linked a pull request Jun 19, 2023 that will close this issue
Add getIfAvailable API (#4956) #4966
Merged
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 19, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
a2febdd 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 19, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
80d88a3 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 19, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
f3942ba 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 19, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
5e858e1 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 19, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
b9dcc77 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 19, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
08592f0 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 19, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
c1a81b5 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
Kyoungwoong added a commit to Kyoungwoong/armeria that referenced this issue Jun 19, 2023
@Kyoungwoong
Add getIfAvailable API (line#4956)
4e8fa20 
Modifications:

Introduce new API for accessing RequestLog only when specific flags are available.
Provide Kotlin users with the ability th utilize null-safe callls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
ikhoon pushed a commit that referenced this issue Jul 30, 2023
@Kyoungwoong
Add getIfAvailable API (#4956) (#4966)
cddfd77 
Motivation:

The current implementation of RequestLog access in AbstractHttpResponseHandler is verbose and potentially inefficient. Each operation checks the availability of RequestLog properties separately, accessing a volatile variable and computing the available flags multiple times. Additionally, the code style does not align with efficient and concise Kotlin programming practices.

Modifications:

Introduce new API for accessing RequestLog only when specific flags are available
Provide Kotlin users with the ability to utilize null-safe calls

Result:

Improved efficiency of RequestLog access by reducing redundant checks and computations
Enhanced compatibility for Kotlin developers by enabling null-safe calls when accessing RequestLog properties
@jrhee17 jrhee17 added this to the 1.25.0 milestone Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue new feature
Projects
None yet
Milestone
1.25.0
Development

Successfully merging a pull request may close this issue.

Add getIfAvailable API (#4956) Kyoungwoong/armeria
3 participants
@ikhoon @jrhee17 @Kyoungwoong