klsince commented on a change in pull request #7255:
URL: https://github.com/apache/pinot/pull/7255#discussion_r685466546
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java
##########
@@ -136,14 +135,14 @@ static long
computeRealtimeTotalDocumentInSegments(List<RealtimeSegmentZKMetadat
groupId = segmentGroupIdName;
}
// Discard all segments with different groupids as they are replicas
- if (groupId.equals(segmentGroupIdName) &&
realtimeSegmentZKMetadata.getTotalDocs() >= 0) {
- numTotalDocs += realtimeSegmentZKMetadata.getTotalDocs();
+ if (groupId.equals(segmentGroupIdName) &&
segmentZKMetadata.getTotalDocs() >= 0) {
+ numTotalDocs += segmentZKMetadata.getTotalDocs();
}
}
} else {
Review comment:
is this else and the if-check below redundant?
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java
##########
@@ -90,7 +91,7 @@
static final long START_TIME_MS = CURRENT_TIME_MS -
TimeUnit.HOURS.toMillis(RANDOM.nextInt(24) + 24);
static final long END_TIME_MS = START_TIME_MS +
TimeUnit.HOURS.toMillis(RANDOM.nextInt(24) + 1);
static final Interval INTERVAL = new Interval(START_TIME_MS, END_TIME_MS);
- static final String CRC = Long.toString(RANDOM.nextLong());
+ static final String CRC = Long.toString(RANDOM.nextLong() & 0xFFFFFFFFL);
Review comment:
curious why `& 0xFFFFFFFFL` here?
##########
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 {
Review comment:
looks like changes in this file can be in its own PR to merge firstly?
as the interfaces are not changed much.
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java
##########
@@ -93,7 +92,8 @@
private static final long TIMEOUT_IN_MS = 10_000L;
@BeforeClass
- public void setUp() throws Exception {
+ public void setUp()
Review comment:
looks like the changes are mostly on formats. how about open a separate
RP to land those first?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -377,10 +377,9 @@ public void run() {
try {
_segmentLogger.info("Marking current segment as completed in
Helix");
- RealtimeSegmentZKMetadata metadataToOverwrite = new
RealtimeSegmentZKMetadata();
+ SegmentZKMetadata metadataToOverwrite = new
SegmentZKMetadata(segmentZKMetadata.getSegmentName());
metadataToOverwrite.setTableName(_tableNameWithType);
-
metadataToOverwrite.setSegmentName(realtimeSegmentZKMetadata.getSegmentName());
- metadataToOverwrite.setSegmentType(SegmentType.OFFLINE);
Review comment:
hmm.. is this a typo in the old code, or intentionally?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/util/TableRetentionValidator.java
##########
@@ -128,54 +128,55 @@ public void run()
try {
timeUnit = TimeUnit.valueOf(timeUnitString.toUpperCase());
} catch (Exception e) {
- LOGGER.error("Table: {}, invalid time unit: {}", tableName,
timeUnitString);
+ LOGGER.error("Table: {}, invalid time unit: {}", tableNameWithType,
timeUnitString);
continue;
}
// Get time duration in days
String timeValueString = retentionConfig.getRetentionTimeValue();
long durationInDays;
try {
- durationInDays = timeUnit.toDays(Long.valueOf(timeValueString));
+ durationInDays = timeUnit.toDays(Long.parseLong(timeValueString));
} catch (Exception e) {
- LOGGER.error("Table: {}, invalid time value: {}", tableName,
timeValueString);
+ LOGGER.error("Table: {}, invalid time value: {}", tableNameWithType,
timeValueString);
continue;
}
if (durationInDays <= 0) {
- LOGGER.error("Table: {}, invalid retention duration in days: {}",
tableName, durationInDays);
+ LOGGER.error("Table: {}, invalid retention duration in days: {}",
tableNameWithType, durationInDays);
continue;
}
if (durationInDays > _durationInDaysThreshold) {
- LOGGER.warn("Table: {}, retention duration in days is too large: {}",
tableName, durationInDays);
+ LOGGER.warn("Table: {}, retention duration in days is too large: {}",
tableNameWithType, durationInDays);
}
// Skip segments metadata check for realtime tables
- if (tableName.endsWith("REALTIME")) {
+ if (tableNameWithType.endsWith("REALTIME")) {
Review comment:
iirc, there is util method to check table type? then may avoid hard
coding "REALTIME" here.
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/SegmentCompletionTest.java
##########
@@ -64,7 +64,8 @@
private final LongMsgOffset s3Offset = new LongMsgOffset(30L);
@BeforeMethod
- public void testCaseSetup() throws Exception {
+ public void testCaseSetup()
Review comment:
changes in this file are also mainly on formats. The changes are so many
that github simply folds the file up. would suggest to open a quick RP to land
those format changes firstly.
--
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]
