[GitHub] [incubator-pinot] jihaozh commented on a change in pull request #5713: [TE] Setup a rest client pipeline for comms between ThirdEye services; added RCA highlights API as an example

[GitBox <gi...@apache.org>]

jihaozh commented on a change in pull request #5713:
URL: https://github.com/apache/incubator-pinot/pull/5713#discussion_r459140310



##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/common/restclient/AbstractRestClient.java
##########
@@ -0,0 +1,269 @@
+/*
+ * 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.pinot.thirdeye.common.restclient;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.io.UnsupportedEncodingException;
+import java.net.HttpURLConnection;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLEncoder;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.SortedMap;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * A generic API Rest Client to perform GET and POST
+ */
+public abstract class AbstractRestClient {

Review comment:
       Have we evaluated using the Jersey Client API library as the REST client? https://howtodoinjava.com/jersey/jersey-restful-client-examples/
   
   We are already using it Jersy to write the service. It seems natural to use it on the client-side as well. It can help us abstract away the low-level details of the client-server communication and avoid potential bugs. 

##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/common/restclient/AbstractRestClient.java
##########
@@ -0,0 +1,269 @@
+/*
+ * 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.pinot.thirdeye.common.restclient;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.io.UnsupportedEncodingException;
+import java.net.HttpURLConnection;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLEncoder;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.SortedMap;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * A generic API Rest Client to perform GET and POST
+ */
+public abstract class AbstractRestClient {
+  private final Logger LOG = LoggerFactory.getLogger(this.getClass());
+
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+  private final HttpURLConnectionFactory connectionFactory;
+
+  /**
+   * Using a URLFactory to create URLs allows unit tests to mock out server communication.
+   */
+  public class HttpURLConnectionFactory {
+    public HttpURLConnection openConnection(String url) throws MalformedURLException, IOException {
+      return (HttpURLConnection) new URL(url).openConnection();
+    }
+  }
+
+  /**
+   * Set up the client with a default URLFactory that creates real HTTP connections.
+   * For unit tests, we use the alternate constructor to pass a mock.
+   */
+  public AbstractRestClient() {
+    connectionFactory = new HttpURLConnectionFactory();
+  }
+
+  /**
+   * For testing only, create a client with an alternate URLFactory. This constructor allows
+   * unit tests to mock server communication.
+   */
+  public AbstractRestClient(HttpURLConnectionFactory connectionFactory) {
+    this.connectionFactory = connectionFactory;
+  }
+
+  /**
+   * Perform a GET request to the given URL, accepts a method that will parse the response as a parameter.
+   * A timeout of zero is interpreted as an infinite timeout.
+   * @param <T>  the type parameter defined as the return type of the response parser method
+   * @param url the http url
+   * @param host the host to connect to
+   * @param headers the headers for communication
+   */
+  public <T> T doGet(String url, String host, Map<String, String> headers,
+      ParseResponseFunction<InputStream, T> responseParserFunc) throws IOException {
+    return doGet(url, host, headers, 0, 0, responseParserFunc);
+  }
+
+  /**
+   * Perform a GET request to the given URL, accepts a method that will parse the response as a parameter.
+   * @param <T>  the type parameter defined as the return type of the response parser method
+   * @param url the http url
+   * @param host the host to connect to
+   * @param connectTimeout timeout in milliseconds
+   * @param readTimeout timeout in milliseconds
+   */
+  public <T> T doGet(String url, String host, Map<String, String> reqHeaders, int connectTimeout, int readTimeout,
+      ParseResponseFunction<InputStream, T> responseParserFunc) throws IOException {
+    Map<String, String> headers = new HashMap<>(reqHeaders);
+    headers.put("User-Agent", getClass().getName());
+    headers.put("Host", host);
+    headers.put("Accept", "application/json");
+
+    return doRequest(url, "GET", new byte[0], null, connectTimeout, readTimeout, headers,
+        responseParserFunc);
+  }
+
+  /**
+   * Perform a POST request to the given URL, with a JSON or raw string as content.
+   * If the payload is an object that will be serialized to JSON the isContentTypeJSON must be set to true
+   * @param <T>  the type parameter defined as the return type of the response parser method
+   * @param url the http url
+   * @param host the host to connect to
+   * @param postContent the post content
+   * @param isContentTypeJson flag indicating if the content is json type or not
+   */
+  public <T> T doPost(String url, String host, Map<String, String> reqHeaders, Object postContent,
+      boolean isContentTypeJson, ParseResponseFunction<InputStream, T> responseParserFunc) throws IOException {
+    ByteArrayOutputStream baos = new ByteArrayOutputStream();
+    OBJECT_MAPPER.writeValue(baos, postContent);
+    byte[] content = baos.toByteArray();
+    String contentType = isContentTypeJson ? "application/json" : null;
+
+    Map<String, String> headers = new HashMap<>(reqHeaders);
+    headers.put("User-Agent", getClass().getName());
+    headers.put("Host", host);
+    headers.put("Accept", "application/json");
+
+    return doRequest(url, "POST", content, contentType, 0, 0, headers,
+        responseParserFunc);
+  }
+
+  /**
+   * Send a request to the given URL with the given parameters.
+   * @param <T> the type parameter defined as the return type of the response parser method
+   * @param url server url.
+   * @param method HTTP method.
+   * @param content request content.
+   * @param contentType request content type.
+   * @param connectTimeoutMillis connection timeout.
+   * @param readTimeoutMillis read timeout.
+   * @param headers any additional request headers.
+   * @return T the result of the parser functions applied to the response content.
+   */
+  private <T> T doRequest(String url, String method, byte[] content, String contentType, int connectTimeoutMillis,
+      int readTimeoutMillis, Map<String, String> headers, ParseResponseFunction<InputStream, T> responseParserFunc)
+      throws IOException {
+    long startedMillis = System.currentTimeMillis();
+
+    HttpURLConnection conn = this.connectionFactory.openConnection(url);

Review comment:
       shall the `connection` be put into a try-with-resources block? so that it can be closed whenever there is any exception.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org