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/20 04:09:30 UTC
[incubator-pinot] 01/01: Fix the issue where
ZkCacheBaseDataAccessor.getChildren() can return list with null znRecords
This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch null_zn_records
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
commit a2b0667bd9951d6a6c2cf462ba888923582a8bcb
Author: Jackie (Xiaotian) Jiang <xa...@linkedin.com>
AuthorDate: Fri Apr 19 21:07:36 2019 -0700
Fix the issue where ZkCacheBaseDataAccessor.getChildren() can return list with null znRecords
This can happen if the record gets removed while calling this method.
Need to handle it to prevent NPE.
---
.../pinot/common/metadata/ZKMetadataProvider.java | 83 +++++++++++-----------
.../helix/core/PinotHelixResourceManager.java | 8 ++-
.../core/realtime/PinotRealtimeSegmentManager.java | 17 ++---
3 files changed, 59 insertions(+), 49 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..9df4d84 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,23 @@ 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) {
+ List<OfflineSegmentZKMetadata> offlineSegmentZKMetadataList = new ArrayList<>(znRecords.size());
+ for (ZNRecord znRecord : znRecords) {
+ // NOTE: It is possible that znRecord is null if the record gets removed while calling this method.
+ // No need to log warning when znRecord is null because it is already logged in ZkCacheBaseDataAccessor.
+ if (znRecord != null) {
+ offlineSegmentZKMetadataList.add(new OfflineSegmentZKMetadata(znRecord));
}
}
+ return offlineSegmentZKMetadataList;
+ } else {
+ LOGGER.warn("Path: {} does not exist", parentPath);
+ return Collections.emptyList();
}
- return resultList;
}
/**
@@ -319,22 +321,24 @@ 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) {
+ List<RealtimeSegmentZKMetadata> realtimeSegmentZKMetadataList = new ArrayList<>(znRecords.size());
+ for (ZNRecord znRecord : znRecords) {
+ // NOTE: It is possible that znRecord is null if the record gets removed while calling this method.
+ // No need to log warning when znRecord is null because it is already logged in ZkCacheBaseDataAccessor.
+ if (znRecord != null) {
+ realtimeSegmentZKMetadataList.add(new RealtimeSegmentZKMetadata(znRecord));
}
}
+ return realtimeSegmentZKMetadataList;
+ } else {
+ LOGGER.warn("Path: {} does not exist", parentPath);
+ return Collections.emptyList();
}
- return resultList;
}
/**
@@ -342,25 +346,24 @@ 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) {
+ List<LLCRealtimeSegmentZKMetadata> llcRealtimeSegmentZKMetadataList = new ArrayList<>(znRecords.size());
+ for (ZNRecord znRecord : znRecords) {
+ // NOTE: It is possible that znRecord is null if the record gets removed while calling this method.
+ // No need to log warning when znRecord is null because it is already logged in ZkCacheBaseDataAccessor.
+ if (znRecord != null) {
+ llcRealtimeSegmentZKMetadataList.add(new LLCRealtimeSegmentZKMetadata(znRecord));
}
}
+ 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..2341dab 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
@@ -290,7 +290,13 @@ 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)));
+ for (ZNRecord znRecord : znRecords) {
+ // NOTE: It is possible that znRecord is null if the record gets removed while calling this method.
+ // No need to log warning when znRecord is null because it is already logged in ZkCacheBaseDataAccessor.
+ if (znRecord != null) {
+ instanceConfigs.add(new InstanceConfig(znRecord));
+ }
+ }
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..74f80c9 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,11 @@ 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.
+ // No need to log warning when znRecord is null because it is already logged in ZkCacheBaseDataAccessor.
+ if (tableConfigZnRecord == null) {
+ continue;
+ }
try {
String znRecordId = tableConfigZnRecord.getId();
if (TableNameBuilder.getTableTypeFromTableName(znRecordId) == TableType.REALTIME) {
@@ -374,13 +380,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