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
Request connection on startup #377
Request connection on startup #377
Conversation
...configure/src/main/java/net/devh/boot/grpc/client/channelfactory/AbstractChannelFactory.java
Outdated
Show resolved
Hide resolved
Maybe we could change the |
98abcf5
to
3d3c8f0
Compare
That's a great idea. Applied it. |
Could you please add a test for this feature?
I can help you with that, if necessary. |
I created three tests and arranged them in one file. I think it’s ok to group them so.
|
Do you have some ideas why build fails? I can't see issues. |
ImmediateConnectTest$ImmediateConnectEnabledAndSuccessfulTest > testFailingCall() FAILED
java.lang.IllegalStateException: Failed to create channel: test
at net.devh.boot.grpc.client.inject.GrpcClientBeanPostProcessor.processInjectionPoint(GrpcClientBeanPostProcessor.java:126)
[...]
Caused by:
java.lang.IllegalStateException: Can't connect to channel test
at net.devh.boot.grpc.client.channelfactory.AbstractChannelFactory.connectOnStartup(AbstractChannelFactory.java:172)
at net.devh.boot.grpc.client.channelfactory.AbstractChannelFactory.newManagedChannel(AbstractChannelFactory.java:140) |
I cannot reproduce the error at my place either. But I assume, that the server might be a bit slow starting up. As an alternative you can also try to uncomment:
in the main |
This issue with tests was the following. A channel can move to
And I applied your suggestion to wait for ready state. When I actually made it, I understood you're right. The code looks much better now 👍 . |
import io.grpc.ConnectivityState; | ||
import io.grpc.ManagedChannel; | ||
import io.grpc.ManagedChannelBuilder; | ||
import io.grpc.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No star imports please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
...configure/src/main/java/net/devh/boot/grpc/client/channelfactory/AbstractChannelFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality wise this is already good to go 👍 . Just some minor changes left.
tests/src/test/java/net/devh/boot/grpc/test/setup/ImmediateConnectTest.java
Outdated
Show resolved
Hide resolved
tests/src/test/java/net/devh/boot/grpc/test/setup/ImmediateConnectTest.java
Outdated
Show resolved
Hide resolved
tests/src/test/java/net/devh/boot/grpc/test/setup/ImmediateConnectTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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 addressing all the small issues this fast.
Thanks guys! Looking forward to release 🚀 |
FYI: I try to finish the remaining issues within the next two weeks and do a release afterwards. |
Solves #376
Looks like I was a bit too optimistic about this task. It's not that easy to check that connection was established in GRPC. It doesn't even have such thing as "connection timeout" becase GRPC internally handles connections and retries on failures.
I came out with this solution:
ConnectivityState
state-machine it should work like this:channel.getState(true)
, registers two callbacks and awaits onCountDownLatch
for some period of time.ConnectivityState
is in wrong status or timeout has passed then we throw exception which stops context from starting.Well any feedback will be highly appreciated. If you suppose design is OK, I'll make connection timeout configurable too.