You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2019/12/21 00:38:52 UTC

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig 

Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14941


Change subject: IMPALA-4192: Move static state from DataSink into a DataSinkConfig
......................................................................

IMPALA-4192: Move static state from DataSink into a DataSinkConfig

This patch adds a new class called DataSinkConfig which contains a
subset of the static state of their corresponding DataSink, of
which there is one instance per fragment. DataSink contains the
runtime state and there can be up to MT_DOP instances of it per
fragment.
Eventually all static state including codegened function pointers
would be moved to the PlanNodes.

Testing:
TODO: Run exhaustive tests successfully.

Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
---
M be/src/exec/blocking-plan-root-sink.cc
M be/src/exec/blocking-plan-root-sink.h
M be/src/exec/buffered-plan-root-sink.cc
M be/src/exec/buffered-plan-root-sink.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hbase-table-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
26 files changed, 447 insertions(+), 235 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14941/1
-- 
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: newchange
Gerrit-Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
Gerrit-Change-Number: 14941
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer 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:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/data-sink.h
File be/src/exec/data-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/data-sink.h@54
PS2, Line 54: ;
no ; needed (this seems to a problem for clang tidy too)


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/hbase-table-sink.h
File be/src/exec/hbase-table-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/hbase-table-sink.h@40
PS2, Line 40:   ~HBaseTableSinkConfig() {}
please add override


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/kudu-table-sink.h
File be/src/exec/kudu-table-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/kudu-table-sink.h@39
PS2, Line 39:   ~KuduTableSinkConfig() {}
please add override


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/nested-loop-join-builder.h
File be/src/exec/nested-loop-join-builder.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/nested-loop-join-builder.h@39
PS2, Line 39:   ~NljBuilderConfig() {}
please add override


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/partitioned-hash-join-builder.h
File be/src/exec/partitioned-hash-join-builder.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/partitioned-hash-join-builder.h@67
PS2, Line 67:   ~PhjBuilderConfig() {}
please add override


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/plan-root-sink.h
File be/src/exec/plan-root-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/plan-root-sink.h@36
PS2, Line 36:   ~PlanRootSinkConfig() {}
please add override


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/runtime/krpc-data-stream-sender.h
File be/src/runtime/krpc-data-stream-sender.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/runtime/krpc-data-stream-sender.h@52
PS2, Line 52:   ~KrpcDataStreamSenderConfig() {}
please add override



-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jan 2020 12:42:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jan 2020 01:56:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5394/ DRY_RUN=false


-- 
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: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2020 18:29:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 1:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/5327/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 21 Dec 2019 01:06:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 3:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/5379/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jan 2020 17:56:10 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5: Verified+1


-- 
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: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2020 23:02:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/data-sink.h
File be/src/exec/data-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/data-sink.h@54
PS2, Line 54: ;
> no ; needed (this seems to a problem for clang tidy too)
Done


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/hbase-table-sink.h
File be/src/exec/hbase-table-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/hbase-table-sink.h@40
PS2, Line 40:   ~HBaseTableSinkConfig() {}
> please add override
Done


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/kudu-table-sink.h
File be/src/exec/kudu-table-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/kudu-table-sink.h@39
PS2, Line 39:   ~KuduTableSinkConfig() {}
> please add override
Done


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/nested-loop-join-builder.h
File be/src/exec/nested-loop-join-builder.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/nested-loop-join-builder.h@39
PS2, Line 39:   ~NljBuilderConfig() {}
> please add override
Done


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/partitioned-hash-join-builder.h
File be/src/exec/partitioned-hash-join-builder.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/partitioned-hash-join-builder.h@67
PS2, Line 67:   ~PhjBuilderConfig() {}
> please add override
Done


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/plan-root-sink.h
File be/src/exec/plan-root-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/exec/plan-root-sink.h@36
PS2, Line 36:   ~PlanRootSinkConfig() {}
> please add override
Done


http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/runtime/krpc-data-stream-sender.h
File be/src/runtime/krpc-data-stream-sender.h:

http://gerrit.cloudera.org:8080/#/c/14941/2/be/src/runtime/krpc-data-stream-sender.h@52
PS2, Line 52:   ~KrpcDataStreamSenderConfig() {}
> please add override
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jan 2020 17:25:13 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/5375/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jan 2020 02:27:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 1: Code-Review+2

(9 comments)

I have a bunch of very minor nits, but LGTM

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: 
nit: unnecessary blank line


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h@66
PS1, Line 66:   /// Pointer to the thrift data sink struct associated with this sink.
nit: is this always initialised to non-null in Init()?


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h@71
PS1, Line 71:   const RowDescriptor* input_row_desc_ = nullptr;
nit: is this always initialised to non-null in Init()?


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@44
PS1, Line 44: /// Partitioned Hash Join Builder Config class. This has a few extra methods to be used
I'm gonna have fun rebasing my change onto this :)


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h@48
PS1, Line 48: 
nit: blank line


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h@51
PS1, Line 51: 
nit: blank line


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h@53
PS1, Line 53:   virtual Status Init(const TDataSink& tsink, const RowDescriptor* input_row_desc,
nit: the virtual isn't really needed along with the override. Not a big deal, the pattern is all over the codebase, but someone pointed out that it was redundant a while back.


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:   DataSinkConfig::CreateConfig(
Any reason not to pass in sink_config_ directly?


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:   virtual Status Init(const TDataSink& tsink, const RowDescriptor* input_row_desc,
same comment as before about the virtual/override redundancy.



-- 
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: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 22 Dec 2019 08:52:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14941

to look at the new patch set (#4).

Change subject: IMPALA-4192: Move static state from DataSink into a DataSinkConfig
......................................................................

IMPALA-4192: Move static state from DataSink into a DataSinkConfig

This patch adds a new class called DataSinkConfig which contains a
subset of the static state of their corresponding DataSink, of
which there is one instance per fragment. DataSink contains the
runtime state and there can be up to MT_DOP instances of it per
fragment.
Eventually all static state including codegened function pointers
would be moved to the PlanNodes.

Testing:
Ran exhaustive tests successfully.

Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
---
M be/src/exec/blocking-plan-root-sink.cc
M be/src/exec/blocking-plan-root-sink.h
M be/src/exec/buffered-plan-root-sink.cc
M be/src/exec/buffered-plan-root-sink.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hbase-table-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
27 files changed, 503 insertions(+), 253 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14941/4
-- 
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: newpatchset
Gerrit-Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
Gerrit-Change-Number: 14941
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer 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 1: Code-Review+1

(5 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@67
PS1, Line 67: tsink_
Can you add that this is owned by FragmentInstanceState?


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: thrift_sink
This is already stored in tsink_.


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc@56
PS1, Line 56:   const vector<TExpr>& thrift_output_exprs = thrift_sink.output_exprs;
This is not used.


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc@60
PS1, Line 60: thrift_sink.type
I would prefer to create DataSinkConfig sub-classes for HBase + Kudu + other sinks in this patch and move this logic to their CreateSink() overrides.

The reason is that these classes will be created in the future anyway (if we want to move more static code out of the DataSinks), and this mixed switch/override solution just makes things harder to understand.


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_ from DataSinkConfig if I saw correctly. Is this a temporary thing related to IMPALA-4224? If not, then I would prefer to add input_row_desc_ here and remove the inheritance.



-- 
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: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2020 15:25:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 4:

fixed clang-tidy errors


-- 
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: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2020 17:25:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14941

to look at the new patch set (#3).

Change subject: IMPALA-4192: Move static state from DataSink into a DataSinkConfig
......................................................................

IMPALA-4192: Move static state from DataSink into a DataSinkConfig

This patch adds a new class called DataSinkConfig which contains a
subset of the static state of their corresponding DataSink, of
which there is one instance per fragment. DataSink contains the
runtime state and there can be up to MT_DOP instances of it per
fragment.
Eventually all static state including codegened function pointers
would be moved to the PlanNodes.

Testing:
Ran exhaustive tests successfully.

Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
---
M be/src/exec/blocking-plan-root-sink.cc
M be/src/exec/blocking-plan-root-sink.h
M be/src/exec/buffered-plan-root-sink.cc
M be/src/exec/buffered-plan-root-sink.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hbase-table-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
27 files changed, 500 insertions(+), 244 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14941/3
-- 
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: newpatchset
Gerrit-Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
Gerrit-Change-Number: 14941
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14941 )

Change subject: IMPALA-4192: Move static state from DataSink into a DataSinkConfig
......................................................................

IMPALA-4192: Move static state from DataSink into a DataSinkConfig

This patch adds a new class called DataSinkConfig which contains a
subset of the static state of their corresponding DataSink, of
which there is one instance per fragment. DataSink contains the
runtime state and there can be up to MT_DOP instances of it per
fragment.
Eventually all static state including codegened function pointers
would be moved to the PlanNodes.

Testing:
Ran exhaustive tests successfully.

Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
Reviewed-on: http://gerrit.cloudera.org:8080/14941
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/blocking-plan-root-sink.cc
M be/src/exec/blocking-plan-root-sink.h
M be/src/exec/buffered-plan-root-sink.cc
M be/src/exec/buffered-plan-root-sink.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hbase-table-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
27 files changed, 503 insertions(+), 253 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
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: merged
Gerrit-Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
Gerrit-Change-Number: 14941
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5394/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2020 17:56:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5: Code-Review+2


-- 
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: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2020 18:29:25 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14941

to look at the new patch set (#2).

Change subject: IMPALA-4192: Move static state from DataSink into a DataSinkConfig
......................................................................

IMPALA-4192: Move static state from DataSink into a DataSinkConfig

This patch adds a new class called DataSinkConfig which contains a
subset of the static state of their corresponding DataSink, of
which there is one instance per fragment. DataSink contains the
runtime state and there can be up to MT_DOP instances of it per
fragment.
Eventually all static state including codegened function pointers
would be moved to the PlanNodes.

Testing:
TODO: Run exhaustive tests successfully.

Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
---
M be/src/exec/blocking-plan-root-sink.cc
M be/src/exec/blocking-plan-root-sink.h
M be/src/exec/buffered-plan-root-sink.cc
M be/src/exec/buffered-plan-root-sink.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hbase-table-sink.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/plan-root-sink.cc
M be/src/exec/plan-root-sink.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
27 files changed, 500 insertions(+), 244 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/14941/2
-- 
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: newpatchset
Gerrit-Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
Gerrit-Change-Number: 14941
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer 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 4: Code-Review+2


-- 
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: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2020 18:16:33 +0000
Gerrit-HasComments: No