You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by ja...@apache.org on 2019/04/23 04:50:58 UTC
[incubator-pinot] branch master updated: Fix the issue where
ZkCacheBaseDataAccessor.getChildren() can return list with null znRecords
(#4152)
This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 2221623 Fix the issue where ZkCacheBaseDataAccessor.getChildren() can return list with null znRecords (#4152)
2221623 is described below
commit 222162323d7a782a10d831ced93b95538a90e74a
Author: Xiaotian (Jackie) Jiang <17...@users.noreply.github.com>
AuthorDate: Mon Apr 22 21:50:53 2019 -0700
Fix the issue where ZkCacheBaseDataAccessor.getChildren() can return list with null znRecords (#4152)
This can happen if the record gets removed while calling this method.
Need to handle it to prevent NPE.
---
.../pinot/common/metadata/ZKMetadataProvider.java | 98 +++++++++++++---------
.../helix/core/PinotHelixResourceManager.java | 14 +++-
.../core/realtime/PinotRealtimeSegmentManager.java | 16 ++--
3 files changed, 78 insertions(+), 50 deletions(-)
diff --git a/pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java b/pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
index 07e3533..6634b27 100644
--- a/pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
+++ b/pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
@@ -297,21 +297,28 @@ public class ZKMetadataProvider {
*/
public static List<OfflineSegmentZKMetadata> getOfflineSegmentZKMetadataListForTable(
ZkHelixPropertyStore<ZNRecord> propertyStore, String tableName) {
- List<OfflineSegmentZKMetadata> resultList = new ArrayList<>();
- if (propertyStore == null) {
- return resultList;
- }
String offlineTableName = TableNameBuilder.OFFLINE.tableNameWithType(tableName);
- if (propertyStore.exists(constructPropertyStorePathForResource(offlineTableName), AccessOption.PERSISTENT)) {
- List<ZNRecord> znRecordList = propertyStore
- .getChildren(constructPropertyStorePathForResource(offlineTableName), null, AccessOption.PERSISTENT);
- if (znRecordList != null) {
- for (ZNRecord record : znRecordList) {
- resultList.add(new OfflineSegmentZKMetadata(record));
+ String parentPath = constructPropertyStorePathForResource(offlineTableName);
+ List<ZNRecord> znRecords = propertyStore.getChildren(parentPath, null, AccessOption.PERSISTENT);
+ if (znRecords != null) {
+ int numZNRecords = znRecords.size();
+ List<OfflineSegmentZKMetadata> offlineSegmentZKMetadataList = new ArrayList<>(numZNRecords);
+ for (ZNRecord znRecord : znRecords) {
+ // NOTE: it is possible that znRecord is null if the record gets removed while calling this method
+ if (znRecord != null) {
+ offlineSegmentZKMetadataList.add(new OfflineSegmentZKMetadata(znRecord));
}
}
+ int numNullZNRecords = numZNRecords - offlineSegmentZKMetadataList.size();
+ if (numNullZNRecords > 0) {
+ LOGGER.warn("Failed to read {}/{} offline segment ZK metadata under path: {}", numZNRecords - numNullZNRecords,
+ numZNRecords, parentPath);
+ }
+ return offlineSegmentZKMetadataList;
+ } else {
+ LOGGER.warn("Path: {} does not exist", parentPath);
+ return Collections.emptyList();
}
- return resultList;
}
/**
@@ -319,22 +326,29 @@ public class ZKMetadataProvider {
* segment names are needed.
*/
public static List<RealtimeSegmentZKMetadata> getRealtimeSegmentZKMetadataListForTable(
- ZkHelixPropertyStore<ZNRecord> propertyStore, String resourceName) {
- List<RealtimeSegmentZKMetadata> resultList = new ArrayList<>();
- if (propertyStore == null) {
- return resultList;
- }
- String realtimeTableName = TableNameBuilder.REALTIME.tableNameWithType(resourceName);
- if (propertyStore.exists(constructPropertyStorePathForResource(realtimeTableName), AccessOption.PERSISTENT)) {
- List<ZNRecord> znRecordList = propertyStore
- .getChildren(constructPropertyStorePathForResource(realtimeTableName), null, AccessOption.PERSISTENT);
- if (znRecordList != null) {
- for (ZNRecord record : znRecordList) {
- resultList.add(new RealtimeSegmentZKMetadata(record));
+ ZkHelixPropertyStore<ZNRecord> propertyStore, String tableName) {
+ String realtimeTableName = TableNameBuilder.REALTIME.tableNameWithType(tableName);
+ String parentPath = constructPropertyStorePathForResource(realtimeTableName);
+ List<ZNRecord> znRecords = propertyStore.getChildren(parentPath, null, AccessOption.PERSISTENT);
+ if (znRecords != null) {
+ int numZNRecords = znRecords.size();
+ List<RealtimeSegmentZKMetadata> realtimeSegmentZKMetadataList = new ArrayList<>(numZNRecords);
+ for (ZNRecord znRecord : znRecords) {
+ // NOTE: it is possible that znRecord is null if the record gets removed while calling this method
+ if (znRecord != null) {
+ realtimeSegmentZKMetadataList.add(new RealtimeSegmentZKMetadata(znRecord));
}
}
+ int numNullZNRecords = numZNRecords - realtimeSegmentZKMetadataList.size();
+ if (numNullZNRecords > 0) {
+ LOGGER.warn("Failed to read {}/{} realtime segment ZK metadata under path: {}", numZNRecords - numNullZNRecords,
+ numZNRecords, parentPath);
+ }
+ return realtimeSegmentZKMetadataList;
+ } else {
+ LOGGER.warn("Path: {} does not exist", parentPath);
+ return Collections.emptyList();
}
- return resultList;
}
/**
@@ -342,25 +356,29 @@ public class ZKMetadataProvider {
* only segment names are needed.
*/
public static List<LLCRealtimeSegmentZKMetadata> getLLCRealtimeSegmentZKMetadataListForTable(
- ZkHelixPropertyStore<ZNRecord> propertyStore, String resourceName) {
- List<LLCRealtimeSegmentZKMetadata> resultList = new ArrayList<>();
- if (propertyStore == null) {
- return resultList;
- }
- String realtimeTableName = TableNameBuilder.REALTIME.tableNameWithType(resourceName);
- if (propertyStore.exists(constructPropertyStorePathForResource(realtimeTableName), AccessOption.PERSISTENT)) {
- List<ZNRecord> znRecordList = propertyStore
- .getChildren(constructPropertyStorePathForResource(realtimeTableName), null, AccessOption.PERSISTENT);
- if (znRecordList != null) {
- for (ZNRecord record : znRecordList) {
- RealtimeSegmentZKMetadata realtimeSegmentZKMetadata = new RealtimeSegmentZKMetadata(record);
- if (SegmentName.isLowLevelConsumerSegmentName(realtimeSegmentZKMetadata.getSegmentName())) {
- resultList.add(new LLCRealtimeSegmentZKMetadata(record));
- }
+ ZkHelixPropertyStore<ZNRecord> propertyStore, String tableName) {
+ String realtimeTableName = TableNameBuilder.REALTIME.tableNameWithType(tableName);
+ String parentPath = constructPropertyStorePathForResource(realtimeTableName);
+ List<ZNRecord> znRecords = propertyStore.getChildren(parentPath, null, AccessOption.PERSISTENT);
+ if (znRecords != null) {
+ int numZNRecords = znRecords.size();
+ List<LLCRealtimeSegmentZKMetadata> llcRealtimeSegmentZKMetadataList = new ArrayList<>(numZNRecords);
+ for (ZNRecord znRecord : znRecords) {
+ // NOTE: it is possible that znRecord is null if the record gets removed while calling this method
+ if (znRecord != null) {
+ llcRealtimeSegmentZKMetadataList.add(new LLCRealtimeSegmentZKMetadata(znRecord));
}
}
+ int numNullZNRecords = numZNRecords - llcRealtimeSegmentZKMetadataList.size();
+ if (numNullZNRecords > 0) {
+ LOGGER.warn("Failed to read {}/{} LLC realtime segment ZK metadata under path: {}",
+ numZNRecords - numNullZNRecords, numZNRecords, parentPath);
+ }
+ return llcRealtimeSegmentZKMetadataList;
+ } else {
+ LOGGER.warn("Path: {} does not exist", parentPath);
+ return Collections.emptyList();
}
- return resultList;
}
/**
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 ef94094..0d0f6c0 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
@@ -289,8 +289,18 @@ public class PinotHelixResourceManager {
*/
public List<InstanceConfig> getAllHelixInstanceConfigs() {
List<ZNRecord> znRecords = _cacheInstanceConfigsDataAccessor.getChildren("/", null, AccessOption.PERSISTENT);
- List<InstanceConfig> instanceConfigs = new ArrayList<>(znRecords.size());
- znRecords.forEach(znRecord -> instanceConfigs.add(new InstanceConfig(znRecord)));
+ int numZNRecords = znRecords.size();
+ List<InstanceConfig> instanceConfigs = new ArrayList<>(numZNRecords);
+ for (ZNRecord znRecord : znRecords) {
+ // NOTE: it is possible that znRecord is null if the record gets removed while calling this method
+ if (znRecord != null) {
+ instanceConfigs.add(new InstanceConfig(znRecord));
+ }
+ }
+ int numNullZNRecords = numZNRecords - instanceConfigs.size();
+ if (numNullZNRecords > 0) {
+ LOGGER.warn("Failed to read {}/{} instance configs", numZNRecords - numNullZNRecords, numZNRecords);
+ }
return instanceConfigs;
}
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotRealtimeSegmentManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotRealtimeSegmentManager.java
index 5a849e1..331a4a0 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotRealtimeSegmentManager.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotRealtimeSegmentManager.java
@@ -82,7 +82,8 @@ public class PinotRealtimeSegmentManager implements HelixPropertyListener, IZkCh
private ControllerMetrics _controllerMetrics;
private final ControllerLeadershipManager _controllerLeadershipManager;
- public PinotRealtimeSegmentManager(PinotHelixResourceManager pinotManager, ControllerLeadershipManager controllerLeadershipManager) {
+ public PinotRealtimeSegmentManager(PinotHelixResourceManager pinotManager,
+ ControllerLeadershipManager controllerLeadershipManager) {
_pinotHelixResourceManager = pinotManager;
_controllerLeadershipManager = controllerLeadershipManager;
String clusterName = _pinotHelixResourceManager.getHelixClusterName();
@@ -331,6 +332,10 @@ public class PinotRealtimeSegmentManager implements HelixPropertyListener, IZkCh
}
for (ZNRecord tableConfigZnRecord : tableConfigs) {
+ // NOTE: it is possible that znRecord is null if the record gets removed while calling this method
+ if (tableConfigZnRecord == null) {
+ continue;
+ }
try {
String znRecordId = tableConfigZnRecord.getId();
if (TableNameBuilder.getTableTypeFromTableName(znRecordId) == TableType.REALTIME) {
@@ -374,13 +379,8 @@ public class PinotRealtimeSegmentManager implements HelixPropertyListener, IZkCh
} catch (Exception e) {
// we want to continue setting watches for other tables for any kind of exception here so that
// errors with one table don't impact others
- if (tableConfigZnRecord == null) {
- // Can happen if the table config zn record failed to parse.
- LOGGER.error("Got null ZN record for table config", e);
- } else {
- LOGGER.error("Caught exception while processing ZNRecord id: {}. Skipping node to continue setting watches",
- tableConfigZnRecord.getId(), e);
- }
+ LOGGER.error("Caught exception while processing ZNRecord id: {}. Skipping node to continue setting watches",
+ tableConfigZnRecord.getId(), e);
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org