You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by jl...@apache.org on 2019/01/29 19:12:23 UTC
[incubator-pinot] branch better-handle-NPE-from-cacheDataAccessor
updated: Address PR comment
This is an automated email from the ASF dual-hosted git repository.
jlli pushed a commit to branch better-handle-NPE-from-cacheDataAccessor
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/better-handle-NPE-from-cacheDataAccessor by this push:
new 8fe8572 Address PR comment
8fe8572 is described below
commit 8fe8572a556e8ea02e5a0b531253a73a0c460228
Author: Jack Li(Analytics Engineering) <jl...@jlli-mn1.linkedin.biz>
AuthorDate: Tue Jan 29 11:12:01 2019 -0800
Address PR comment
---
.../api/resources/PinotInstanceRestletResource.java | 7 ++-----
.../controller/api/resources/PqlQueryResource.java | 7 ++-----
.../pinot/controller/api/upload/SegmentValidator.java | 8 +++++---
.../helix/core/PinotHelixResourceManager.java | 18 +++++++++---------
.../apache/pinot/controller/util/TableSizeReader.java | 6 ++++--
.../controller/validation/StorageQuotaChecker.java | 6 ++++--
.../helix/core/PinotHelixResourceManagerTest.java | 14 ++------------
7 files changed, 28 insertions(+), 38 deletions(-)
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java
index a1c1a1b..b717bd0 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java
@@ -37,7 +37,6 @@ import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import org.apache.helix.model.InstanceConfig;
-import org.apache.pinot.common.exception.InstanceNotFoundException;
import org.apache.pinot.common.utils.JsonUtils;
import org.apache.pinot.controller.api.pojos.Instance;
import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
@@ -95,10 +94,8 @@ public class PinotInstanceRestletResource {
public String getInstance(
@ApiParam(value = "Instance name", required = true, example = "Server_a.b.com_20000 | Broker_my.broker.com_30000")
@PathParam("instanceName") String instanceName) {
- InstanceConfig instanceConfig;
- try {
- instanceConfig = pinotHelixResourceManager.getHelixInstanceConfig(instanceName);
- } catch (InstanceNotFoundException e) {
+ InstanceConfig instanceConfig = pinotHelixResourceManager.getHelixInstanceConfig(instanceName);
+ if (instanceConfig == null) {
throw new ControllerApplicationException(LOGGER, "Instance " + instanceName + " not found",
Response.Status.NOT_FOUND);
}
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PqlQueryResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PqlQueryResource.java
index 956de8d..8fe77dc 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PqlQueryResource.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PqlQueryResource.java
@@ -43,7 +43,6 @@ import javax.ws.rs.core.HttpHeaders;
import org.apache.helix.model.InstanceConfig;
import org.apache.pinot.common.Utils;
import org.apache.pinot.common.config.TableNameBuilder;
-import org.apache.pinot.common.exception.InstanceNotFoundException;
import org.apache.pinot.common.exception.QueryException;
import org.apache.pinot.common.request.BrokerRequest;
import org.apache.pinot.common.utils.JsonUtils;
@@ -131,10 +130,8 @@ public class PqlQueryResource {
// Send query to a random broker.
String instanceId = instanceIds.get(RANDOM.nextInt(instanceIds.size()));
- InstanceConfig instanceConfig;
- try {
- instanceConfig = _pinotHelixResourceManager.getHelixInstanceConfig(instanceId);
- } catch (InstanceNotFoundException e) {
+ InstanceConfig instanceConfig = _pinotHelixResourceManager.getHelixInstanceConfig(instanceId);
+ if (instanceConfig == null) {
LOGGER.error("Instance {} not found", instanceId);
return QueryException.INTERNAL_ERROR.toString();
}
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/SegmentValidator.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/SegmentValidator.java
index 3282f60..835486e 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/SegmentValidator.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/SegmentValidator.java
@@ -26,6 +26,7 @@ import javax.ws.rs.core.Response;
import org.apache.commons.httpclient.HttpConnectionManager;
import org.apache.pinot.common.config.TableConfig;
import org.apache.pinot.common.config.TableNameBuilder;
+import org.apache.pinot.common.exception.InstanceNotFoundException;
import org.apache.pinot.common.exception.InvalidConfigException;
import org.apache.pinot.common.metadata.ZKMetadataProvider;
import org.apache.pinot.common.metrics.ControllerMetrics;
@@ -63,7 +64,7 @@ public class SegmentValidator {
}
- public void validateSegment(SegmentMetadata segmentMetadata, File tempSegmentDir) throws Exception {
+ public void validateSegment(SegmentMetadata segmentMetadata, File tempSegmentDir) {
String rawTableName = segmentMetadata.getTableName();
String offlineTableName = TableNameBuilder.OFFLINE.tableNameWithType(rawTableName);
String segmentName = segmentMetadata.getName();
@@ -78,7 +79,7 @@ public class SegmentValidator {
StorageQuotaChecker.QuotaCheckerResponse quotaResponse;
try {
quotaResponse = checkStorageQuota(tempSegmentDir, segmentMetadata, offlineTableConfig);
- } catch (InvalidConfigException e) {
+ } catch (InvalidConfigException | InstanceNotFoundException e) {
// Admin port is missing, return response with 500 status code.
throw new ControllerApplicationException(LOGGER,
"Quota check failed for segment: " + segmentName + " of table: " + offlineTableName + ", reason: " + e.getMessage(), Response.Status.INTERNAL_SERVER_ERROR);
@@ -104,7 +105,8 @@ public class SegmentValidator {
* @param offlineTableConfig offline table configuration. This should not be null.
*/
private StorageQuotaChecker.QuotaCheckerResponse checkStorageQuota(@Nonnull File segmentFile,
- @Nonnull SegmentMetadata metadata, @Nonnull TableConfig offlineTableConfig) throws Exception {
+ @Nonnull SegmentMetadata metadata, @Nonnull TableConfig offlineTableConfig)
+ throws InvalidConfigException, InstanceNotFoundException {
if (!_controllerConf.getEnableStorageQuotaCheck()) {
return StorageQuotaChecker.success("Quota check is disabled");
}
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
index da979cd..fb74e6d 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
@@ -284,15 +284,9 @@ public class PinotHelixResourceManager {
* @param instanceId Instance Id
* @return Helix instance config
*/
- @Nonnull
- public InstanceConfig getHelixInstanceConfig(@Nonnull String instanceId) throws InstanceNotFoundException {
+ public InstanceConfig getHelixInstanceConfig(@Nonnull String instanceId) {
ZNRecord znRecord = _cacheInstanceConfigsDataAccessor.get("/" + instanceId, null, AccessOption.PERSISTENT);
- if (znRecord == null) {
- String errorMsg = String.format("Instance %s not found", instanceId);
- LOGGER.warn(errorMsg);
- throw new InstanceNotFoundException(errorMsg);
- }
- return new InstanceConfig(znRecord);
+ return znRecord != null ? new InstanceConfig(znRecord) : null;
}
/**
@@ -2199,11 +2193,17 @@ public class PinotHelixResourceManager {
* server instances. With BiMap, both mappings are easily available
*/
public @Nonnull
- BiMap<String, String> getDataInstanceAdminEndpoints(@Nonnull Set<String> instances) throws Exception {
+ BiMap<String, String> getDataInstanceAdminEndpoints(@Nonnull Set<String> instances)
+ throws InstanceNotFoundException, InvalidConfigException {
Preconditions.checkNotNull(instances);
BiMap<String, String> endpointToInstance = HashBiMap.create(instances.size());
for (String instance : instances) {
InstanceConfig helixInstanceConfig = getHelixInstanceConfig(instance);
+ if (helixInstanceConfig == null) {
+ String message = String.format("Instance %s not found", instance);
+ LOGGER.error(message);
+ throw new InstanceNotFoundException(message);
+ }
ZNRecord record = helixInstanceConfig.getRecord();
String[] hostnameSplit = helixInstanceConfig.getHostName().split("_");
Preconditions.checkState(hostnameSplit.length >= 2);
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/util/TableSizeReader.java b/pinot-controller/src/main/java/org/apache/pinot/controller/util/TableSizeReader.java
index 41aa548..55d3ac8 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/util/TableSizeReader.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/util/TableSizeReader.java
@@ -31,6 +31,7 @@ import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.apache.commons.httpclient.HttpConnectionManager;
import org.apache.pinot.common.config.TableNameBuilder;
+import org.apache.pinot.common.exception.InstanceNotFoundException;
import org.apache.pinot.common.exception.InvalidConfigException;
import org.apache.pinot.common.metrics.ControllerGauge;
import org.apache.pinot.common.metrics.ControllerMetrics;
@@ -72,7 +73,7 @@ public class TableSizeReader {
*/
@Nullable
public TableSizeDetails getTableSizeDetails(@Nonnull String tableName, @Nonnegative int timeoutMsec)
- throws Exception {
+ throws InstanceNotFoundException, InvalidConfigException {
Preconditions.checkNotNull(tableName, "Table name should not be null");
Preconditions.checkArgument(timeoutMsec > 0, "Timeout value must be greater than 0");
@@ -146,7 +147,8 @@ public class TableSizeReader {
public Map<String, SegmentSizeInfo> serverInfo = new HashMap<>();
}
- public TableSubTypeSizeDetails getTableSubtypeSize(String tableNameWithType, int timeoutMs) throws Exception {
+ public TableSubTypeSizeDetails getTableSubtypeSize(String tableNameWithType, int timeoutMs)
+ throws InvalidConfigException, InstanceNotFoundException {
Map<String, List<String>> serverToSegmentsMap = _helixResourceManager.getServerToSegmentsMap(tableNameWithType);
ServerTableSizeReader serverTableSizeReader = new ServerTableSizeReader(_executor, _connectionManager);
BiMap<String, String> endpoints = _helixResourceManager.getDataInstanceAdminEndpoints(serverToSegmentsMap.keySet());
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/validation/StorageQuotaChecker.java b/pinot-controller/src/main/java/org/apache/pinot/controller/validation/StorageQuotaChecker.java
index 1330d6f..ca02647 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/validation/StorageQuotaChecker.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/validation/StorageQuotaChecker.java
@@ -26,6 +26,7 @@ import javax.annotation.Nonnull;
import org.apache.commons.io.FileUtils;
import org.apache.pinot.common.config.QuotaConfig;
import org.apache.pinot.common.config.TableConfig;
+import org.apache.pinot.common.exception.InstanceNotFoundException;
import org.apache.pinot.common.exception.InvalidConfigException;
import org.apache.pinot.common.metrics.ControllerGauge;
import org.apache.pinot.common.metrics.ControllerMetrics;
@@ -85,7 +86,8 @@ public class StorageQuotaChecker {
*
*/
public QuotaCheckerResponse isSegmentStorageWithinQuota(@Nonnull File segmentFile, @Nonnull String tableNameWithType,
- @Nonnull String segmentName, @Nonnegative int timeoutMsec) throws Exception {
+ @Nonnull String segmentName, @Nonnegative int timeoutMsec)
+ throws InstanceNotFoundException, InvalidConfigException {
Preconditions.checkNotNull(segmentFile);
Preconditions.checkNotNull(tableNameWithType);
Preconditions.checkNotNull(segmentName);
@@ -120,7 +122,7 @@ public class StorageQuotaChecker {
TableSizeReader.TableSubTypeSizeDetails tableSubtypeSize = null;
try {
tableSubtypeSize = _tableSizeReader.getTableSubtypeSize(tableNameWithType, timeoutMsec);
- } catch (InvalidConfigException e) {
+ } catch (InvalidConfigException | InstanceNotFoundException e) {
LOGGER.error("Failed to get table size for table {}", tableNameWithType, e);
throw e;
}
diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java
index 01dd027..4eabcfb 100644
--- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java
+++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java
@@ -19,7 +19,6 @@
package org.apache.pinot.controller.helix.core;
import com.google.common.collect.BiMap;
-import java.util.HashSet;
import java.util.List;
import java.util.Random;
import java.util.Set;
@@ -33,8 +32,6 @@ import org.apache.pinot.common.config.TableConfig;
import org.apache.pinot.common.config.TableNameBuilder;
import org.apache.pinot.common.config.TagNameUtils;
import org.apache.pinot.common.config.Tenant;
-import org.apache.pinot.common.exception.InstanceNotFoundException;
-import org.apache.pinot.common.exception.InvalidConfigException;
import org.apache.pinot.common.metadata.ZKMetadataProvider;
import org.apache.pinot.common.metadata.segment.OfflineSegmentZKMetadata;
import org.apache.pinot.common.metadata.segment.RealtimeSegmentZKMetadata;
@@ -109,7 +106,7 @@ public class PinotHelixResourceManagerTest extends ControllerTest {
zkClient.close();
}
- private void modifyExistingInstanceConfig(ZkClient zkClient) throws InterruptedException, InstanceNotFoundException {
+ private void modifyExistingInstanceConfig(ZkClient zkClient) throws InterruptedException {
String instanceName = "Server_localhost_" + new Random().nextInt(NUM_INSTANCES);
String instanceConfigPath = PropertyPathBuilder.instanceConfig(_helixClusterName, instanceName);
Assert.assertTrue(zkClient.exists(instanceConfigPath));
@@ -225,7 +222,7 @@ public class PinotHelixResourceManagerTest extends ControllerTest {
}
@Test
- public void testRetrieveMetadata() throws Exception {
+ public void testRetrieveMetadata() {
String segmentName = "testSegment";
// Test retrieving OFFLINE segment ZK metadata
@@ -259,13 +256,6 @@ public class PinotHelixResourceManagerTest extends ControllerTest {
}
}
- @Test
- public void testGetDataInstanceAdminEndpoints() {
- Set<String> fakeInstances = new HashSet<>();
- new Random().nextInt(NUM_INSTANCES);
- }
-
-
@AfterClass
public void tearDown() {
stopController();
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org