Copilot commented on code in PR #18931:
URL: https://github.com/apache/pinot/pull/18931#discussion_r3540234821
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfigProperties.java:
##########
@@ -58,15 +58,10 @@ private StreamConfigProperties() {
* @deprecated because the property key is confusing (says size but is
actually rows). Use
* {@link StreamConfigProperties#SEGMENT_FLUSH_THRESHOLD_ROWS}
*
- * Row count flush threshold for realtime segments. This behaves in a
similar way for HLC and LLC. For HLC,
- * since there is only one consumer per server, this size is used as the
size of the consumption buffer and
- * determines after how many rows we flush to disk. For example, if this
threshold is set to two million rows,
- * then a high level consumer would have a buffer size of two million.
- *
- * For LLC, this size is divided across all the segments assigned to a given
server and is set on a per segment
- * basis. Assuming a low level consumer server is assigned four stream
partitions to consume from and a flush
- * size of two million, then each consuming segment would have a flush size
of five hundred thousand rows, for a
- * total of two million rows in memory.
+ * Row count flush threshold for realtime segments. This size is divided
across all the segments assigned to a given
+ * server and is set on a per segment basis. Assuming a server is assigned
four stream partitions to consume from and
+ * a flush size of two million, then each consuming segment would have a
flush size of five hundred thousand rows, for
+ * a total of two million rows in memory.
Review Comment:
This Javadoc line exceeds the repo’s 120-character checkstyle limit
(config/checkstyle.xml sets LineLength max=120), which will fail checkstyle in
CI. Please wrap the sentence across multiple lines.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConsumerFactory.java:
##########
@@ -62,30 +61,24 @@ public StreamPartitionMsgOffsetFactory
createStreamMsgOffsetFactory() {
return new LongMsgOffsetFactory();
}
- /**
- * Creates a partition group consumer, which can fetch messages from a
partition group
- */
- public PartitionGroupConsumer createPartitionGroupConsumer(String clientId,
- PartitionGroupConsumptionStatus partitionGroupConsumptionStatus) {
- return createPartitionLevelConsumer(clientId,
partitionGroupConsumptionStatus.getStreamPartitionGroupId());
- }
+ /// Creates a [PartitionGroupConsumer] that fetches messages from the
partition group described by
+ /// `partitionGroupConsumptionStatus`.
+ ///
+ /// Every [StreamConsumerFactory] implementation must override this method.
The returned consumer is owned by
+ /// the caller, which is responsible for closing it via
[PartitionGroupConsumer#close()]. Each invocation returns
+ /// a new consumer instance; implementations are not required to make the
returned consumer thread-safe.
+ ///
+ /// @param clientId identifies the creator of this consumer
+ /// @param partitionGroupConsumptionStatus the partition group to consume
and the offsets to start from
+ /// @return a new, non-null partition group consumer
+ public abstract PartitionGroupConsumer createPartitionGroupConsumer(String
clientId,
+ PartitionGroupConsumptionStatus partitionGroupConsumptionStatus);
Review Comment:
Making createPartitionGroupConsumer(...) abstract forces pre-built
out-of-tree StreamConsumerFactory implementations to fail with
AbstractMethodError at runtime. Consider keeping a concrete default that throws
UnsupportedOperationException with a clear migration message instead; this
preserves a clearer failure mode without affecting in-tree factories (they
already override the method).
--
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]
