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

Issue #5018 - add request timeout onto websocket ClientUpgradeRequest #5024

Merged
merged 4 commits into from
Jul 8, 2020
Merged

Issue #5018 - add request timeout onto websocket ClientUpgradeRequest #5024

merged 4 commits into from
Jul 8, 2020

Conversation

lachlan-roberts
Copy link
Contributor

Issue #5018

Add setter and getter for request timeout on the ClientUpgradeRequest in the same style as the methods onorg.eclipse.jetty.client.api.Request.

@lachlan-roberts lachlan-roberts mentioned this pull request Jul 4, 2020
Closed
@lachlan-roberts
fix checkstyle issue
fbe3803 
Signed-off-by: Lachlan Roberts <[email protected]>
sbordet
sbordet requested changes Jul 6, 2020
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

@lachlan-roberts please add a test case.

@lachlan-roberts lachlan-roberts requested a review from sbordet July 7, 2020 08:18
sbordet
sbordet approved these changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Please fix what detailed in the comments, but otherwise LGTM.

...ebsocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java Outdated Show resolved Hide resolved
...ebsocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java Outdated Show resolved Hide resolved
.../websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java Outdated Show resolved Hide resolved
@lachlan-roberts
Issue #5018 - rename ClientUpgradeRequest timeout to setTimeout
702a48c 
and increase timeouts used for testing

Signed-off-by: Lachlan Roberts <[email protected]>
@joakime
Copy link
Contributor

joakime commented Jul 7, 2020

Tests failed.

Parallel Stage / Build / Test - JDK11 / testAsyncRest – org.eclipse.jetty.tests.distribution.DemoBaseTests
org.opentest4j.AssertionFailedError: expected: <200> but was: <500>
	at org.eclipse.jetty.tests.distribution.DemoBaseTests.testAsyncRest(DemoBaseTests.java:106)

Probably as a result of the following ...

2020-07-07 12:36:31.177:INFO:oejtd.DistributionTester:ConsoleStreamer/STDERR: 2020-07-07 12:36:31.176:WARN:oejs.HttpChannel:qtp373182087-17: 
  handleException /async-rest/testSerial java.io.IOException: Server returned HTTP response code: 400 for URL: 
  http://open.api.ebay.com/shopping?MaxEntries=3&appid=Webtide81-adf4-4f0a-ad58-d91e41bbe85&version=573&siteid=0&callname=FindItems&responseencoding=JSON&QueryKeywords=gnome

@lachlan-roberts
Copy link
Contributor Author

@joakime the failure is unrelated to the changes in this PR, I re-ran and it passed the tests.
Going ahead with the merge.

@lachlan-roberts lachlan-roberts merged commit 794d67c into jetty-9.4.x Jul 8, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-9.4.x-5018-ClientUpgradeRequestTimeout branch July 8, 2020 06:36
@lachlan-roberts lachlan-roberts changed the title Issue #5018 - add request timeout onto ClientUpgradeRequest Issue #5018 - add request timeout onto websocket ClientUpgradeRequest Jul 8, 2020
@lachlan-roberts lachlan-roberts mentioned this pull request Jul 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@joakime joakime Awaiting requested review from joakime

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lachlan-roberts @joakime @sbordet