You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2023/11/21 17:41:37 UTC
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Csaba Ringhofer has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20719
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
WIP: Share compressor in KrpcDataStreamSender
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/compress.cc
M be/src/util/compress.h
9 files changed, 117 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/3
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20719/5/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/20719/5/be/src/runtime/krpc-data-stream-sender.cc@167
PS5, Line 167: Status Init(RuntimeState* state, std::shared_ptr<CharMemTrackerAllocator> allocator, OutboundRowBatch::Serializer* serializer);
line too long (129 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/5/be/src/runtime/krpc-data-stream-sender.cc@378
PS5, Line 378: RuntimeState* state, std::shared_ptr<CharMemTrackerAllocator> allocator, OutboundRowBatch::Serializer* serializer) {
line too long (120 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/5/be/src/runtime/krpc-data-stream-sender.cc@819
PS5, Line 819: outbound_batches_.emplace_back(char_mem_tracker_allocator_, serializer_/*.get()*/, is_local);
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/5/be/src/runtime/krpc-data-stream-sender.cc@1208
PS5, Line 1208: }
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/20719/5/be/src/util/compress.cc
File be/src/util/compress.cc:
http://gerrit.cloudera.org:8080/#/c/20719/5/be/src/util/compress.cc@349
PS5, Line 349: const int acceleration = 1;
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 12:22:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#4).
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
WIP: Share compressor in KrpcDataStreamSender
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/compress.cc
M be/src/util/compress.h
9 files changed, 118 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/4
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14716/ : 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/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2023 11:38:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 12: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 12
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Jan 2024 00:19:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9953/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 22:10:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 15:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10256/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Thu, 08 Feb 2024 15:35:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9986/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 13:54:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Zihao Ye (Code Review)" <ge...@cloudera.org>.
Zihao Ye has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 12: Code-Review+1
(2 comments)
Looks great!
http://gerrit.cloudera.org:8080/#/c/20719/12/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/20719/12/be/src/runtime/krpc-data-stream-sender.cc@1056
PS12, Line 1056:
nit: redundant space
http://gerrit.cloudera.org:8080/#/c/20719/12/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/20719/12/be/src/runtime/row-batch.cc@280
PS12, Line 280: _
nit: redundant underline
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 12
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Fri, 02 Feb 2024 07:02:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 18:14:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9948/
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 17:01:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 15: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Wed, 14 Feb 2024 16:12:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9948/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 12:21:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/14501/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 12:29:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Kurt Deschler, Zihao Ye, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#13).
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Before this patch each KrpcDataStreamSender::Channel had 2
OutboundRowBatch with its own serialization and compression buffers.
This patch switches to use a single buffer per channel. This is
enough to store the in-flight data in KRPC, while other buffers
are only used during serialization and compression which is done for
just a single channel at a time, so can be shared among channels.
Memory estimates in the planner are not changed because the existing
calculation has several issues (see IMPALA-12594).
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
8 files changed, 135 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/13
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 13
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 13:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20719/12/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/20719/12/be/src/runtime/krpc-data-stream-sender.cc@1056
PS12, Line 1056: u
> nit: redundant space
Done
http://gerrit.cloudera.org:8080/#/c/20719/11/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/20719/11/be/src/runtime/row-batch.cc@271
PS11, Line 271: bool* is_compressed, int64_t size, TrackedString* compression_scratch) {
> NM. I missed that this can be null.
ack
http://gerrit.cloudera.org:8080/#/c/20719/12/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/20719/12/be/src/runtime/row-batch.cc@280
PS12, Line 280:
> nit: redundant underline
Thanks for spotting, these used to be members in a previous iteration of the patch.
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 13
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Thu, 08 Feb 2024 15:29:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#9).
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Before this patch each KrpcDataStreamSender::Channel had 2
OutboundRowBatch with its own serialization and compression buffers.
This patch switches to use a single buffer per channel. This is
enough to store the in-flight data in KRPC, while other buffers
are only used during serialization and compression which is done for
just a single channel at a time, so can be shared among channels.
Memory estimates in the planner are not changed because the existing
calculation has several issues (see IMPALA-12594).
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
9 files changed, 128 insertions(+), 92 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/9
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 9
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14724/ : 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/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2023 20:37:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 10:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG@12
PS8, Line 12: This patch switches to use a single buffer per channel. This is
> Please also discuss the partitioned case, as that seems to be handled diffe
You mean in that unpartitioned case? The partitioned case uses 1 buffer per channel.
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG@18
PS8, Line 18: calculation has several issues (see IMPALA-12594).
> Is this supposed to point to IMPALA-12594?
oops, thanks
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/benchmarks/row-batch-serialize-benchmark.cc
File be/src/benchmarks/row-batch-serialize-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/benchmarks/row-batch-serialize-benchmark.cc@345
PS8, Line 345:
> What cleanup is still needed?
no, this remains from an older state of the patch
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/catalog/catalog-util.cc@358
PS8, Line 358: return Status::OK();
> Please note why this was added in the commit message (i.e. what was missed
This was needed only in a previous state of the patch, which made LZ4 compressor stateful. Decided to separate that change as it is orthogonal, so this is no longer needed.
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.h
File be/src/runtime/krpc-data-stream-sender.h:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.h@172
PS8, Line 172: Status SerializeBatch(
> Please update the function comment to describe how 'compress' should be use
Done
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@123
PS8, Line 123: // backing the in-flight RPC or kept as reserve if there is no ongoing RPC. The actual
> typo: in-flight
Done
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@128
PS8, Line 128: // unpartitioned case the shared outbound_batch_ holds the data for the the in-flight RPC.
> typo: in-flight
Done
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@185
PS8, Line 185: // If 'swap_batch' is true, 'outbound_batch' is swapped with the Channel's
> Please update the comment to describe when swap_batch should be true/false.
Done
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@244
PS8, Line 244: // Owns an outbound row batch that can be referenced by the in-flight RPC. Contains
> Does this TODO still make sense?
yes, I think that it could make things lot faster in some scenarios
I was considering adding simple queuing in this patch, but ideally it would be added together with / after implementing reservation support for KrpcDataStreamSender, which is a more complex task
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@601
PS8, Line 601: rpc_in_flight_ = true;
> Is there a DCHECK we could add here?
Done
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/row-batch.h@414
PS8, Line 414: Status Serialize(OutboundRowBatch* output_batch, bool full_dedup,
> line has trailing whitespace
Done
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2023 12:22:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 10: Code-Review+1
(10 comments)
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG@12
PS8, Line 12: This patch switches to use a single buffer per channel. This is
> You mean in that unpartitioned case? The partitioned case uses 1 buffer per
I guess I meant unpartitioned. But rereading it I understand you were referring to the buffer specifically, so this makes sense.
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG@18
PS8, Line 18: calculation has several issues (see IMPALA-12594).
> oops, thanks
Done
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/benchmarks/row-batch-serialize-benchmark.cc
File be/src/benchmarks/row-batch-serialize-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/benchmarks/row-batch-serialize-benchmark.cc@345
PS8, Line 345:
> no, this remains from an older state of the patch
Ack
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/catalog/catalog-util.cc@358
PS8, Line 358: return Status::OK();
> This was needed only in a previous state of the patch, which made LZ4 compr
Ack
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.h
File be/src/runtime/krpc-data-stream-sender.h:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.h@172
PS8, Line 172: Status SerializeBatch(
> Done
Ack
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@123
PS8, Line 123: // backing the in-flight RPC or kept as reserve if there is no ongoing RPC. The actual
> Done
Ack
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@128
PS8, Line 128: // unpartitioned case the shared outbound_batch_ holds the data for the the in-flight RPC.
> Done
Ack
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@185
PS8, Line 185: // If 'swap_batch' is true, 'outbound_batch' is swapped with the Channel's
> Done
Ack
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@244
PS8, Line 244: // Owns an outbound row batch that can be referenced by the in-flight RPC. Contains
> yes, I think that it could make things lot faster in some scenarios
Ack
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@601
PS8, Line 601: rpc_in_flight_ = true;
> Done
Ack
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2023 17:52:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20719/11/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/20719/11/be/src/runtime/row-batch.cc@271
PS11, Line 271: bool* is_compressed, int64_t size, TrackedString* compression_scratch) {
Change to TrackedString& or TrackedString*const here and above
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2023 18:36:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 15:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10257/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Thu, 08 Feb 2024 15:35:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Before this patch each KrpcDataStreamSender::Channel had 2
OutboundRowBatch with its own serialization and compression buffers.
This patch switches to use a single buffer per channel. This is
enough to store the in-flight data in KRPC, while other buffers
are only used during serialization and compression which is done for
just a single channel at a time, so can be shared among channels.
Memory estimates in the planner are not changed because the existing
calculation has several issues (see IMPALA-12594).
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Reviewed-on: http://gerrit.cloudera.org:8080/20719
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Kurt Deschler <kd...@cloudera.com>
Reviewed-by: Zihao Ye <ey...@163.com>
Reviewed-by: Michael Smith <mi...@cloudera.com>
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
8 files changed, 135 insertions(+), 96 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Kurt Deschler: Looks good to me, but someone else must approve
Zihao Ye: Looks good to me, but someone else must approve
Michael Smith: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 16
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#5).
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
WIP: Share compressor in KrpcDataStreamSender
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/compress.cc
M be/src/util/compress.h
9 files changed, 118 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/5
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#6).
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
WIP: Share compressor in KrpcDataStreamSender
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
M be/src/util/compress.cc
M be/src/util/compress.h
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
11 files changed, 141 insertions(+), 88 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/6
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/14492/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2023 18:10:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9944/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2023 17:42:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/15210/ : 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/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 13
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Thu, 08 Feb 2024 15:55:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#10).
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Before this patch each KrpcDataStreamSender::Channel had 2
OutboundRowBatch with its own serialization and compression buffers.
This patch switches to use a single buffer per channel. This is
enough to store the in-flight data in KRPC, while other buffers
are only used during serialization and compression which is done for
just a single channel at a time, so can be shared among channels.
Memory estimates in the planner are not changed because the existing
calculation has several issues (see IMPALA-12594).
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
8 files changed, 131 insertions(+), 92 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/10
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 11: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Dec 2023 01:30:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#11).
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Before this patch each KrpcDataStreamSender::Channel had 2
OutboundRowBatch with its own serialization and compression buffers.
This patch switches to use a single buffer per channel. This is
enough to store the in-flight data in KRPC, while other buffers
are only used during serialization and compression which is done for
just a single channel at a time, so can be shared among channels.
Memory estimates in the planner are not changed because the existing
calculation has several issues (see IMPALA-12594).
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
8 files changed, 135 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/11
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20719/11/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/20719/11/be/src/runtime/row-batch.cc@271
PS11, Line 271: bool* is_compressed, int64_t size, TrackedString* compression_scratch) {
> Change to TrackedString& or TrackedString*const here and above
NM. I missed that this can be null.
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 20:07:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#8).
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Before this patch each KrpcDataStreamSender::Channel had 2
OutboundRowBatch with its own serialization and compression buffers.
This patch switches to use a single buffer per channel. This is
enough to store the in-flight data in KRPC, while other buffers
are only used during serialization and compression which is done for
just a single channel at a time, so can be shared among channels.
Memory estimates in the planner are not changed because the existing
calculation has several issues (see IMPALA-12433).
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
9 files changed, 128 insertions(+), 92 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/8
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14620/ : 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/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 14:21:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20719/6/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20719/6/be/src/runtime/row-batch.h@414
PS6, Line 414: Status Serialize(OutboundRowBatch* output_batch, bool full_dedup,
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/20719/6/be/src/util/compress.cc
File be/src/util/compress.cc:
http://gerrit.cloudera.org:8080/#/c/20719/6/be/src/util/compress.cc@349
PS6, Line 349: const int acceleration = 1;
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 21:45:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/14507/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 22:03:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 8:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG@12
PS8, Line 12: This patch switches to use a single buffer per channel. This is
Please also discuss the partitioned case, as that seems to be handled differently.
http://gerrit.cloudera.org:8080/#/c/20719/8//COMMIT_MSG@18
PS8, Line 18: calculation has several issues (see IMPALA-12433).
Is this supposed to point to IMPALA-12594?
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/benchmarks/row-batch-serialize-benchmark.cc
File be/src/benchmarks/row-batch-serialize-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/benchmarks/row-batch-serialize-benchmark.cc@345
PS8, Line 345: // TODO: cleanup
What cleanup is still needed?
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/catalog/catalog-util.cc@358
PS8, Line 358: compressor->Close();
Please note why this was added in the commit message (i.e. what was missed before).
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.h
File be/src/runtime/krpc-data-stream-sender.h:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.h@172
PS8, Line 172: RowBatch* src, OutboundRowBatch* dest, bool compress, int num_receivers = 1);
Please update the function comment to describe how 'compress' should be used.
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@123
PS8, Line 123: // backing the in-fligh RPC or kept as reserve if there is no ongoing RPC. The actual
typo: in-flight
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@128
PS8, Line 128: // unpartitioned case the shared outbound_batch_ holds the data for the the in-fligh RPC.
typo: in-flight
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@185
PS8, Line 185: Status TransmitData(std::unique_ptr<OutboundRowBatch>* outbound_batch, bool swap_batch);
Please update the comment to describe when swap_batch should be true/false.
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@244
PS8, Line 244: // TODO: replace this with a queue. Schedule another RPC callback in the
Does this TODO still make sense?
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/krpc-data-stream-sender.cc@601
PS8, Line 601: // At this point the previous RPC must be already finished and the previous buffer
Is there a DCHECK we could add here?
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 22:31:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20719/8/be/src/runtime/row-batch.h@414
PS8, Line 414: Status Serialize(OutboundRowBatch* output_batch, bool full_dedup,
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 13:55:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 5:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/14502/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 12:45:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 7: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9953/
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Nov 2023 02:35:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20719/7/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20719/7/be/src/runtime/row-batch.h@414
PS7, Line 414: Status Serialize(OutboundRowBatch* output_batch, bool full_dedup,
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/20719/7/be/src/util/compress.cc
File be/src/util/compress.cc:
http://gerrit.cloudera.org:8080/#/c/20719/7/be/src/util/compress.cc@349
PS7, Line 349: const int acceleration = 1;
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 22:10:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Zihao Ye (Code Review)" <ge...@cloudera.org>.
Zihao Ye has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 15: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Mon, 19 Feb 2024 02:56:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 15: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Thu, 08 Feb 2024 20:15:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zihao Ye <ey...@163.com>
Gerrit-Comment-Date: Tue, 20 Feb 2024 17:50:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9952/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 21:44:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20719/4/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/20719/4/be/src/runtime/krpc-data-stream-sender.cc@167
PS4, Line 167: Status Init(RuntimeState* state, std::shared_ptr<CharMemTrackerAllocator> allocator, OutboundRowBatch::Serializer* serializer);
line too long (129 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/4/be/src/runtime/krpc-data-stream-sender.cc@378
PS4, Line 378: RuntimeState* state, std::shared_ptr<CharMemTrackerAllocator> allocator, OutboundRowBatch::Serializer* serializer) {
line too long (120 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/4/be/src/runtime/krpc-data-stream-sender.cc@819
PS4, Line 819: outbound_batches_.emplace_back(char_mem_tracker_allocator_, serializer_/*.get()*/, is_local);
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/4/be/src/runtime/krpc-data-stream-sender.cc@1208
PS4, Line 1208: }
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/20719/4/be/src/util/compress.cc
File be/src/util/compress.cc:
http://gerrit.cloudera.org:8080/#/c/20719/4/be/src/util/compress.cc@349
PS4, Line 349: const int acceleration = 1;
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 12:10:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 7:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/14509/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2023 22:36:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20719
to look at the new patch set (#7).
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
WIP: Share compressor in KrpcDataStreamSender
Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
M be/src/util/compress.cc
M be/src/util/compress.h
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
11 files changed, 144 insertions(+), 90 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/20719/7
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9944/
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2023 19:51:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Share compressor in KrpcDataStreamSender
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: WIP: Share compressor in KrpcDataStreamSender
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20719/3/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/20719/3/be/src/runtime/krpc-data-stream-sender.cc@167
PS3, Line 167: Status Init(RuntimeState* state, std::shared_ptr<CharMemTrackerAllocator> allocator, OutboundRowBatch::Serializer* serializer);
line too long (129 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/3/be/src/runtime/krpc-data-stream-sender.cc@378
PS3, Line 378: RuntimeState* state, std::shared_ptr<CharMemTrackerAllocator> allocator, OutboundRowBatch::Serializer* serializer) {
line too long (120 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/3/be/src/runtime/krpc-data-stream-sender.cc@819
PS3, Line 819: outbound_batches_.emplace_back(char_mem_tracker_allocator_, serializer_/*.get()*/, is_local);
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/20719/3/be/src/runtime/krpc-data-stream-sender.cc@1208
PS3, Line 1208: }
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/20719/3/be/src/util/compress.cc
File be/src/util/compress.cc:
http://gerrit.cloudera.org:8080/#/c/20719/3/be/src/util/compress.cc@349
PS3, Line 349: const int acceleration = 1;
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2023 17:42:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20719 )
Change subject: IMPALA-12433: Share buffers among channels in KrpcDataStreamSender
......................................................................
Patch Set 11:
PS 11 changes only a stale comment.
--
To view, visit http://gerrit.cloudera.org:8080/20719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I64854a350a9dae8bf3af11c871882ea4750e60b3
Gerrit-Change-Number: 20719
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Dec 2023 20:10:52 +0000
Gerrit-HasComments: No