klsince commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r685440074
##########
File path:
pinot-common/src/test/java/org/apache/pinot/common/tier/TierSegmentSelectorTest.java
##########
@@ -82,8 +81,8 @@ public void testTimeBasedSegmentSelector() {
// realtime segment
segmentName = "myTable__4__1__" + now;
tableNameWithType = "myTable_REALTIME";
- LLCRealtimeSegmentZKMetadata realtimeSegmentZKMetadata = new
LLCRealtimeSegmentZKMetadata();
- realtimeSegmentZKMetadata.setSegmentName(segmentName);
+ SegmentZKMetadata realtimeSegmentZKMetadata = new
SegmentZKMetadata(segmentName);
+
offlineSegmentZKMetadata.setSegmentType(CommonConstants.Segment.SegmentType.REALTIME);
Review comment:
realtimeSegmentZKMetadata?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -309,89 +272,27 @@ public static Schema getTableSchema(@Nonnull
ZkHelixPropertyStore<ZNRecord> prop
* NOTE: this method is very expensive, use {@link
#getSegments(ZkHelixPropertyStore, String)} instead if only segment
* segment names are needed.
Review comment:
typo `... only segment segment names...`
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java
##########
@@ -309,89 +272,27 @@ public static Schema getTableSchema(@Nonnull
ZkHelixPropertyStore<ZNRecord> prop
* NOTE: this method is very expensive, use {@link
#getSegments(ZkHelixPropertyStore, String)} instead if only segment
* segment names are needed.
*/
- public static List<OfflineSegmentZKMetadata>
getOfflineSegmentZKMetadataListForTable(
- ZkHelixPropertyStore<ZNRecord> propertyStore, String tableName) {
- String offlineTableName =
TableNameBuilder.OFFLINE.tableNameWithType(tableName);
- String parentPath =
constructPropertyStorePathForResource(offlineTableName);
- List<ZNRecord> znRecords = propertyStore.getChildren(parentPath, null,
AccessOption.PERSISTENT,
- CommonConstants.Helix.ZkClient.RETRY_COUNT,
CommonConstants.Helix.ZkClient.RETRY_INTERVAL_MS);
+ public static List<SegmentZKMetadata>
getSegmentsZKMetadata(ZkHelixPropertyStore<ZNRecord> propertyStore,
+ String tableNameWithType) {
+ String parentPath =
constructPropertyStorePathForResource(tableNameWithType);
+ List<ZNRecord> znRecords = propertyStore
+ .getChildren(parentPath, null, AccessOption.PERSISTENT,
CommonConstants.Helix.ZkClient.RETRY_COUNT,
+ CommonConstants.Helix.ZkClient.RETRY_INTERVAL_MS);
if (znRecords != null) {
int numZNRecords = znRecords.size();
- List<OfflineSegmentZKMetadata> offlineSegmentZKMetadataList = new
ArrayList<>(numZNRecords);
+ List<SegmentZKMetadata> segmentsZKMetadata = 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));
+ segmentsZKMetadata.add(new SegmentZKMetadata(znRecord));
}
}
- int numNullZNRecords = numZNRecords -
offlineSegmentZKMetadataList.size();
+ int numNullZNRecords = numZNRecords - segmentsZKMetadata.size();
if (numNullZNRecords > 0) {
- LOGGER.warn("Failed to read {}/{} offline segment ZK metadata under
path: {}", numZNRecords - numNullZNRecords,
+ LOGGER.warn("Failed to read {}/{} segment ZK metadata under path: {}",
numZNRecords - numNullZNRecords,
Review comment:
per the log msg, it feels like the first {} should be filled with
'numNullZnRecord'?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotRealtimeSegmentManager.java
##########
@@ -201,15 +201,13 @@ private synchronized void
assignRealtimeSegmentsToServerInstancesIfNecessary() {
for (String partition : state.getPartitionSet()) {
// Helix partition is the segment name
if (SegmentName.isHighLevelConsumerSegmentName(partition)) {
- HLCSegmentName segName = new HLCSegmentName(partition);
- RealtimeSegmentZKMetadata realtimeSegmentZKMetadata =
ZKMetadataProvider
-
.getRealtimeSegmentZKMetadata(_pinotHelixResourceManager.getPropertyStore(),
segName.getTableName(),
- partition);
- if (realtimeSegmentZKMetadata == null) {
+ SegmentZKMetadata segmentZKMetadata = ZKMetadataProvider
Review comment:
looks like segName was not used in the old code. but should it be used
actually? segName vs. partition in the getSegmentZKMetadata() call
##########
File path:
pinot-common/src/test/java/org/apache/pinot/common/tier/TierSegmentSelectorTest.java
##########
@@ -46,8 +45,8 @@ public void testTimeBasedSegmentSelector() {
// offline segment
String segmentName = "segment_0";
String tableNameWithType = "myTable_OFFLINE";
- OfflineSegmentZKMetadata offlineSegmentZKMetadata = new
OfflineSegmentZKMetadata();
- offlineSegmentZKMetadata.setSegmentName(segmentName);
+ SegmentZKMetadata offlineSegmentZKMetadata = new
SegmentZKMetadata(segmentName);
+
offlineSegmentZKMetadata.setSegmentType(CommonConstants.Segment.SegmentType.OFFLINE);
Review comment:
I didn't see setSegmentType() was called in other places above.
Is type required? If so, can it be added to constructor?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java
##########
@@ -36,209 +34,291 @@
import org.slf4j.LoggerFactory;
-public abstract class SegmentZKMetadata implements ZKMetadata {
+public class SegmentZKMetadata implements ZKMetadata {
private static final Logger LOGGER =
LoggerFactory.getLogger(SegmentZKMetadata.class);
+ private static final String NULL = "null";
- protected static final String NULL = "null";
-
- private String _segmentName;
- private SegmentType _segmentType;
- private long _startTime = -1;
- private long _endTime = -1;
- private TimeUnit _timeUnit;
- private String _indexVersion;
- private long _totalDocs = -1;
- private long _crc = -1;
- private long _creationTime = -1;
- private SegmentPartitionMetadata _partitionMetadata;
- private long _segmentUploadStartTime = -1;
- private String _crypterName;
- private Map<String, String> _customMap;
+ private final ZNRecord _znRecord;
+ private Map<String, String> _simpleFields;
- @Deprecated
- private String _rawTableName;
-
- public SegmentZKMetadata() {
+ public SegmentZKMetadata(String segmentName) {
+ _znRecord = new ZNRecord(segmentName);
+ _simpleFields = _znRecord.getSimpleFields();
+ // TODO: Remove this field after releasing 0.9.0
Review comment:
would be helpful to also comment the reason to leave this field till
0.9.0
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
