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

[JENKINS-61409] Websockets: Use AbstractByteBufferCommandTransport to transport messages #373

Merged
merged 4 commits into from
Mar 25, 2020
Merged

[JENKINS-61409] Websockets: Use AbstractByteBufferCommandTransport to transport messages #373

merged 4 commits into from
Mar 25, 2020

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Mar 19, 2020 •
edited
Loading

See JENKINS-61409.

Fixes support of websocket messages more than 64kb long.

See downstream jenkinsci/jenkins#4596

@jglick jglick self-requested a review March 19, 2020 13:03
@Vlatombe Vlatombe changed the title Split message when over max size Use AbstractByteBufferCommandTransport to transport messages Mar 20, 2020
@Vlatombe Vlatombe changed the title Use AbstractByteBufferCommandTransport to transport messages Websockets: Use AbstractByteBufferCommandTransport to transport messages Mar 20, 2020
@Vlatombe
Copy link
Member Author

Incrementals is down 😢

[2020-03-20T10:14:17.616Z] + curl -i -H Content-Type: application/json -d {"build_url":"https://ci.jenkins.io/job/Core/job/remoting/job/PR-373/3/"} https://jenkins-community-functions.azurewebsites.net/api/incrementals-publisher?clientId=default&code=****
[2020-03-20T10:14:17.617Z]   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
[2020-03-20T10:14:17.617Z]                                  Dload  Upload   Total   Spent    Left  Speed
[2020-03-20T10:14:17.882Z] 
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100   102  100    29  100    73     78    196 --:--:-- --:--:-- --:--:--   274
[2020-03-20T10:14:17.882Z] HTTP/1.1 503 Service Unavailable
[2020-03-20T10:14:17.882Z] Content-Length: 29
[2020-03-20T10:14:17.882Z] Date: Fri, 20 Mar 2020 10:14:17 GMT
[2020-03-20T10:14:17.882Z] 
[2020-03-20T10:14:17.927Z] Function host is not running.

@Vlatombe Vlatombe closed this Mar 20, 2020
@Vlatombe Vlatombe reopened this Mar 20, 2020
@Vlatombe Vlatombe closed this Mar 20, 2020
@Vlatombe Vlatombe reopened this Mar 20, 2020
@Vlatombe Vlatombe changed the title Websockets: Use AbstractByteBufferCommandTransport to transport messages [JENKINS-61409] Websockets: Use AbstractByteBufferCommandTransport to transport messages Mar 20, 2020
@Vlatombe Vlatombe mentioned this pull request Mar 20, 2020
Merged
10 tasks
@Vlatombe Vlatombe marked this pull request as ready for review March 20, 2020 13:20
jeffret-b
jeffret-b approved these changes Mar 20, 2020
View reviewed changes
Copy link
Contributor

@jeffret-b jeffret-b 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 if it solves the problem. Have you tested it manually to make sure it does that?

@jeffret-b
Copy link
Contributor

Other than the changelog, do we need to do anything else to properly convey information about this breaking change? This feature is considered beta so I don't know that we need to rev the major version. (And we had a discussion not long ago about how useful that is anyway.)

@jeffret-b jeffret-b requested a review from jvz March 20, 2020 19:16
jvz
jvz approved these changes Mar 20, 2020
View reviewed changes
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

LGTM

@jglick
Copy link
Member

jglick commented Mar 20, 2020

do we need to do anything else to properly convey information about this breaking change?

At a minimum we will need to release a new Docker image, and update the kubernetes plugin to use that as a default.

@Vlatombe Vlatombe closed this Mar 23, 2020
@Vlatombe Vlatombe reopened this Mar 23, 2020
@jeffret-b
Copy link
Contributor

@Vlatombe , have you tested this in a running environment? It looks like it should work but I just want to check.

@Vlatombe
Copy link
Member Author

@jeffret-b yes

@jeffret-b
Copy link
Contributor

Great. I'll mark it ready for merge and try to get it released before too long.

@jeffret-b jeffret-b merged commit 082530f into jenkinsci:master Mar 25, 2020
@jglick jglick mentioned this pull request Mar 25, 2020
Merged
10 tasks
@jglick
Copy link
Member

jglick commented Mar 25, 2020

And do not forget

we will need to release a new Docker image

after which @Vlatombe or I can

update the kubernetes plugin to use that as a default

@jeffret-b
Copy link
Contributor

Coming up

@jeffret-b
Copy link
Contributor

First Docker PR at jenkinsci/docker-agent#110. After the first one builds, I'll submit the other one.

@Vlatombe Vlatombe deleted the JENKINS-61409-split branch March 26, 2020 08:56
@jeffret-b
Copy link
Contributor

Second Docker PR at jenkinsci/docker-inbound-agent#153.

jeffret-b pushed a commit that referenced this pull request Apr 8, 2020
@Vlatombe @jeffret-b
[JENKINS-61409] Websockets: Use AbstractByteBufferCommandTransport to…
b72909b 
… transport messages (#373)

* Split message when over max size

* Should be private

* [JENKINS-61409] Use AbstractByteBufferCommandTransport

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

@jeffret-b jeffret-b jeffret-b approved these changes

@jvz jvz jvz approved these changes

@jglick jglick jglick approved these changes

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Vlatombe @jeffret-b @jglick @jvz