You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/04/21 20:40:02 UTC
[GitHub] [druid] clintropolis commented on a change in pull request #9734: Lazy initialization of SettableByteEntityReader to avoid NPE
clintropolis commented on a change in pull request #9734:
URL: https://github.com/apache/druid/pull/9734#discussion_r412472637
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/StreamChunkParser.java
##########
@@ -38,20 +45,38 @@
{
@Nullable
private final InputRowParser<ByteBuffer> parser;
- private final SettableByteEntityReader byteEntityReader;
+ private final Supplier<SettableByteEntityReader> lazyByteEntityReaderSupplier; // lazy initializer
- StreamChunkParser(@Nullable InputRowParser<ByteBuffer> parser, SettableByteEntityReader byteEntityReader)
+ /**
+ * Either parser or inputFormat shouldn't be null.
+ */
+ StreamChunkParser(
+ @Nullable InputRowParser<ByteBuffer> parser,
+ @Nullable InputFormat inputFormat,
+ InputRowSchema inputRowSchema,
+ TransformSpec transformSpec,
+ File indexingTmpDir
+ )
{
+ if (parser == null && inputFormat == null) {
+ throw new IAE("Either parser or inputFormat shouldn't be set");
Review comment:
nit: this error message should be "Either parser or inputFormat **should** be set" maybe since this is checking if both are null?
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/StreamChunkParser.java
##########
@@ -38,20 +45,38 @@
{
@Nullable
private final InputRowParser<ByteBuffer> parser;
- private final SettableByteEntityReader byteEntityReader;
+ private final Supplier<SettableByteEntityReader> lazyByteEntityReaderSupplier; // lazy initializer
- StreamChunkParser(@Nullable InputRowParser<ByteBuffer> parser, SettableByteEntityReader byteEntityReader)
+ /**
+ * Either parser or inputFormat shouldn't be null.
+ */
+ StreamChunkParser(
+ @Nullable InputRowParser<ByteBuffer> parser,
+ @Nullable InputFormat inputFormat,
+ InputRowSchema inputRowSchema,
+ TransformSpec transformSpec,
+ File indexingTmpDir
+ )
{
+ if (parser == null && inputFormat == null) {
+ throw new IAE("Either parser or inputFormat shouldn't be set");
+ }
this.parser = parser;
- this.byteEntityReader = byteEntityReader;
+ // Create a lazy initializer since it will fail to create a SettableByteEntityReader if inputFormat is null
+ this.lazyByteEntityReaderSupplier = Suppliers.memoize(() -> new SettableByteEntityReader(
+ inputFormat,
+ inputRowSchema,
+ transformSpec,
+ indexingTmpDir
+ ));
}
List<InputRow> parse(List<byte[]> streamChunk) throws IOException
{
if (parser != null) {
return parseWithParser(parser, streamChunk);
} else {
- return parseWithInputFormat(byteEntityReader, streamChunk);
+ return parseWithInputFormat(lazyByteEntityReaderSupplier.get(), streamChunk);
Review comment:
Would it be better to prefer the inputFormat if it exists?
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/StreamChunkParser.java
##########
@@ -38,20 +45,38 @@
{
@Nullable
private final InputRowParser<ByteBuffer> parser;
- private final SettableByteEntityReader byteEntityReader;
+ private final Supplier<SettableByteEntityReader> lazyByteEntityReaderSupplier; // lazy initializer
- StreamChunkParser(@Nullable InputRowParser<ByteBuffer> parser, SettableByteEntityReader byteEntityReader)
+ /**
+ * Either parser or inputFormat shouldn't be null.
+ */
+ StreamChunkParser(
+ @Nullable InputRowParser<ByteBuffer> parser,
+ @Nullable InputFormat inputFormat,
+ InputRowSchema inputRowSchema,
+ TransformSpec transformSpec,
+ File indexingTmpDir
+ )
{
+ if (parser == null && inputFormat == null) {
+ throw new IAE("Either parser or inputFormat shouldn't be set");
+ }
this.parser = parser;
- this.byteEntityReader = byteEntityReader;
+ // Create a lazy initializer since it will fail to create a SettableByteEntityReader if inputFormat is null
+ this.lazyByteEntityReaderSupplier = Suppliers.memoize(() -> new SettableByteEntityReader(
Review comment:
nit: Why a supplier instead of just making `byteEntityReader` `@Nullable`? it seems like it would not be used if `parser` is not null.
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/StreamChunkParser.java
##########
@@ -38,20 +45,38 @@
{
@Nullable
private final InputRowParser<ByteBuffer> parser;
- private final SettableByteEntityReader byteEntityReader;
+ private final Supplier<SettableByteEntityReader> lazyByteEntityReaderSupplier; // lazy initializer
- StreamChunkParser(@Nullable InputRowParser<ByteBuffer> parser, SettableByteEntityReader byteEntityReader)
+ /**
+ * Either parser or inputFormat shouldn't be null.
+ */
+ StreamChunkParser(
+ @Nullable InputRowParser<ByteBuffer> parser,
+ @Nullable InputFormat inputFormat,
+ InputRowSchema inputRowSchema,
+ TransformSpec transformSpec,
+ File indexingTmpDir
+ )
{
+ if (parser == null && inputFormat == null) {
+ throw new IAE("Either parser or inputFormat shouldn't be set");
+ }
this.parser = parser;
- this.byteEntityReader = byteEntityReader;
+ // Create a lazy initializer since it will fail to create a SettableByteEntityReader if inputFormat is null
+ this.lazyByteEntityReaderSupplier = Suppliers.memoize(() -> new SettableByteEntityReader(
+ inputFormat,
+ inputRowSchema,
+ transformSpec,
+ indexingTmpDir
+ ));
}
List<InputRow> parse(List<byte[]> streamChunk) throws IOException
{
if (parser != null) {
Review comment:
I know this isn't new, but why do we prefer the parser over the byteEntityReader if both are set?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org