navina commented on code in PR #9224:
URL: https://github.com/apache/pinot/pull/9224#discussion_r979772662
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamDataDecoderResult.java:
##########
@@ -16,27 +16,32 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.pinot.plugin.stream.kafka20;
+package org.apache.pinot.spi.stream;
-import java.nio.ByteBuffer;
-import org.apache.pinot.plugin.stream.kafka.MessageAndOffset;
-import org.apache.pinot.spi.stream.RowMetadata;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.data.readers.GenericRow;
-public class MessageAndOffsetAndMetadata extends MessageAndOffset {
- private final RowMetadata _rowMetadata;
+/**
+ * A container class for holding the result of a decoder
+ * At any point in time, only one of Result or exception is set as null.
+ */
+public final class StreamDataDecoderResult {
+ private final GenericRow _result;
+ private final Exception _exception;
Review Comment:
> You have an exception declared here. I see that the decode() operation is
guaranteed not to throw an exception. You may also want to add a comment on the
decode() interface that it is NOT supposed to throw an exception (just to guard
against someone changing it).
Added
> Also, instead of a generic exception, I suggest that we define some
specific ones (or at least specific categories) -- retryable or not. See PR
https://github.com/apache/pinot/pull/9051
I specifically don't want to add any "types" of exception. The reason is
that typically, when decoding fails, the caller either ignores the record or
aborts the process. It is very uncommon to try and "handle" decoding errors.
That's why I preferred encapsulating the exception into a class, instead of
throwing it. It is then, completely left to the caller to determine whether to
handle specific exceptions or not. This leaves the interface more flexible for
usage as well.
For #9051 and the like: The current `StreamDecoder` interface allows
returning `null` value, which seems to the main source of confusion. The
interface is defining how a specific caller should handle a `null` value when
there can be more than 1 caller. At the time of writing #9051, there was only
one caller for this method `LLRealtimeSegmentDataManager`. Now that we have one
more, `StreamDataDecoder`, it is prone to error.
I will ensure that the existing functionalities are not affected or find a
workaround. Thanks for pointing it out.
--
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
