Jackie-Jiang commented on code in PR #11869:
URL: https://github.com/apache/pinot/pull/11869#discussion_r1814211853
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -116,7 +117,7 @@ public static String
getBatchSegmentIngestionType(TableConfig tableConfig) {
if (segmentIngestionType == null) {
segmentIngestionType =
tableConfig.getValidationConfig().getSegmentPushType();
}
- return segmentIngestionType;
+ return (segmentIngestionType == null) ? DEFAULT_SEGMENT_INGESTION_TYPE :
segmentIngestionType;
Review Comment:
Throwing exception could potentially break existing use cases.
The push type should be either APPEND or REFRESH, and there are other places
where we check whether the push type is REFRESH, e.g.
https://github.com/apache/pinot/blob/master/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/ConsistentDataPushUtils.java#L221.
We shouldn't allow empty push type which doesn't match APPEND or REFRESH.
Since we are already making it default to `APPEND`, and this has been merged
for 1 year, I guess we need to follow this going forward. I'm labelling this PR
backward incompatible. Majority of the use cases should be `APPEND` when the
time column is configured.
Sorry for causing this incident. Will also update the documentation about
`APPEND` being the default push type
--
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
