Add duration support for setConnectTimeout and setReadTimeout #13355

nosan · 2018-06-04T13:07:28Z

this PR provides new methods setConnectTimeout(Duration) and setReadTimeout(Duration) for RestTemplateBuilder .

nosan · 2018-06-04T14:04:28Z

	if (!CollectionUtils.isEmpty(this.restTemplateCustomizers)) {
			for (RestTemplateCustomizer customizer : this.restTemplateCustomizers) {
				customizer.customize(restTemplate);
			}
		}
		restTemplate.getInterceptors().addAll(this.interceptors);

Maybe Customizers should be moved in the end of the method in 2.1.0?

snicoll · 2018-06-04T14:07:10Z

@nosan please do not use this issue to discuss something totally unrelated. We're on gitter if you want to chat. Or you can create another issue.

nosan · 2018-06-04T15:31:23Z

@snicoll Got it, sorry about that.

snicoll

Thanks for the PR. I've made a few comments and welcome your thoughts on it.

I really prefer a dedicated PR to focus on the task at hand. We also welcome "polish" PR that improve code structure, fix typo, etc.

snicoll · 2018-06-04T15:39:03Z

...oject/spring-boot/src/main/java/org/springframework/boot/web/client/RestTemplateBuilder.java


 	private final String rootUri;

+	private final Duration readTimeout;


That's not what I have in mind, please keep your change to a minimum. I expect two new methods passing a Duration to the existing customizers with the deprecated methods delegating to them.

snicoll · 2018-06-04T15:40:23Z

...oject/spring-boot/src/main/java/org/springframework/boot/web/client/RestTemplateBuilder.java

 	 * {@link ClientHttpRequestFactory}.
 	 * @param connectTimeout the connect timeout in milliseconds
 	 * @return a new builder instance.
+	 * @deprecated Deprecated in favor of {@link #setConnectTimeout(Duration)}


The format we use is @deprecated since 2.1.0 in favour of {@link #setConnectTimeout(Duration)}.

snicoll · 2018-06-04T15:41:09Z

...oject/spring-boot/src/main/java/org/springframework/boot/web/client/RestTemplateBuilder.java

+	}
+
+	/**
+	 * Sets the connection timeout on the underlying {@link ClientHttpRequestFactory}.


I'd put the method taking Duration above the deprecated one, not below.

snicoll · 2018-06-04T15:51:23Z

...ain/java/org/springframework/boot/webservices/client/HttpWebServiceMessageSenderBuilder.java

 public class HttpWebServiceMessageSenderBuilder {

-	private Integer connectionTimeout;
+	private Duration connectTimeout;


Good catch. I've polished the wrong name in 93396ae please rebase and push force an update.

snicoll · 2018-06-04T15:54:05Z

.../spring-boot/src/test/java/org/springframework/boot/web/client/RestTemplateBuilderTests.java

+
+	@Test
+	@SuppressWarnings("deprecation")
+	public void nullReadTimeoutShouldNotAppliedIgnorePreviousSettings() {


Those two tests are new right? You could do the same with one that takes a Duration and one that takes null afteR. No need for the deprecation. Besides, I'd appreciate a separate PR for this as they are unrelated.

nosan · 2018-06-05T09:22:51Z

@snicoll Thank you.
Adding only setReadTimeout(Duration) and setConnectTimeout(Duration) without changing RestTemplateBuilder is not a correct way.
Currently, RestTemplateBuilder uses Set<RestTemplateCustomizer> restTemplateCustomizers> for configuring these timeouts, so every time when a user is trying to set a new timeout, RestTemplateBuilder will add a new Customizer in the Set<RestTemplateCustomizer> restTemplateCustomizers> .
It works perfectly for the current implementation because RestTemplateBuilder has only setReadTimeout(int) and setConnectTimeout(int) and a user is not able to set null timeout.
With the new methods user is able to do this and in that case, it means a user doesn't want to set a timeout on the underlying, thus I've provided new fields for reach this purpose.

P.S. With the current implementation this test doesn't work.

	ClientHttpRequestFactory requestFactory = this.builder
				.requestFactory(SimpleClientHttpRequestFactory.class)
				.setConnectTimeout(Duration.ofSeconds(5))
				.setConnectTimeout(null).build()
				.getRequestFactory();
		assertThat(ReflectionTestUtils.getField(requestFactory, "connectTimeout"))
				.isEqualTo(-1);

nosan · 2018-06-05T09:53:06Z

@snicoll I've updated PR.
I've created a separate PR for HttpWebServiceMessageSenderBuilder. gh-13364

`setConnectTimeout(Duration)`

See gh-13355

* pr/13355: Polish "Add duration support for setConnectTimeout and setReadTimeout" Add duration support for setConnectTimeout and setReadTimeout Polish "Add duration support for setConnectTimout and setReadTimeout"

snicoll · 2018-06-05T14:07:44Z

@nosan thanks for the PR. I found the changes in RestTemplateBuilder too invasive so I've pre-polished your PR, see 8691d01.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 4, 2018

snicoll requested changes Jun 4, 2018

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jun 4, 2018

nosan force-pushed the duration-instead-of-timeout branch from 6bf173d to f1e38fc Compare June 5, 2018 09:53

RestTemplateBuilder new methods setReadTimeout(Duration) and

a9b8404

`setConnectTimeout(Duration)`

nosan force-pushed the duration-instead-of-timeout branch from f1e38fc to a9b8404 Compare June 5, 2018 09:54

nosan changed the title ~~Duration instead of 'int'. RestTemplateBuilder HttpWebServiceMessageSenderBuilder~~ Duration instead of 'int'. RestTemplateBuilder Jun 5, 2018

snicoll changed the title ~~Duration instead of 'int'. RestTemplateBuilder~~ Add duration support for setConnectTimeout and setReadTimeout Jun 5, 2018

snicoll added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Jun 5, 2018

snicoll self-assigned this Jun 5, 2018

snicoll added this to the 2.1.0.M1 milestone Jun 5, 2018

snicoll added a commit that referenced this pull request Jun 5, 2018

Polish "Add duration support for setConnectTimout and setReadTimeout"

8691d01

See gh-13355

snicoll pushed a commit that referenced this pull request Jun 5, 2018

Add duration support for setConnectTimeout and setReadTimeout

36f3c1b

See gh-13355

snicoll closed this in e9c3df4 Jun 5, 2018

nosan deleted the duration-instead-of-timeout branch April 23, 2019 11:36


		private final String rootUri;

		private final Duration readTimeout;

Add duration support for setConnectTimeout and setReadTimeout #13355

Add duration support for setConnectTimeout and setReadTimeout #13355

Uh oh!

Conversation

nosan commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nosan commented Jun 4, 2018

Uh oh!

snicoll commented Jun 4, 2018

Uh oh!

nosan commented Jun 4, 2018

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

snicoll Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

snicoll Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

snicoll Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

snicoll Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

snicoll Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

nosan commented Jun 5, 2018

Uh oh!

nosan commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snicoll commented Jun 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nosan commented Jun 4, 2018 •

edited

Loading

nosan commented Jun 5, 2018 •

edited

Loading