-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: WebSocketOutputStream wait to write until WebSocket queue is released #2388
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
fix: WebSocketOutputStream wait to write until WebSocket queue is released #2388
Conversation
…eased Signed-off-by: Daniel Arteaga Barba <darteaga@vmware.com>
Signed-off-by: Daniel Arteaga Barba <darteaga@vmware.com>
| while (WebSocketStreamHandler.this.socket.queueSize() + byteString.size() > MAX_QUEUE_SIZE | ||
| && attempts < MAX_QUEUE_SIZE_MAX_ATTEMPTS) { | ||
| try { | ||
| Thread.sleep(MAX_QUEUE_SIZE_WAIT_MILLISECONDS); |
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.
Rather than just straight sleeping, what do you think about using wait(MAX_TIME)/notify here. It would reduce latency and also be more efficient.
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.
Great suggestion! it is actually way long better
Using wait (~3min)
# exec 1
2022-09-27T14:12:16.502+0200 [ main][DEBUG] Copying /var/folders/v_/9khlk6_94hg0q1prl51fxbkh0000gp/T/12395450492776725845 to default:tests-c58fcf48c-n5w8f:/tmp
2022-09-27T14:15:44.043+0200 [ main][DEBUG] Copied /var/folders/v_/9khlk6_94hg0q1prl51fxbkh0000gp/T/12395450492776725845
# exec 2
2022-09-27T14:27:31.007+0200 [ main][DEBUG] Copying /var/folders/v_/9khlk6_94hg0q1prl51fxbkh0000gp/T/8625417876961582076 to default:tests-c58fcf48c-n5w8f:/tmp
2022-09-27T14:30:51.087+0200 [ main][DEBUG] Copied /var/folders/v_/9khlk6_94hg0q1prl51fxbkh0000gp/T/8625417876961582076
Using Thread.sleep (~4min)
# exec 1
2022-09-27T14:38:52.833+0200 [ main][DEBUG] Copying /var/folders/v_/9khlk6_94hg0q1prl51fxbkh0000gp/T/422416366883411235 to default:tests-c58fcf48c-n5w8f:/tmp
2022-09-27T14:42:23.119+0200 [ main][DEBUG] Copied /var/folders/v_/9khlk6_94hg0q1prl51fxbkh0000gp/T/422416366883411235
# exec 2
2022-09-27T15:24:35.732+0200 [ main][DEBUG] Copying /var/folders/v_/9khlk6_94hg0q1prl51fxbkh0000gp/T/2455455714318474992 to default:tests-c58fcf48c-n5w8f:/tmp
2022-09-27T15:28:35.794+0200 [ main][DEBUG] Copied /var/folders/v_/9khlk6_94hg0q1prl51fxbkh0000gp/T/tests-workplace-2455455714318474992
util/src/main/java/io/kubernetes/client/util/WebSocketStreamHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Daniel Arteaga Barba <darteaga@vmware.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, dani8art The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…dding-data-to-queue fix: WebSocketOutputStream wait to write until WebSocket queue is released
Signed-off-by: Daniel Arteaga Barba darteaga@vmware.com
This PR is a proposal for the following issue:#2309 based in this comment: #2309 (comment)