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.

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

INT-4208: Support for AsycRestTemplate and thus noblocking IO #2043

Closed
wants to merge 4 commits into from

Conversation

pkuashi
Copy link
Contributor

@pkuashi pkuashi commented Jan 31, 2017

https://jira.spring.io/browse/INT-4208

Add AsyncRestTemplate support in HttpRequestExecutingMessageHandler and corresponding config/dsl support

https://jira.spring.io/browse/INT-4208

Add AsyncRestTemplate support in HttpRequestExecutingMessageHandler and corresponding config/dsl support
static void verifyNoAsyncRestTemplateAttributes(Element element, ParserContext parserContext) {
for (String attributeName : ASYNC_REST_TEMPLATE_REFERENCE_ATTRIBUTES) {
if (element.hasAttribute(attributeName)) {
parserContext.getReaderContext().error("When providing a 'rest-template' reference, the '"
Copy link
Member

Choose a reason for hiding this comment

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

"When providing an 'async-rest-template'...

Correct? To properly reflect the reality.

@@ -33,28 +34,51 @@
* @author Oleg Zhurakousky
* @author Gary Russell
* @author Artem Bilan
* @author Shiliang Li
Copy link
Member

Choose a reason for hiding this comment

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

Missed Copyright update in this class.

import org.springframework.web.client.AsyncRestTemplate;

/**
* The {@link org.springframework.integration.dsl.MessageHandlerSpec} implementation for the {@link AsyncHttpRequestExecutingMessageHandler}.
Copy link
Member

Choose a reason for hiding this comment

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

This is BaseHttpMessageHandlerSpec impl.
No reason to mention the super of super.

AsyncHttpMessageHandlerSpec(Expression uriExpression, AsyncRestTemplate asyncRestTemplate) {
this.target = new AsyncHttpRequestExecutingMessageHandler(uriExpression, asyncRestTemplate);
this.target.setUriVariableExpressions(this.uriVariableExpressions);
this.target.setHeaderMapper(this.headerMapper);
Copy link
Member

Choose a reason for hiding this comment

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

M-m-m. I think we have to move these to assignments to the ctor of super class.
And in that case we won't need to make those variables as protected to make Sonar Qube angry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean moving the assignments to the ctor of BaseHttpMessageHandlerSpec? I have tried that but faced one problem: I still want to leave the this.target = new AsyncHttpRequestExecutingMessageHandler(uriExpression, asyncRestTemplate); to the concrete subclass to avoid reflection solution then after moving this.target.setUriVariableExpressions(this.uriVariableExpressions); this.target.setHeaderMapper(this.headerMapper); statements to BaseHttpMessageHandlerSpec will throw error due to the this.target is not initialized yet.

What you think?

Copy link
Member

Choose a reason for hiding this comment

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

???
How about this?

protected BaseHttpMessageHandlerSpec(E handler) {
	this.target = handler;
	this.target.setUriVariableExpressions(this.uriVariableExpressions);
	this.target.setHeaderMapper(this.headerMapper);
}
...
AsyncHttpMessageHandlerSpec(Expression uriExpression, AsyncRestTemplate asyncRestTemplate) {
	super(new AsyncHttpRequestExecutingMessageHandler(uriExpression, asyncRestTemplate));
	this.asyncRestTemplate = asyncRestTemplate;
}
...
HttpMessageHandlerSpec(Expression uriExpression, RestTemplate restTemplate) {
	super(new HttpRequestExecutingMessageHandler(uriExpression, restTemplate));
	this.restTemplate = restTemplate;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

return Collections.singletonList(this.headerMapper);
}

protected abstract boolean isRestTemplateSet();
Copy link
Member

Choose a reason for hiding this comment

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

abstract in the end, after all other public` full methods.

* @since 5.0
*/
public class AsyncHttpRequestExecutingMessageHandler extends AbstractHttpRequestExecutingMessageHandler {
private AsyncRestTemplate asyncRestTemplate;
Copy link
Member

Choose a reason for hiding this comment

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

Must be final and surrounded with blank lines.

</bean>
----

You can configure the AsyncClientHttpRequestFactory instance to use:
Copy link
Member

Choose a reason for hiding this comment

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

Consider to wrap all the classes mentioned to the code using "`" symbol.

@artembilan
Copy link
Member

Please, also mention in the commit message https://jira.spring.io/browse/INT-4076

https://jira.spring.io/browse/INT-4076
https://jira.spring.io/browse/INT-4208

Add AsyncRestTemplate support in HttpRequestExecutingMessageHandler and corresponding config/dsl support
@pkuashi
Copy link
Contributor Author

pkuashi commented Feb 1, 2017

Will clear the errors tomorrow

https://jira.spring.io/browse/INT-4076
https://jira.spring.io/browse/INT-4208

Add AsyncRestTemplate support in HttpRequestExecutingMessageHandler and corresponding config/dsl support
https://jira.spring.io/browse/INT-4076
https://jira.spring.io/browse/INT-4208

Add AsyncRestTemplate support in HttpRequestExecutingMessageHandler and corresponding config/dsl support
@artembilan
Copy link
Member

Merged as 8020619

Awesome work, @pkuashi !
Thank you very much for the contribution.

Looking forward for more!

@artembilan artembilan closed this Feb 3, 2017
@pkuashi
Copy link
Contributor Author

pkuashi commented Feb 4, 2017

@artembilan Thanks for hosting my first PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants