Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14941 )
Change subject: IMPALA-4192: Move static state from DataSink into a DataSinkConfig ...................................................................... Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h File be/src/exec/data-sink.h: http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h@51 PS1, Line 51: class DataSinkConfig { > nit: unnecessary blank line Done http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h@66 PS1, Line 66: const RowDescriptor* input_row_desc_ = nullptr; > nit: is this always initialised to non-null in Init()? yup. Added "Set in Init()" to the comments, let me know if you want it to be more descriptive. http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h@67 PS1, Line 67: > Can you add that this is owned by FragmentInstanceState? Done http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h@71 PS1, Line 71: > nit: is this always initialised to non-null in Init()? (same as above) http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc File be/src/exec/data-sink.cc: http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc@55 PS1, Line 55: = state->ob > This is already stored in tsink_. Done http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc@56 PS1, Line 56: DataSinkConfig* data_sink = nullptr; > This is not used. Done http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc@60 PS1, Line 60: ODO: figure out > I would prefer to create DataSinkConfig sub-classes for HBase + Kudu + othe Done http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h@45 PS1, Line 45: Since it is expected to only be created : /// and used by PartitionedHashJoinPlanNode only, the DataSinkConfig::Init() and : /// DataSinkConfig::CreateSink() are not implemented for it. > This inheritance doesn't seem too intuitive, we only reuse input_row_desc_ Yes, IMPALA-4224 would eventually add a way of creating an instance of this sink for the fragment instance which is independent of the join node and would be achieved thorough the regular CreateSink and CreatConfig methods defined in data-sink. http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h@48 PS1, Line 48: class PhjBuilderConfig : public DataSinkConfig { > nit: blank line Done http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h@51 PS1, Line 51: const TPlanFragmentInstanceCtx& fragment_instance_ctx, > nit: blank line Done http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h@53 PS1, Line 53: > nit: the virtual isn't really needed along with the override. Not a big dea Done http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/runtime/fragment-instance-state.cc@220 PS1, Line 220: thrift_sink, plan_tree_->row_descriptor_, runtime_state_, &sink_config_)); > Any reason not to pass in sink_config_ directly? that was because it needed to call init on the new config object and sink_config_ is a pointer to a const, but after reading your comment it made me rethink and i think it will be better to just call init on a non const pointer inside Init() then assign it to the input pointer(which will be a pointer to a const). Done. http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/runtime/krpc-data-stream-sender.h File be/src/runtime/krpc-data-stream-sender.h: http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/runtime/krpc-data-stream-sender.h@44 PS1, Line 44: DataSink* CreateSink(const TPlanFragmentCtx& fragment_ctx, > same comment as before about the virtual/override redundancy. Done -- To view, visit http://gerrit.cloudera.org:8080/14941 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2 Gerrit-Change-Number: 14941 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 08 Jan 2020 01:56:40 +0000 Gerrit-HasComments: Yes
