Jackie-Jiang commented on code in PR #12681:
URL: https://github.com/apache/pinot/pull/12681#discussion_r1533284146
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/segment/FlushThresholdUpdateManager.java:
##########
@@ -33,16 +33,20 @@ public class FlushThresholdUpdateManager {
* Check table config for flush size.
*
* If flush size > 0, create a new DefaultFlushThresholdUpdater with given
flush size.
- * If flush size <= 0, create new SegmentSizeBasedFlushThresholdUpdater if
not already created. Create only 1 per
- * table because we want to maintain tuning information for the table in the
updater.
+ * If flush size <= 0, create new SegmentRowsBasedFlushThresholdUpdater if
flushThresholdSegmentRows > 0.
+ * If flush size <= 0 AND segment.row <=0, create new
SegmentSizeBasedFlushThresholdUpdater if not already created.
+ * Create only 1 per table because we want to maintain tuning information
for the table in the updater.
*/
public FlushThresholdUpdater getFlushThresholdUpdater(StreamConfig
streamConfig) {
String realtimeTableName = streamConfig.getTableNameWithType();
int flushThresholdRows = streamConfig.getFlushThresholdRows();
+ int flushThresholdSegmentRows =
streamConfig.getFlushThresholdSegmentRows();
if (flushThresholdRows > 0) {
_flushThresholdUpdaterMap.remove(realtimeTableName);
return new DefaultFlushThresholdUpdater(flushThresholdRows);
+ } else if (flushThresholdSegmentRows > 0) {
+ return new
SegmentRowsBasedFlushThresholdUpdater(flushThresholdSegmentRows);
Review Comment:
Suggest renaming it to `FixedFlushThresholdUpdater`. You also need to call
`_flushThresholdUpdaterMap.remove(realtimeTableName);`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/segment/FlushThresholdUpdateManager.java:
##########
@@ -33,16 +33,20 @@ public class FlushThresholdUpdateManager {
* Check table config for flush size.
*
* If flush size > 0, create a new DefaultFlushThresholdUpdater with given
flush size.
- * If flush size <= 0, create new SegmentSizeBasedFlushThresholdUpdater if
not already created. Create only 1 per
- * table because we want to maintain tuning information for the table in the
updater.
+ * If flush size <= 0, create new SegmentRowsBasedFlushThresholdUpdater if
flushThresholdSegmentRows > 0.
Review Comment:
We need to revise the javadoc here. Currently it is not clear.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfigProperties.java:
##########
@@ -81,6 +81,7 @@ private StreamConfigProperties() {
*/
public static final String DEPRECATED_SEGMENT_FLUSH_THRESHOLD_ROWS =
"realtime.segment.flush.threshold.size";
public static final String SEGMENT_FLUSH_THRESHOLD_ROWS =
"realtime.segment.flush.threshold.rows";
+ public static final String SEGMENT_FLUSH_THRESHOLD_SEGMENT_ROWS =
"realtime.segment.flush.threshold.segment.rows";
Review Comment:
Please add some javadoc explaining the difference between this and
`SEGMENT_FLUSH_THRESHOLD_ROWS`
--
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]
