Jackie-Jiang commented on code in PR #17264:
URL: https://github.com/apache/pinot/pull/17264#discussion_r2594275755
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java:
##########
@@ -675,19 +682,44 @@ private void convertFormatIfNecessary(File
segmentDirectory)
converter.convert(segmentDirectory);
}
+ private boolean hasAnyColumnWithForwardIndexDisabled() {
+ Map<String, FieldIndexConfigs> indexConfigsMap =
_config.getIndexConfigsByColName();
Review Comment:
Iterating over this map itself should be good enough
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/CrcUtils.java:
##########
@@ -38,47 +39,78 @@ public class CrcUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(CrcUtils.class);
private static final int BUFFER_SIZE = 65536;
private static final String CRC_FILE_EXTENSTION = ".crc";
+ private static final List<String> DATA_FILE_EXTENSIONS =
Arrays.asList(".fwd", ".dict");
private final List<File> _files;
+ private final List<File> _dataFiles;
- private CrcUtils(List<File> files) {
+ private CrcUtils(List<File> files, List<File> dataFiles) {
Review Comment:
Let's use this opportunity to make this a real util class, i.e. providing 2
static methods: `computeCrc(File indexDir)` and `computeDataCrc(File indexDir)`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -192,6 +193,13 @@ private void loadCreationMeta(File crcFile)
final DataInputStream ds = new DataInputStream(new
FileInputStream(crcFile));
_crc = ds.readLong();
_creationTime = ds.readLong();
+ try {
+ _dataCrc = ds.readLong();
+ } catch (IOException e) {
+ LOGGER.debug("Could not find data crc, falling back to default
LONG_MIN value");
+ } finally {
+ ds.close();
Review Comment:
No need to close it here. We are not throwing the exception.
A better approach would be to trap the `DataInputStream` construct into a
try-with-resource block to ensure it is always closed
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2138,9 +2139,14 @@ private void handleLLCUpload(String segmentName, String
rawTableName, SegmentZKM
throws Exception {
long currentCrc = currentMetadata.getCrc();
long newCrc = response.getCrc();
+ long newDataCrc = response.getDataCrc();
if (currentCrc != newCrc) {
Review Comment:
What if only data crc mismatch (very unlikely, but for defensive coding we
should check both)
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java:
##########
@@ -170,10 +170,18 @@ public long getCrc() {
return _znRecord.getLongField(Segment.CRC, -1);
}
+ public long getDataCrc() {
+ return _znRecord.getLongField(Segment.DATA_CRC, -1);
+ }
+
Review Comment:
(minor) Keep the getter and setter for the same field together (i.e. move
this below `setCrc()`)
--
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]
