[GitHub] [helix] dasahcc commented on a change in pull request #1163: Implement RoutingDataManager to replace HttpRoutingDataReader

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

dasahcc commented on a change in pull request #1163:
URL: https://github.com/apache/helix/pull/1163#discussion_r459643553



##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/routing/RoutingDataManager.java
##########
@@ -0,0 +1,179 @@
+package org.apache.helix.zookeeper.routing;
+
+/*
+ * 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.
+ */
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.helix.msdcommon.constant.MetadataStoreRoutingConstants;
+import org.apache.helix.msdcommon.datamodel.MetadataStoreRoutingData;
+import org.apache.helix.msdcommon.datamodel.TrieRoutingData;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.constant.RoutingDataReaderType;
+import org.apache.helix.zookeeper.exception.MultiZkException;
+
+
+/**
+ * RoutingDataManager is a static Singleton that
+ * 1. resolves RoutingDataReader based on the system config given
+ * 2. caches routing data
+ * 3. provides public methods for reading routing data from various sources (configurable)
+ */
+public class RoutingDataManager {
+  /** HTTP call to MSDS is used to fetch routing data by default */
+  private static final String DEFAULT_MSDS_ENDPOINT =
+      System.getProperty(MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY);
+
+  /** Double-checked locking requires that the following fields be final (volatile) */
+  // The following map stands for (RoutingDataReaderType_endpoint ID, Raw Routing Data)
+  private static final Map<String, Map<String, List<String>>> _rawRoutingDataMap =
+      new ConcurrentHashMap<>();
+  // The following map stands for (RoutingDataReaderType_endpoint ID, MetadataStoreRoutingData)
+  private static final Map<String, MetadataStoreRoutingData> _metadataStoreRoutingDataMap =
+      new ConcurrentHashMap<>();
+
+  /**
+   * This class is a Singleton.
+   */
+  private RoutingDataManager() {
+  }
+
+  /**
+   * Fetches routing data from the data source via HTTP by querying the MSDS configured in the JVM
+   * config.
+   * @return
+   */
+  public static Map<String, List<String>> getRawRoutingData() {
+    if (DEFAULT_MSDS_ENDPOINT == null || DEFAULT_MSDS_ENDPOINT.isEmpty()) {
+      throw new IllegalStateException(
+          "HttpRoutingDataReader was unable to find a valid MSDS endpoint String in System "
+              + "Properties!");
+    }
+    return getRawRoutingData(RoutingDataReaderType.HTTP, DEFAULT_MSDS_ENDPOINT);

Review comment:
       This should be configurable in your next PR, right?

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/routing/RoutingDataManager.java
##########
@@ -0,0 +1,179 @@
+package org.apache.helix.zookeeper.routing;
+
+/*
+ * 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.
+ */
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.helix.msdcommon.constant.MetadataStoreRoutingConstants;
+import org.apache.helix.msdcommon.datamodel.MetadataStoreRoutingData;
+import org.apache.helix.msdcommon.datamodel.TrieRoutingData;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.constant.RoutingDataReaderType;
+import org.apache.helix.zookeeper.exception.MultiZkException;
+
+
+/**
+ * RoutingDataManager is a static Singleton that
+ * 1. resolves RoutingDataReader based on the system config given
+ * 2. caches routing data
+ * 3. provides public methods for reading routing data from various sources (configurable)
+ */
+public class RoutingDataManager {
+  /** HTTP call to MSDS is used to fetch routing data by default */
+  private static final String DEFAULT_MSDS_ENDPOINT =
+      System.getProperty(MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY);
+
+  /** Double-checked locking requires that the following fields be final (volatile) */
+  // The following map stands for (RoutingDataReaderType_endpoint ID, Raw Routing Data)
+  private static final Map<String, Map<String, List<String>>> _rawRoutingDataMap =
+      new ConcurrentHashMap<>();
+  // The following map stands for (RoutingDataReaderType_endpoint ID, MetadataStoreRoutingData)
+  private static final Map<String, MetadataStoreRoutingData> _metadataStoreRoutingDataMap =
+      new ConcurrentHashMap<>();
+
+  /**
+   * This class is a Singleton.
+   */
+  private RoutingDataManager() {
+  }
+
+  /**
+   * Fetches routing data from the data source via HTTP by querying the MSDS configured in the JVM
+   * config.
+   * @return
+   */
+  public static Map<String, List<String>> getRawRoutingData() {
+    if (DEFAULT_MSDS_ENDPOINT == null || DEFAULT_MSDS_ENDPOINT.isEmpty()) {
+      throw new IllegalStateException(
+          "HttpRoutingDataReader was unable to find a valid MSDS endpoint String in System "
+              + "Properties!");
+    }
+    return getRawRoutingData(RoutingDataReaderType.HTTP, DEFAULT_MSDS_ENDPOINT);
+  }
+
+  /**
+   * Fetches routing data from the data source via HTTP.
+   * @return a mapping from "metadata store realm addresses" to lists of
+   * "metadata store sharding keys", where the sharding keys in a value list all route to
+   * the realm address in the key disallows a meaningful mapping to be returned.
+   * @param routingDataReaderType
+   * @param endpoint
+   */
+  public static Map<String, List<String>> getRawRoutingData(
+      RoutingDataReaderType routingDataReaderType, String endpoint) {
+    String routingDataCacheKey = getRoutingDataCacheKey(routingDataReaderType, endpoint);
+    Map<String, List<String>> rawRoutingData = _rawRoutingDataMap.get(routingDataCacheKey);
+    if (rawRoutingData == null) {
+      synchronized (RoutingDataManager.class) {
+        rawRoutingData = _rawRoutingDataMap.get(routingDataCacheKey);
+        if (rawRoutingData == null) {
+          RoutingDataReader reader = resolveRoutingDataReader(routingDataReaderType);
+          rawRoutingData = reader.getRawRoutingData(endpoint);
+          _rawRoutingDataMap.put(routingDataCacheKey, rawRoutingData);
+        }
+      }
+    }
+    return rawRoutingData;
+  }
+
+  /**
+   * Returns the routing data read from MSDS in a MetadataStoreRoutingData format by querying the
+   * MSDS configured in the JVM config.
+   * @return MetadataStoreRoutingData
+   */
+  public static MetadataStoreRoutingData getMetadataStoreRoutingData()
+      throws InvalidRoutingDataException {
+    if (DEFAULT_MSDS_ENDPOINT == null || DEFAULT_MSDS_ENDPOINT.isEmpty()) {
+      throw new IllegalStateException(
+          "HttpRoutingDataReader was unable to find a valid MSDS endpoint String in System "
+              + "Properties!");
+    }
+    return getMetadataStoreRoutingData(RoutingDataReaderType.HTTP, DEFAULT_MSDS_ENDPOINT);
+  }
+
+  /**
+   * Returns the routing data read from MSDS as a MetadataStoreRoutingData object.
+   * @param routingDataReaderType
+   * @param endpoint
+   * @return
+   * @throws IOException
+   * @throws InvalidRoutingDataException
+   */
+  public static MetadataStoreRoutingData getMetadataStoreRoutingData(
+      RoutingDataReaderType routingDataReaderType, String endpoint)
+      throws InvalidRoutingDataException {
+    String routingDataCacheKey = getRoutingDataCacheKey(routingDataReaderType, endpoint);
+    MetadataStoreRoutingData metadataStoreRoutingData =
+        _metadataStoreRoutingDataMap.get(routingDataCacheKey);
+    if (metadataStoreRoutingData == null) {
+      synchronized (RoutingDataManager.class) {
+        metadataStoreRoutingData = _metadataStoreRoutingDataMap.get(routingDataCacheKey);
+        if (metadataStoreRoutingData == null) {
+          metadataStoreRoutingData =
+              new TrieRoutingData(getRawRoutingData(routingDataReaderType, endpoint));
+          _metadataStoreRoutingDataMap.put(routingDataCacheKey, metadataStoreRoutingData);
+        }
+      }
+    }
+    return metadataStoreRoutingData;
+  }
+
+  /**
+   * Clears the statically-cached routing data and private fields.
+   */
+  public synchronized static void reset() {
+    _rawRoutingDataMap.clear();
+    _metadataStoreRoutingDataMap.clear();
+  }
+
+  /**
+   * Returns an appropriate instance of RoutingDataReader given the type.
+   * @param routingDataReaderType
+   * @return
+   */
+  private static RoutingDataReader resolveRoutingDataReader(
+      RoutingDataReaderType routingDataReaderType) {
+    // Instantiate an instance of routing data reader using the type
+    try {
+      return (RoutingDataReader) Class.forName(routingDataReaderType.getClassName()).newInstance();
+    } catch (ClassNotFoundException | IllegalAccessException | InstantiationException e) {
+      throw new MultiZkException(

Review comment:
       Do we want to add some logs here for debugging purpose? Because rely on exception stack trace could be too long when we handle it.

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/routing/HttpRoutingDataReader.java
##########
@@ -0,0 +1,132 @@
+package org.apache.helix.zookeeper.routing;
+
+/*
+ * 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.
+ */
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.helix.msdcommon.constant.MetadataStoreRoutingConstants;
+import org.apache.helix.msdcommon.datamodel.MetadataStoreRoutingData;
+import org.apache.helix.msdcommon.datamodel.TrieRoutingData;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.exception.MultiZkException;
+import org.apache.http.HttpEntity;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.DefaultBackoffStrategy;
+import org.apache.http.impl.client.DefaultHttpRequestRetryHandler;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+
+
+/**
+ * HTTP-based RoutingDataReader that makes an HTTP call to Metadata Store Directory Service (REST)
+ * to fetch routing data.
+ */
+public class HttpRoutingDataReader implements RoutingDataReader {
+  private static final int DEFAULT_HTTP_TIMEOUT_IN_MS = 5000;
+
+  /**
+   * Returns an object form of metadata store routing data.
+   * @param endpoint
+   * @return
+   */
+  @Override
+  public MetadataStoreRoutingData getMetadataStoreRoutingData(String endpoint) {
+    try {
+      return new TrieRoutingData(getRawRoutingData(endpoint));
+    } catch (InvalidRoutingDataException e) {
+      throw new MultiZkException(e);
+    }
+  }
+
+  /**
+   * Returns a map form of metadata store routing data.
+   * The map fields stand for metadata store realm address (key), and a corresponding list of ZK
+   * path sharding keys (key).
+   * @param endpoint
+   * @return
+   */
+  @Override
+  public Map<String, List<String>> getRawRoutingData(String endpoint) {
+    try {
+      String routingDataJson = getAllRoutingData(endpoint);
+      return parseRoutingData(routingDataJson);
+    } catch (IOException e) {
+      throw new MultiZkException(e);
+    }
+  }
+
+  /**
+   * Makes an HTTP call to fetch all routing data.
+   * @return
+   * @throws IOException
+   */
+  private String getAllRoutingData(String msdsEndpoint) throws IOException {
+    // Note that MSDS_ENDPOINT should provide high-availability - it risks becoming a single point
+    // of failure if it's backed by a single IP address/host
+    // Retry count is 3 by default.
+    HttpGet requestAllData = new HttpGet(
+        msdsEndpoint + MetadataStoreRoutingConstants.MSDS_GET_ALL_ROUTING_DATA_ENDPOINT);
+
+    // Define timeout configs
+    RequestConfig config = RequestConfig.custom().setConnectTimeout(DEFAULT_HTTP_TIMEOUT_IN_MS)
+        .setConnectionRequestTimeout(DEFAULT_HTTP_TIMEOUT_IN_MS)
+        .setSocketTimeout(DEFAULT_HTTP_TIMEOUT_IN_MS).build();
+
+    try (CloseableHttpClient httpClient = HttpClients.custom().setDefaultRequestConfig(config)
+        .setConnectionBackoffStrategy(new DefaultBackoffStrategy())
+        .setRetryHandler(new DefaultHttpRequestRetryHandler()).build()) {
+      // Return the JSON because try-resources clause closes the CloseableHttpResponse
+      HttpEntity entity = httpClient.execute(requestAllData).getEntity();
+      if (entity == null) {
+        throw new IOException("Response's entity is null!");
+      }
+      return EntityUtils.toString(entity, "UTF-8");
+    }

Review comment:
       Shall we have finally for returning something if there some exception in the try-with-resource statement?

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/routing/RoutingDataManager.java
##########
@@ -0,0 +1,179 @@
+package org.apache.helix.zookeeper.routing;
+
+/*
+ * 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.
+ */
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.helix.msdcommon.constant.MetadataStoreRoutingConstants;
+import org.apache.helix.msdcommon.datamodel.MetadataStoreRoutingData;
+import org.apache.helix.msdcommon.datamodel.TrieRoutingData;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.constant.RoutingDataReaderType;
+import org.apache.helix.zookeeper.exception.MultiZkException;
+
+
+/**
+ * RoutingDataManager is a static Singleton that
+ * 1. resolves RoutingDataReader based on the system config given
+ * 2. caches routing data
+ * 3. provides public methods for reading routing data from various sources (configurable)
+ */
+public class RoutingDataManager {
+  /** HTTP call to MSDS is used to fetch routing data by default */
+  private static final String DEFAULT_MSDS_ENDPOINT =
+      System.getProperty(MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY);
+
+  /** Double-checked locking requires that the following fields be final (volatile) */
+  // The following map stands for (RoutingDataReaderType_endpoint ID, Raw Routing Data)
+  private static final Map<String, Map<String, List<String>>> _rawRoutingDataMap =
+      new ConcurrentHashMap<>();
+  // The following map stands for (RoutingDataReaderType_endpoint ID, MetadataStoreRoutingData)
+  private static final Map<String, MetadataStoreRoutingData> _metadataStoreRoutingDataMap =
+      new ConcurrentHashMap<>();
+
+  /**
+   * This class is a Singleton.
+   */
+  private RoutingDataManager() {
+  }

Review comment:
       If we would like to make it as singleton, we dont have to make all methods to be static but just add a static method to get the singleton instance. Somehow, this implementation mixing static utils with singleton implementation.




----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org