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

Feat(defaultRequestTimeout) add defaultRequestTimeout property for the client #1132

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Feat(defaultRequestTimeout) add defaultRequestTimeout property for the client #1132

wants to merge 20 commits into from

Conversation

batsura-sa
Copy link

@batsura-sa batsura-sa commented Aug 3, 2024 •
edited
Loading

Hi! Could we add deadline property for client? @ST-DDT who will do review? what I'am doing wrong?

ST-DDT
ST-DDT requested changes Sep 2, 2024
View reviewed changes
...ent-spring-boot-starter/src/main/java/net/devh/boot/grpc/client/deadline/DeadlineHelper.java Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/java/net/devh/boot/grpc/client/config/GrpcChannelProperties.java Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/java/net/devh/boot/grpc/client/config/GrpcChannelProperties.java Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/java/net/devh/boot/grpc/client/config/GrpcChannelProperties.java Outdated Show resolved Hide resolved
tests/src/test/java/net/devh/boot/grpc/test/setup/DeadlineTests.java Outdated Show resolved Hide resolved
...ent-spring-boot-starter/src/main/java/net/devh/boot/grpc/client/deadline/DeadlineHelper.java Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the enhancement A feature request or improvement label Sep 2, 2024
@batsura-sa batsura-sa force-pushed the feature/deadline_property branch 4 times, most recently from 1094f3f to a19dbef Compare September 3, 2024 13:51
@batsura-sa batsura-sa marked this pull request as draft September 3, 2024 14:30
@batsura-sa batsura-sa marked this pull request as ready for review September 3, 2024 16:54
ST-DDT
ST-DDT reviewed Sep 3, 2024
View reviewed changes
...rter/src/main/java/net/devh/boot/grpc/client/interceptor/DeadlineSetupClientInterceptor.java Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/java/net/devh/boot/grpc/client/config/GrpcChannelProperties.java Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/java/net/devh/boot/grpc/client/config/GrpcChannelProperties.java Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/java/net/devh/boot/grpc/client/config/GrpcChannelProperties.java Outdated Show resolved Hide resolved
...c/main/java/net/devh/boot/grpc/client/autoconfigure/GrpcClientDeadlineAutoConfiguration.java Outdated Show resolved Hide resolved
...c/main/java/net/devh/boot/grpc/client/autoconfigure/GrpcClientDeadlineAutoConfiguration.java Outdated Show resolved Hide resolved
batsura-sa and others added 2 commits September 5, 2024 08:15
@batsura-sa @ST-DDT
Update grpc-client-spring-boot-starter/src/main/java/net/devh/boot/gr…
42ebf40 
…pc/client/interceptor/DeadlineSetupClientInterceptor.java

Co-authored-by: ST-DDT <[email protected]>
@batsura-sa @ST-DDT
Update grpc-client-spring-boot-starter/src/main/java/net/devh/boot/gr…
809bc5b 
…pc/client/interceptor/DeadlineSetupClientInterceptor.java

Co-authored-by: ST-DDT <[email protected]>
ST-DDT
ST-DDT reviewed Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only a few small changes, and then this is good to go.

...c/main/java/net/devh/boot/grpc/client/autoconfigure/GrpcClientDeadlineAutoConfiguration.java Outdated Show resolved Hide resolved
...c/main/java/net/devh/boot/grpc/client/autoconfigure/GrpcClientDeadlineAutoConfiguration.java Outdated Show resolved Hide resolved
tests/src/test/java/net/devh/boot/grpc/test/setup/DeadlineTests.java Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/resources/META-INF/additional-spring-configuration-metadata.json Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/java/net/devh/boot/grpc/client/config/GrpcChannelProperties.java Outdated Show resolved Hide resolved
@genuss
Copy link
Contributor

genuss commented Sep 5, 2024

What do you think about calling the configured property timeout instead of deadline? I believe the current name create some confusion. Deadline in context of GRPC has special meaning and that is "An absolute point in time", although the configured property is a duration.

@batsura-sa @ST-DDT
Update grpc-client-spring-boot-starter/src/main/java/net/devh/boot/gr…
5a219a7 
…pc/client/autoconfigure/GrpcClientDeadlineAutoConfiguration.java

Co-authored-by: ST-DDT <[email protected]>
@genuss
Copy link
Contributor

genuss commented Sep 6, 2024 •
edited
Loading

OK. For example grpc.client.GLOBAL.timeout=1s ? right?

Exactly 👍

@batsura-sa batsura-sa changed the title Feat(deadline) add deadline property for client Feat(timeout) add timeout property for client Sep 6, 2024
@batsura-sa batsura-sa changed the title Feat(timeout) add timeout property for client Feat(timeout) add timeout property for the client Sep 6, 2024
@batsura-sa
Copy link
Author

OK. For example grpc.client.GLOBAL.timeout=1s ? right?

Exactly 👍

Done, not sure about the names. We can improve the source code or comments if you think it is necessary.

ST-DDT
ST-DDT reviewed Sep 6, 2024
View reviewed changes
...rc/main/java/net/devh/boot/grpc/client/autoconfigure/GrpcClientTimeoutAutoConfiguration.java Outdated Show resolved Hide resolved
...rc/main/java/net/devh/boot/grpc/client/autoconfigure/GrpcClientTimeoutAutoConfiguration.java Outdated Show resolved Hide resolved
...arter/src/main/java/net/devh/boot/grpc/client/interceptor/TimeoutSetupClientInterceptor.java Outdated Show resolved Hide resolved
...arter/src/main/java/net/devh/boot/grpc/client/interceptor/TimeoutSetupClientInterceptor.java Outdated Show resolved Hide resolved
...pring-boot-starter/src/main/resources/META-INF/additional-spring-configuration-metadata.json Outdated Show resolved Hide resolved
tests/src/test/java/net/devh/boot/grpc/test/setup/TimeoutSetupTests.java Outdated Show resolved Hide resolved
batsura-sa and others added 5 commits September 6, 2024 12:11
@batsura-sa @ST-DDT
Update grpc-client-spring-boot-starter/src/main/java/net/devh/boot/gr…
0ebcc0a 
…pc/client/autoconfigure/GrpcClientTimeoutAutoConfiguration.java

Co-authored-by: ST-DDT <[email protected]>
@batsura-sa @ST-DDT
Update grpc-client-spring-boot-starter/src/main/java/net/devh/boot/gr…
6ab4d0d 
…pc/client/autoconfigure/GrpcClientTimeoutAutoConfiguration.java

Co-authored-by: ST-DDT <[email protected]>
@batsura-sa @ST-DDT
Update grpc-client-spring-boot-starter/src/main/java/net/devh/boot/gr…
e6403b6 
…pc/client/interceptor/TimeoutSetupClientInterceptor.java

Co-authored-by: ST-DDT <[email protected]>
@batsura-sa @ST-DDT
Update grpc-client-spring-boot-starter/src/main/resources/META-INF/ad…
20dc197 
…ditional-spring-configuration-metadata.json

Co-authored-by: ST-DDT <[email protected]>
Feat(timeout) constructor and test changed.
9db6998
@batsura-sa
Copy link
Author

batsura-sa commented Sep 6, 2024 •
edited
Loading

May be rename the timeout property to requestTimeout? Like this "grpc.client.GLOBAL.requestTimeout=1s". IMHO timeout is really abstract property name.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Sep 6, 2024

May be rename the timeout property to requestTimeout? Like this "grpc.client.GLOBAL.requestTimeout=1s". IMHO timeout is really abstract property name.

requestTimeout or defaultRequestTimeout?

@batsura-sa
Copy link
Author

batsura-sa commented Sep 6, 2024 •
edited
Loading

May be rename the timeout property to requestTimeout? Like this "grpc.client.GLOBAL.requestTimeout=1s". IMHO timeout is really abstract property name.

requestTimeout or defaultRequestTimeout?

I like both, can you choose one? May be defaultRequestTimeout is better.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Sep 6, 2024

I like both, can you choose one? May be defaultRequestTimeout is better.

Then defaultRequestTimeout it is.

@batsura-sa
Copy link
Author

batsura-sa commented Sep 6, 2024 •
edited
Loading

I like both, can you choose one? May be defaultRequestTimeout is better.

Then defaultRequestTimeout it is.

Done. Please let me know if we can do comments or code better.

@batsura-sa @ST-DDT
Update grpc-client-spring-boot-starter/src/main/java/net/devh/boot/gr…
da2f32c 
…pc/client/config/GrpcChannelProperties.java

Co-authored-by: ST-DDT <[email protected]>
@batsura-sa batsura-sa changed the title Feat(timeout) add timeout property for the client Feat(defaultRequestTimeout) add defaultRequestTimeout property for the client Sep 7, 2024
ST-DDT
ST-DDT approved these changes Sep 7, 2024
View reviewed changes
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Thanks for your contribution. ❤️

@batsura-sa
Copy link
Author

Looks good to me. Thanks for your contribution. ❤️

Thanks for the review, it was very helpful. Is it possible to see this feature in the release? What needs to be done for this?

@ST-DDT
Copy link
Collaborator

ST-DDT commented Sep 7, 2024

The maintainers have to review/merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

Assignees
No one assigned
Labels
enhancement A feature request or improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@batsura-sa @genuss @ST-DDT