feat: move setup of the sink to plan builders #3360

rodesai · 2019-09-16T18:32:52Z

Description

This patch moves stream and table sinks (SchemaKX.into) into execution
plan builders. This change also moves computation of the rowkey/rowtime
indexes to be excluded from the output into the plan builder.

Testing done

Added unit tests for step builders.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

big-andy-coates

Thanks @rodesai

Few nits, but my main concern is the incorrect use of generics in this new code. I think we need to think about how we want the generics on these new types, and on ExecutionStep to work.

I hope you agree we shouldn't be committing new code with incorrect use of generics and raw types.

Happy to jump on a call and talk it through with you if you want.

big-andy-coates · 2019-09-17T11:01:56Z

ksql-execution/src/main/java/io/confluent/ksql/execution/builder/KsqlQueryBuilder.java

@@ -135,6 +136,28 @@ public KsqlQueryBuilder withKsqlConfig(final KsqlConfig newConfig) {
        .push(context);
  }

+  @SuppressWarnings("unchecked")
+  public KeySerde<Object> buildKeySerde(


As per comment in other PR the generics on this are wrong.

#3361 (comment)

big-andy-coates · 2019-09-17T11:08:17Z

ksql-streams/src/main/java/io/confluent/ksql/execution/streams/StreamSinkBuilder.java

+  }
+
+  public static void build(
+      final KStream<Object, GenericRow> kstream,


wrong generics: kstream would be either KStream<Windowed<Struct>, GenericRow> or KStream<Struct, GenericRow>. Neither of which can be cast to KStream<Object, GenericRow>.

Maybe this method should be something like:

public static <K> void build( final KStream<K, GenericRow> kstream, final StreamSink<KStream<K, GenericRow>> streamSink, final KsqlQueryBuilder queryBuilder) {

???

big-andy-coates · 2019-09-17T11:09:03Z

ksql-streams/src/main/java/io/confluent/ksql/execution/streams/TableSinkBuilder.java

+  private TableSinkBuilder() {
+  }
+
+  public static void build(


As above, wrong generics.

big-andy-coates · 2019-09-17T11:09:30Z

ksql-streams/src/test/java/io/confluent/ksql/execution/streams/StreamSinkBuilderTest.java

+  @Mock
+  private KsqlQueryBuilder queryBuilder;
+  @Mock
+  private KStream stream;


Test using raw types - can we please use generic types.

big-andy-coates · 2019-09-17T11:09:38Z

ksql-streams/src/test/java/io/confluent/ksql/execution/streams/TableSinkBuilderTest.java

+  @Mock
+  private KsqlQueryBuilder queryBuilder;
+  @Mock
+  private KTable table;


Test using raw types - can we please use generic types.

rodesai · 2019-09-19T04:48:29Z

ksql-engine/src/main/java/io/confluent/ksql/structured/SchemaKStream.java

+    StreamSinkBuilder.build(
+        kstream,
+        step,
+        (fmt, schema, ctx) -> keySerde,


Once we build the streams app from a visitor, we'll actually get a factory from the source node. For now, we can just pass in the serde we have.

I actually think that the Logical model shouldn't know anything about serializers. The source node should know about schemas, format and serde options only. The physical builder should be able to convert these to a serde, or a serde factory.

@big-andy-coates that's what I mean. By source node I mean the "source execution step of this step". So the physical builder will do something like:

class KStreamAndKeySerdeFactory<K> { KStream<K, GenericRow> kstream; KeySerdeFactory<K> factory; } KStreamAndKeySerdeFactory<K> visitStreamSink(final StreamSink<K> streamSink) { final KStreamAndKeySerdeFactory<K> k = streamSink.getSource().accept(this); StreamSinkBuilder.build(streamSink.kstream, streamSink, streamSink.factory, ...); ... }

big-andy-coates

Thanks @rodesai looks much better!

big-andy-coates · 2019-09-19T16:05:32Z

ksql-streams/src/main/java/io/confluent/ksql/execution/streams/TableSinkBuilder.java

+                return null;
+              }
+              final List<Object> columns = new ArrayList<>();
+              for (int i = 0; i < row.getColumns().size(); i++) {


No pertaining to this PR directly, but sometimes rowkeyIndexes is empty. In such a situation we don't actually need the map step. Removing this would obviously change out topology, which is a pain, but maybe we could do:

mapValues(row -> { if (row == null || rowkeyIndexes.isEmpty()) { return row; } ... }

To avoid the unnecessary copy of the row into another list? Just a small optimisation.

big-andy-coates · 2019-09-19T16:06:01Z

ksql-streams/src/test/java/io/confluent/ksql/execution/streams/StreamSinkBuilderTest.java

+  @Captor
+  private ArgumentCaptor<ValueMapper<GenericRow, GenericRow>> mapperCaptor;
+
+  private final QueryContext queryContext =


Can we not use a mock context?

big-andy-coates · 2019-09-19T16:06:41Z

ksql-streams/src/test/java/io/confluent/ksql/execution/streams/StreamSinkBuilderTest.java

+  private StreamSink<Struct> sink;
+
+  @Rule
+  public final MockitoRule mockitoRule = MockitoJUnit.rule();


Use of MockitoRule triggers spotbugs errors for me locally. Can we use @RunWith(MockitoJUnitRunner.class) instead please?

big-andy-coates · 2019-09-19T16:07:32Z

ksql-streams/src/test/java/io/confluent/ksql/execution/streams/TableSinkBuilderTest.java

+  private TableSink<Struct> sink;
+
+  @Rule
+  public final MockitoRule mockitoRule = MockitoJUnit.rule();


This patch moves stream and table sinks (SchemaKX.into) into execution plan builders. This change also moves computation of the rowkey/rowtime indexes to be excluded from the output into the plan builder.

rodesai requested a review from a team as a code owner September 16, 2019 18:32

big-andy-coates suggested changes Sep 17, 2019

View reviewed changes

rodesai force-pushed the move-sink-to-plan-builder branch from 26c94e3 to 0729c81 Compare September 18, 2019 08:24

rodesai requested a review from big-andy-coates September 18, 2019 08:32

rodesai force-pushed the move-sink-to-plan-builder branch 2 times, most recently from 54af8fa to 5d1a002 Compare September 19, 2019 04:46

rodesai commented Sep 19, 2019

View reviewed changes

rodesai force-pushed the move-sink-to-plan-builder branch from 5d1a002 to 20a0905 Compare September 19, 2019 05:15

big-andy-coates requested a review from a team September 19, 2019 15:55

big-andy-coates approved these changes Sep 19, 2019

View reviewed changes

rodesai added 2 commits September 19, 2019 12:43

feat: move setup of the sink to plan builders

0e09cd5

This patch moves stream and table sinks (SchemaKX.into) into execution plan builders. This change also moves computation of the rowkey/rowtime indexes to be excluded from the output into the plan builder.

feedback

c777323

rodesai force-pushed the move-sink-to-plan-builder branch from 20a0905 to c777323 Compare September 19, 2019 19:49

checkstyle

cc60b19

rodesai merged commit bfbdc20 into confluentinc:master Sep 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move setup of the sink to plan builders #3360

feat: move setup of the sink to plan builders #3360

rodesai commented Sep 16, 2019

big-andy-coates left a comment

big-andy-coates Sep 17, 2019

big-andy-coates Sep 17, 2019

big-andy-coates Sep 17, 2019

big-andy-coates Sep 17, 2019

big-andy-coates Sep 17, 2019

rodesai Sep 19, 2019

big-andy-coates Sep 19, 2019

rodesai Sep 19, 2019 •

edited

Loading

big-andy-coates left a comment

big-andy-coates Sep 19, 2019

big-andy-coates Sep 19, 2019

big-andy-coates Sep 19, 2019

big-andy-coates Sep 19, 2019

feat: move setup of the sink to plan builders #3360

feat: move setup of the sink to plan builders #3360

Conversation

rodesai commented Sep 16, 2019

Description

Testing done

Reviewer checklist

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai Sep 19, 2019 •

edited

Loading