terrymanu commented on a change in pull request #1345: URL: https://github.com/apache/shardingsphere-elasticjob/pull/1345#discussion_r467684438
########## File path: elasticjob-executor/elasticjob-executor-type/elasticjob-http-executor/src/main/java/org/apache/shardingsphere/elasticjob/http/pojo/HttpParam.java ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shardingsphere.elasticjob.http.pojo; + +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.Getter; + +@Getter +@AllArgsConstructor(access = AccessLevel.PUBLIC) Review comment: 1. @Getter should behind of @AllArgsConstructor 2. It is unnecessary use access = AccessLevel.PUBLIC because it is default value 3. Use @RequiredArgsConstructor is better if all fields are final ########## File path: elasticjob-executor/elasticjob-executor-type/elasticjob-http-executor/src/main/java/org/apache/shardingsphere/elasticjob/http/executor/HttpJobExecutor.java ########## @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shardingsphere.elasticjob.http.executor; + +import com.google.common.base.Strings; +import lombok.extern.slf4j.Slf4j; +import org.apache.shardingsphere.elasticjob.api.ElasticJob; +import org.apache.shardingsphere.elasticjob.api.JobConfiguration; +import org.apache.shardingsphere.elasticjob.api.ShardingContext; +import org.apache.shardingsphere.elasticjob.executor.JobFacade; +import org.apache.shardingsphere.elasticjob.executor.item.impl.TypedJobItemExecutor; +import org.apache.shardingsphere.elasticjob.http.pojo.HttpParam; +import org.apache.shardingsphere.elasticjob.http.props.HttpJobProperties; +import org.apache.shardingsphere.elasticjob.infra.exception.JobConfigurationException; +import org.apache.shardingsphere.elasticjob.infra.exception.JobExecutionException; +import org.apache.shardingsphere.elasticjob.infra.json.GsonFactory; + +import java.io.BufferedReader; +import java.io.DataOutputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import java.net.HttpURLConnection; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Properties; + +/** + * Http job executor. + */ +@Slf4j +public final class HttpJobExecutor implements TypedJobItemExecutor { + + @Override + public void process(final ElasticJob elasticJob, final JobConfiguration jobConfig, final JobFacade jobFacade, final ShardingContext shardingContext) { + HttpParam httpParam = getHttpParam(jobConfig.getProps()); + HttpURLConnection connection = null; + BufferedReader bufferedReader = null; + try { Review comment: Can we use try with resource to handle close? ########## File path: elasticjob-executor/elasticjob-executor-type/elasticjob-http-executor/src/main/java/org/apache/shardingsphere/elasticjob/http/executor/HttpJobExecutor.java ########## @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shardingsphere.elasticjob.http.executor; + +import com.google.common.base.Strings; +import lombok.extern.slf4j.Slf4j; +import org.apache.shardingsphere.elasticjob.api.ElasticJob; +import org.apache.shardingsphere.elasticjob.api.JobConfiguration; +import org.apache.shardingsphere.elasticjob.api.ShardingContext; +import org.apache.shardingsphere.elasticjob.executor.JobFacade; +import org.apache.shardingsphere.elasticjob.executor.item.impl.TypedJobItemExecutor; +import org.apache.shardingsphere.elasticjob.http.pojo.HttpParam; +import org.apache.shardingsphere.elasticjob.http.props.HttpJobProperties; +import org.apache.shardingsphere.elasticjob.infra.exception.JobConfigurationException; +import org.apache.shardingsphere.elasticjob.infra.exception.JobExecutionException; +import org.apache.shardingsphere.elasticjob.infra.json.GsonFactory; + +import java.io.BufferedReader; +import java.io.DataOutputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import java.net.HttpURLConnection; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Properties; + +/** + * Http job executor. + */ +@Slf4j +public final class HttpJobExecutor implements TypedJobItemExecutor { + + @Override + public void process(final ElasticJob elasticJob, final JobConfiguration jobConfig, final JobFacade jobFacade, final ShardingContext shardingContext) { + HttpParam httpParam = getHttpParam(jobConfig.getProps()); + HttpURLConnection connection = null; + BufferedReader bufferedReader = null; + try { + URL realUrl = new URL(httpParam.getUrl()); + connection = (HttpURLConnection) realUrl.openConnection(); + connection.setRequestMethod(httpParam.getMethod()); + connection.setDoOutput(httpParam.isPostMethod()); + connection.setConnectTimeout(httpParam.getConnectTimeout()); + connection.setReadTimeout(httpParam.getReadTimeout()); + if (!Strings.isNullOrEmpty(httpParam.getContentType())) { + connection.setRequestProperty("Content-Type", httpParam.getContentType()); + } + connection.connect(); + String data = httpParam.getData(); + if (httpParam.isPostMethod() && !Strings.isNullOrEmpty(data)) { + DataOutputStream dataOutputStream = new DataOutputStream(connection.getOutputStream()); + if (httpParam.isEnableTransparentShardingContext()) { + StringBuilder builder = new StringBuilder(data); + builder.append("&").append(HttpJobProperties.TRANSPARENT_SHARDING_CONTEXT_KEY); + builder.append("=").append(GsonFactory.getGson().toJson(shardingContext)); + data = builder.toString(); + } + dataOutputStream.write(data.getBytes(StandardCharsets.UTF_8)); + dataOutputStream.flush(); + dataOutputStream.close(); + } + int code = connection.getResponseCode(); + if (code != 200) { + throw new JobExecutionException("Http job %s executed with response code %d", jobConfig.getJobName(), code); + } + bufferedReader = new BufferedReader(new InputStreamReader(connection.getInputStream(), StandardCharsets.UTF_8)); + StringBuilder result = new StringBuilder(); + String line; + while ((line = bufferedReader.readLine()) != null) { + result.append(line); + } + log.debug("http job execute result : {}", result.toString()); + } catch (IOException ex) { Review comment: catch (IOException ex) should be catch (`final` IOException ex) ########## File path: elasticjob-executor/elasticjob-executor-type/elasticjob-http-executor/pom.xml ########## @@ -0,0 +1,48 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ Licensed to the Apache Software Foundation (ASF) under one or more + ~ contributor license agreements. See the NOTICE file distributed with + ~ this work for additional information regarding copyright ownership. + ~ The ASF licenses this file to You under the Apache License, Version 2.0 + ~ (the "License"); you may not use this file except in compliance with + ~ the License. You may obtain a copy of the License at + ~ + ~ http://www.apache.org/licenses/LICENSE-2.0 + ~ + ~ Unless required by applicable law or agreed to in writing, software + ~ distributed under the License is distributed on an "AS IS" BASIS, + ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + ~ See the License for the specific language governing permissions and + ~ limitations under the License. + --> + +<project xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.shardingsphere.elasticjob</groupId> + <artifactId>elasticjob-executor-type</artifactId> + <version>3.0.0-beta-SNAPSHOT</version> + </parent> + <artifactId>elasticjob-http-executor</artifactId> + <name>${project.artifactId}</name> + + <dependencies> + <dependency> + <groupId>org.apache.shardingsphere.elasticjob</groupId> + <artifactId>elasticjob-executor-kernel</artifactId> + <version>${project.parent.version}</version> + </dependency> + + <dependency> + <groupId>org.projectlombok</groupId> + <artifactId>lombok</artifactId> + </dependency> + + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + </dependency> + </dependencies> +</project> Review comment: Please add a new line in the end of file ########## File path: elasticjob-executor/elasticjob-executor-type/elasticjob-http-executor/src/main/java/org/apache/shardingsphere/elasticjob/http/executor/HttpJobExecutor.java ########## @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shardingsphere.elasticjob.http.executor; + +import com.google.common.base.Strings; +import lombok.extern.slf4j.Slf4j; +import org.apache.shardingsphere.elasticjob.api.ElasticJob; +import org.apache.shardingsphere.elasticjob.api.JobConfiguration; +import org.apache.shardingsphere.elasticjob.api.ShardingContext; +import org.apache.shardingsphere.elasticjob.executor.JobFacade; +import org.apache.shardingsphere.elasticjob.executor.item.impl.TypedJobItemExecutor; +import org.apache.shardingsphere.elasticjob.http.pojo.HttpParam; +import org.apache.shardingsphere.elasticjob.http.props.HttpJobProperties; +import org.apache.shardingsphere.elasticjob.infra.exception.JobConfigurationException; +import org.apache.shardingsphere.elasticjob.infra.exception.JobExecutionException; +import org.apache.shardingsphere.elasticjob.infra.json.GsonFactory; + +import java.io.BufferedReader; +import java.io.DataOutputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import java.net.HttpURLConnection; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Properties; + +/** + * Http job executor. + */ +@Slf4j +public final class HttpJobExecutor implements TypedJobItemExecutor { + + @Override + public void process(final ElasticJob elasticJob, final JobConfiguration jobConfig, final JobFacade jobFacade, final ShardingContext shardingContext) { + HttpParam httpParam = getHttpParam(jobConfig.getProps()); + HttpURLConnection connection = null; + BufferedReader bufferedReader = null; + try { + URL realUrl = new URL(httpParam.getUrl()); + connection = (HttpURLConnection) realUrl.openConnection(); + connection.setRequestMethod(httpParam.getMethod()); + connection.setDoOutput(httpParam.isPostMethod()); + connection.setConnectTimeout(httpParam.getConnectTimeout()); + connection.setReadTimeout(httpParam.getReadTimeout()); + if (!Strings.isNullOrEmpty(httpParam.getContentType())) { + connection.setRequestProperty("Content-Type", httpParam.getContentType()); + } + connection.connect(); + String data = httpParam.getData(); + if (httpParam.isPostMethod() && !Strings.isNullOrEmpty(data)) { + DataOutputStream dataOutputStream = new DataOutputStream(connection.getOutputStream()); + if (httpParam.isEnableTransparentShardingContext()) { + StringBuilder builder = new StringBuilder(data); + builder.append("&").append(HttpJobProperties.TRANSPARENT_SHARDING_CONTEXT_KEY); + builder.append("=").append(GsonFactory.getGson().toJson(shardingContext)); + data = builder.toString(); + } + dataOutputStream.write(data.getBytes(StandardCharsets.UTF_8)); + dataOutputStream.flush(); + dataOutputStream.close(); + } + int code = connection.getResponseCode(); + if (code != 200) { + throw new JobExecutionException("Http job %s executed with response code %d", jobConfig.getJobName(), code); + } + bufferedReader = new BufferedReader(new InputStreamReader(connection.getInputStream(), StandardCharsets.UTF_8)); + StringBuilder result = new StringBuilder(); + String line; + while ((line = bufferedReader.readLine()) != null) { + result.append(line); + } + log.debug("http job execute result : {}", result.toString()); + } catch (IOException ex) { + throw new JobExecutionException(ex); + } finally { + try { + if (bufferedReader != null) { Review comment: `bufferedReader != null` should be `null != bufferedReader` ########## File path: elasticjob-executor/elasticjob-executor-type/elasticjob-http-executor/src/main/java/org/apache/shardingsphere/elasticjob/http/pojo/HttpParam.java ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shardingsphere.elasticjob.http.pojo; + +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.Getter; + +@Getter +@AllArgsConstructor(access = AccessLevel.PUBLIC) +public final class HttpParam { Review comment: Missing java doc ########## File path: elasticjob-executor/elasticjob-executor-type/elasticjob-http-executor/src/main/java/org/apache/shardingsphere/elasticjob/http/pojo/HttpParam.java ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shardingsphere.elasticjob.http.pojo; + +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.Getter; + +@Getter +@AllArgsConstructor(access = AccessLevel.PUBLIC) +public final class HttpParam { + + private final String url; + + private final String method; + + private final boolean isPostMethod; + + private final String data; + + private final int connectTimeout; + + private final int readTimeout; + + private final String contentType; + + private final boolean enableTransparentShardingContext; + Review comment: Please remove useless blank line ---------------------------------------------------------------- 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: [email protected]
