Jackie-Jiang commented on code in PR #11650:
URL: https://github.com/apache/pinot/pull/11650#discussion_r1343021730
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-common/src/main/java/org/apache/pinot/plugin/ingestion/batch/common/SegmentGenerationTaskRunner.java:
##########
@@ -65,6 +65,7 @@ public class SegmentGenerationTaskRunner implements
Serializable {
public static final String DEPRECATED_USE_LOCAL_DIRECTORY_SEQUENCE_ID =
"local.directory.sequence.id";
public static final String USE_GLOBAL_DIRECTORY_SEQUENCE_ID =
"use.global.directory.sequence.id";
public static final String APPEND_UUID_TO_SEGMENT_NAME =
"append.uuid.to.segment.name";
+ public static final String EXCLUDE_TIMESTAMPS_IN_SEGMENT_NAME =
"exclude.timestamps.in.segment.name";
Review Comment:
I'd suggest calling it time instead of timestamp because it might not follow
timestamp format
```suggestion
public static final String EXCLUDE_TIME_IN_SEGMENT_NAME =
"exclude.time.in.segment.name";
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/BatchConfig.java:
##########
@@ -190,6 +193,10 @@ public boolean isAppendUUIDToSegmentName() {
return _appendUUIDToSegmentName;
}
+ public boolean isExcludeTimestampsFromSegmentName() {
Review Comment:
Make this consistent with config name
```suggestion
public boolean isExcludeTimeInSegmentName() {
```
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/SimpleSegmentNameGenerator.java:
##########
@@ -66,8 +68,9 @@ public String generateSegmentName(int sequenceId, @Nullable
Object minTimeValue,
SegmentNameUtils.validatePartialOrFullSegmentName(maxTimeValue.toString());
}
- return JOINER.join(_segmentNamePrefix, minTimeValue, maxTimeValue,
_segmentNamePostfix,
- sequenceId >= 0 ? sequenceId : null, _appendUUIDToSegmentName ?
UUID.randomUUID() : null);
+ return JOINER.join(_segmentNamePrefix, _excludeTimestampsInSegmentName ?
null : minTimeValue,
Review Comment:
We should put the check before performing the time value check
```suggestion
if (_excludeTimeInSegmentName) {
JOINER.join(_segmentNamePrefix, ...);
} else {
if (minTimeValue != null) {
...
}
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/BatchConfigProperties.java:
##########
@@ -62,6 +62,7 @@ private BatchConfigProperties() {
public static final String FAIL_ON_EMPTY_SEGMENT = "fail.on.empty.segment";
public static final String AUTH_TOKEN = "authToken";
public static final String APPEND_UUID_TO_SEGMENT_NAME =
"append.uuid.to.segment.name";
+ public static final String EXCLUDE_TIMESTAMPS_IN_SEGMENT_NAME =
"exclude.timestamps.in.segment.name";
Review Comment:
```suggestion
public static final String EXCLUDE_TIME_IN_SEGMENT_NAME =
"exclude.time.in.segment.name";
```
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-common/src/main/java/org/apache/pinot/plugin/ingestion/batch/common/SegmentGenerationTaskRunner.java:
##########
@@ -134,13 +135,15 @@ private SegmentNameGenerator
getSegmentNameGenerator(SegmentGeneratorConfig segm
boolean appendUUIDToSegmentName =
Boolean.parseBoolean(segmentNameGeneratorConfigs.get(APPEND_UUID_TO_SEGMENT_NAME));
+ boolean excludeTimestampsInSegmentName =
Review Comment:
This can be applied to `NormalizedDateSegmentNameGenerator` as well, so I'd
suggest adding it to avoid confusion
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-common/src/main/java/org/apache/pinot/plugin/ingestion/batch/common/SegmentGenerationTaskRunner.java:
##########
@@ -65,6 +65,7 @@ public class SegmentGenerationTaskRunner implements
Serializable {
public static final String DEPRECATED_USE_LOCAL_DIRECTORY_SEQUENCE_ID =
"local.directory.sequence.id";
public static final String USE_GLOBAL_DIRECTORY_SEQUENCE_ID =
"use.global.directory.sequence.id";
public static final String APPEND_UUID_TO_SEGMENT_NAME =
"append.uuid.to.segment.name";
+ public static final String EXCLUDE_TIMESTAMPS_IN_SEGMENT_NAME =
"exclude.timestamps.in.segment.name";
Review Comment:
Actually we should directly use the constant within `BatchConfigProperties`
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
