-
Notifications
You must be signed in to change notification settings - Fork 52
Fixes updating traffic shaping options throws IllegalStateException #130
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
Conversation
|
|
||
| sidecar: | ||
| host: 0.0.0.0 | ||
| port: 9043 |
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.
configuring the port to be non-zero, surfaces the issue because the logic in vert.x treats the zero port configuration differently
e57aa0b to
77b17ad
Compare
Patch by Francisco Guerrero; Reviewed by Yifan Cai for CASSANDRASC-140
77b17ad to
d837b74
Compare
sarankk
left a comment
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.
+1 Looks good to me
arjunashok
left a comment
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.
LGTM, +1
| catch (Exception exception) | ||
| { | ||
| Assertions.fail(exception); | ||
| } |
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.
I guess the catch is not necessary. Is it added because IDE complains no assertion is performed?
If so, how about this?
assertThatNoException().isThrownBy(() -> {
server.start().toCompletionStage().toCompletableFuture().get(30, TimeUnit.SECONDS);
TrafficShapingOptions update = new TrafficShapingOptions()
.setOutboundGlobalBandwidth(100 * 1024 * 1024);
server.updateTrafficShapingOptions(update);
});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.
Without the fix, the exception was thrown and I would fail the test. I will update the test. I was running @RepeatedTest so it was necessary to fail the test so the server was properly closed during teardown
yifan-c
left a comment
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.
One nit. lgtm
Patch by Francisco Guerrero; Reviewed by Yifan Cai for CASSANDRASC-140