-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
KubernetesMockServer regression in 6.3.0+ #4786
Comments
I'm not sure there have been any changes to the I do remember refactoring the dispatcher, but I think that was before 6.2. The only changes I can track are the ones from #4430. |
If I simply change the fabric8 version the test slowness goes away. It is slow/errors in 6.3.0/6.4.0 but 6.2.0 completely fine |
I get that, but it might be related to the change in the HttpClient implementation (client-side) instead of the server-side. I'm assuming you are using the default OkHttp client implementation. Is it possible to try with the JDK implementation instead for example? |
It's at the debug level. The exception is just to show the full stacktrace. This was added as part of tracking down some inconsistent test runs, and was also intended for users who have several times opened issues / discussions about why they are getting rejected execution exceptions - the answer always is that the client has been closed, you just need to find out where. |
Line 248 in fbe8cd1
OT: This doesn't look right. Why are we instantiating an Exception and passing it in as an argument? |
See the previous comment.
An empty mock test won't engage with the server at all.
If I repeat your test from the command line, I don't see nearly as fast of execution time end-to-end as what you are showing. How are you instrumenting your tests, and what time are you reporting - for the whole test class, a single method, etc. Also are you always running your tests with debug engaged? |
I missed the part about
Added that for completeness, those changes should not affect performance at all. |
+1.
In my opinion, this debug message is quite confusing and makes users believe that there is something wrong with tests. This is especially confusing when there is actually something wrong with tests because exception stacktrace makes you think that problem is somehow related to the mock server. |
For 6.5 the message has changed Line 250 in f344495
|
I've already upgraded to 6.5.0.
yes, I know it. I'm not really convinced that this stacktrace is helpful, maybe it would be enough to just print out a short log message instead? |
We routinely have received issues/discussions from users who have inadvertently closed their client and receive an obtuse error from okhttp. This message is to make where that is happening clear with debug enabled. If you want to exclude it from you debug logs, or use a different http client, you won't see this message. If you feel even more strongly about it, you can submit a patch for what behavior you would like to see. |
The original issue was addressed under #4924 |
Describe the bug
After upgrading to 6.3.0 or 6.4.0 from 6.2.0 we found a regression in the KubernetesMockServer.
After every test run we keep getting the following exception:
Furthermore the tests also run much longer.
OkHttp version: 4.10.0 (both before and after the fabric8 upgrade)
The regression occurs even if we don't have any logic in the tests.
Fabric8 Kubernetes Client version
6.4.0
Steps to reproduce
Expected behavior
With 6.2.0 the empty test simply runs without any errors and much faster:
6.2.0: 13ms
6.4.0: 18ms
Maybe the time diff here is not significant but on longer tests it is much worse (6s vs 38s)
Runtime
other (please specify in additional context)
Kubernetes API Server version
other (please specify in additional context)
Environment
macOS
Fabric8 Kubernetes Client Logs
Additional context
No response
The text was updated successfully, but these errors were encountered: