You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/02/01 18:16:49 UTC
[GitHub] [camel] perttuk opened a new pull request #4990: Slack: Re-use http client and consume response
perttuk opened a new pull request #4990:
URL: https://github.com/apache/camel/pull/4990
Re-use http client and consume response.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on a change in pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #4990:
URL: https://github.com/apache/camel/pull/4990#discussion_r568327919
##########
File path: components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackProducer.java
##########
@@ -32,21 +32,23 @@
import org.apache.http.client.methods.HttpPost;
import org.apache.http.entity.StringEntity;
import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.util.EntityUtils;
public class SlackProducer extends DefaultProducer {
- private SlackEndpoint slackEndpoint;
+ private final SlackEndpoint slackEndpoint;
+ private final HttpClient client;
public SlackProducer(SlackEndpoint endpoint) {
super(endpoint);
this.slackEndpoint = endpoint;
+ this.client = HttpClientBuilder.create().useSystemProperties().build();
Review comment:
Same as consumer
##########
File path: components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackConsumer.java
##########
@@ -48,10 +48,12 @@
private SlackEndpoint slackEndpoint;
private String timestamp;
private String channelId;
+ private HttpClient client;
public SlackConsumer(SlackEndpoint endpoint, Processor processor) throws IOException, DeserializationException {
super(endpoint, processor);
this.slackEndpoint = endpoint;
+ this.client = HttpClientBuilder.create().useSystemProperties().build();
Review comment:
Do not create the client in the constructor, but in doStart
##########
File path: components/camel-slack/pom.xml
##########
@@ -62,6 +62,10 @@
<groupId>org.apache.httpcomponents</groupId>
Review comment:
Can the old client be removed?
##########
File path: components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackProducer.java
##########
@@ -59,7 +61,7 @@ protected void doStop() throws Exception {
}
Review comment:
And you have a doStop method where you stop/close the client?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] oscerd commented on pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
oscerd commented on pull request #4990:
URL: https://github.com/apache/camel/pull/4990#issuecomment-771718814
I was just curious about the idea. It doesn't harm, so no need for reverting
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on a change in pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #4990:
URL: https://github.com/apache/camel/pull/4990#discussion_r568708338
##########
File path: components/camel-slack/pom.xml
##########
@@ -62,6 +62,10 @@
<groupId>org.apache.httpcomponents</groupId>
Review comment:
Can the old client be removed?
##########
File path: components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackProducer.java
##########
@@ -59,7 +61,7 @@ protected void doStop() throws Exception {
}
Review comment:
And you have a doStop method where you stop/close the client?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] oscerd commented on pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
oscerd commented on pull request #4990:
URL: https://github.com/apache/camel/pull/4990#issuecomment-771718814
I was just curious about the idea. It doesn't harm, so no need for reverting
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on a change in pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #4990:
URL: https://github.com/apache/camel/pull/4990#discussion_r568327919
##########
File path: components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackProducer.java
##########
@@ -32,21 +32,23 @@
import org.apache.http.client.methods.HttpPost;
import org.apache.http.entity.StringEntity;
import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.util.EntityUtils;
public class SlackProducer extends DefaultProducer {
- private SlackEndpoint slackEndpoint;
+ private final SlackEndpoint slackEndpoint;
+ private final HttpClient client;
public SlackProducer(SlackEndpoint endpoint) {
super(endpoint);
this.slackEndpoint = endpoint;
+ this.client = HttpClientBuilder.create().useSystemProperties().build();
Review comment:
Same as consumer
##########
File path: components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackConsumer.java
##########
@@ -48,10 +48,12 @@
private SlackEndpoint slackEndpoint;
private String timestamp;
private String channelId;
+ private HttpClient client;
public SlackConsumer(SlackEndpoint endpoint, Processor processor) throws IOException, DeserializationException {
super(endpoint, processor);
this.slackEndpoint = endpoint;
+ this.client = HttpClientBuilder.create().useSystemProperties().build();
Review comment:
Do not create the client in the constructor, but in doStart
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] perttuk commented on a change in pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
perttuk commented on a change in pull request #4990:
URL: https://github.com/apache/camel/pull/4990#discussion_r568711078
##########
File path: components/camel-slack/pom.xml
##########
@@ -62,6 +62,10 @@
<groupId>org.apache.httpcomponents</groupId>
Review comment:
It is still used by consumer
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] perttuk commented on a change in pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
perttuk commented on a change in pull request #4990:
URL: https://github.com/apache/camel/pull/4990#discussion_r568711078
##########
File path: components/camel-slack/pom.xml
##########
@@ -62,6 +62,10 @@
<groupId>org.apache.httpcomponents</groupId>
Review comment:
It is still used by consumer
##########
File path: components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackProducer.java
##########
@@ -59,7 +61,7 @@ protected void doStop() throws Exception {
}
Review comment:
I added that in previous commit (https://github.com/apache/camel/pull/4990/commits/424273684ce44708461b0188cc90832517aae26b)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus merged pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
davsclaus merged pull request #4990:
URL: https://github.com/apache/camel/pull/4990
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] perttuk commented on pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
perttuk commented on pull request #4990:
URL: https://github.com/apache/camel/pull/4990#issuecomment-771714456
>
>
> Is there any particular reason to switch to async? I don't thibk there is a gain in this scenario
No, not really. I was thinking it would be minor improvement :-) Should I revert the change?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] perttuk commented on pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
perttuk commented on pull request #4990:
URL: https://github.com/apache/camel/pull/4990#issuecomment-771714456
>
>
> Is there any particular reason to switch to async? I don't thibk there is a gain in this scenario
No, not really. I was thinking it would be minor improvement :-) Should I revert the change?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] perttuk commented on a change in pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
perttuk commented on a change in pull request #4990:
URL: https://github.com/apache/camel/pull/4990#discussion_r568720876
##########
File path: components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackProducer.java
##########
@@ -59,7 +61,7 @@ protected void doStop() throws Exception {
}
Review comment:
I added that in previous commit (https://github.com/apache/camel/pull/4990/commits/424273684ce44708461b0188cc90832517aae26b)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus merged pull request #4990: Slack: Re-use http client and consume response
Posted by GitBox <gi...@apache.org>.
davsclaus merged pull request #4990:
URL: https://github.com/apache/camel/pull/4990
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org