[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 7 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 00:55:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14641/7/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/14641/7/be/src/util/debug-util.h@146 PS7, Line 146: std::vector args) const-reference -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 27 Nov 2019 00:55:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9181: Serialize TQueryCtx once per query
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14777 )
Change subject: IMPALA-9181: Serialize TQueryCtx once per query
..
Patch Set 3: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14777/3/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/14777/3/be/src/runtime/coordinator-backend-state.cc@215
PS3, Line 215: unique_ptr sidecar_buf =
make_unique();
: sidecar_buf->assign_copy(serialized_buf, serialized_len);
: unique_ptr rpc_sidecar =
RpcSidecar::FromFaststring(move(sidecar_buf));
Not this change but this can in theory be converted to sidecar using Slice too.
That said, we may better hold off from doing so now until we convert
ExecQueryFInstance() RPC to asynchronous as the memory management is slightly
different in that case.
May be a TODO for now is sufficient.
http://gerrit.cloudera.org:8080/#/c/14777/3/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/14777/3/be/src/service/control-service.cc@120
PS3, Line 120: Status GetSidecar(int sidecar_idx, RpcContext* rpc_context, T*
thrift_obj) {
static
--
To view, visit http://gerrit.cloudera.org:8080/14777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a4dd302fd5602ec2775492a041ddd51e7d7a6c6
Gerrit-Change-Number: 14777
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Wed, 27 Nov 2019 00:09:52 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9137, IMPALA-9138: Mark failed RPCs as retryable and update blacklist
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14677 ) Change subject: IMPALA-9137, IMPALA-9138: Mark failed RPCs as retryable and update blacklist .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/14677/3/common/thrift/Status.thrift File common/thrift/Status.thrift: http://gerrit.cloudera.org:8080/#/c/14677/3/common/thrift/Status.thrift@48 PS3, Line 48: 4: optional TRPCErrorMessage rpc_msg > yeah, the point about constructing new Status objects from existing ones is Discussed with Sahil offline about it. Summary below: - StatusAuxInfo may be a slightly better fit for future extensibility - Most control services are initiated from the coordinator so not much need for elaborate propagation of the aux info - For the most part, the propagation of aux info only needs to happen for fragment instance's execution. In which case. we can consider dumping the aux info in RuntimeState or some per-fragment instance state and collect them when reporting status. -- To view, visit http://gerrit.cloudera.org:8080/14677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I733cca13847fde43c8ea2ae574d3ae04bd06419c Gerrit-Change-Number: 14677 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 26 Nov 2019 21:29:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8138: Reintroduce rpc debugging options
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 ) Change subject: IMPALA-8138: Reintroduce rpc debugging options .. Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@50 PS5, Line 50: address Please document this arg too. http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@115 PS5, Line 115: std::string hostname_; : std::string port_; These can be const, right ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@153 PS5, Line 153: WARN_UNUSED_RESULT nit: Not your change, but can this be moved to the end of the declaration ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@154 PS5, Line 154: std::vector const std::vector& ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@159 PS5, Line 159: WARN_UNUSED_RESULT nit: Not your change, but can this be moved to the end of the declaration ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@161 PS5, Line 161: std::vector() Given the default arg above, is this necessary or is it just for clarity ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@165 PS5, Line 165: std::vector() Given the default arg above, is this necessary or is it just for clarity ? http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@92 PS5, Line 92: :...: Given that ":" is already used for delimiting different component of the debug action, do you think it may be slightly clearer to not re-use ":" for separating the args ? Instead, can we use "," or other character ? http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@105 PS5, Line 105: //exercising different error paths. May help to briefly document the purpose of arg list. -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 25 Nov 2019 21:31:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10855 ) Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC .. Patch Set 23: (1 comment) http://gerrit.cloudera.org:8080/#/c/10855/23/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10855/23/be/src/runtime/query-state.cc@353 PS23, Line 353: unique_ptr sidecar_buf = make_unique(); : sidecar_buf->assign_copy(profile_buf, profile_len); : unique_ptr sidecar = RpcSidecar::FromFaststring(move(sidecar_buf)); Can't we use a slice here instead ? -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 23 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 02 Nov 2019 00:19:19 + Gerrit-HasComments: Yes
***UNCHECKED*** [Impala-ASF-CR] IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
..
Patch Set 27: Code-Review+2
(19 comments)
LGTM. Please address the comments below.
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/benchmarks/bloom-filter-benchmark.cc
File be/src/benchmarks/bloom-filter-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/benchmarks/bloom-filter-benchmark.cc@290
PS27, Line 290: std::shared_ptr
No need for shared_ptr
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1022
PS25, Line 1022: {
> Thanks for pointing this out.
Makes sense.
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1072
PS25, Line 1072:
bloom_filter_directory.swap(state->bloom_filter_directory());
: DCHECK(rpc_params.bloom_filter().always_false()
> Thanks for the suggestion!
I guess it can simply be
*rpc_params.mutable_bloom_filter() = state->bloom_filter();
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1074
PS25, Line 1074: || rpc_params.bloo
> Thanks for the suggestion.
Missed the part about the scope of state. Think it's fine to keep it as-is.
Thanks for the explanation.
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1098
PS25, Line 1098:
> Since 'state' is defined inside of the critical section as mentioned above,
Makes sense.
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/runtime/coordinator.cc@1096
PS27, Line 1096: reinterpret_cast(&(bloom_filter_directory[0])),
: static_cast(bloom_filter_directory.size())
Please see comments at BloomFilter::AddDirectorySidecar(). We can pass in the
string directly instead.
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/runtime/runtime-filter-bank.cc
File be/src/runtime/runtime-filter-bank.cc:
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/runtime/runtime-filter-bank.cc@187
PS27, Line 187: DCHECK(
nit: DCHECK_EQ
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/service/impala-server.h@35
PS25, Line 35: #include "gen-cpp/ImpalaInternalService.h"
> Thanks for this suggestion.
Looks like it's more involved than expected. Please feel free to defer it to
the follow-up patch which removes ImpalaInternalService altogether.
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/service/impala-server.h@70
PS25, Line 70: class TSessionState;
: class TQueryOptions;
> Thanks for pointing this out!
Sounds good.
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter-test.cc
File be/src/util/bloom-filter-test.cc:
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter-test.cc@a68
PS27, Line 68:
:
:
Why not keep most of this header comment which explains what this test does and
talks about the output arguments (e.g. success, protobuf, directory)
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter-test.cc@103
PS27, Line 103: *success = directory_y.compare(directory_y2) == 0 ? true :
false;
*success = directory_y.compare(directory_y2) == 0;
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter-test.cc@365
PS27, Line 365: std::shared_ptr<
No need for shared_ptr
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter-test.cc@367
PS27, Line 367:
nit: unnecessary blank line
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter-test.cc@395
PS27, Line 395: TBloomFilter
BloomFilterPB
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter.h@95
PS27, Line 95: const uint8_t* directory_in,
: size_t directory_in_size)
Can this interface take a string only instead for the directory ? Can we use
directory.size() to get the size in Init() or are there cases in which passing
a string won't work ?
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter.cc
File be/src/util/bloom-filter.cc:
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter.cc@79
PS27, Line 79: protobuf
rpc_params
http://gerrit.cloudera.org:8080/#/c/13882/27/be/src/util/bloom-filter.cc@80
PS27, Line 80: const char* directory,
: unsigned long directory_size
See comments below. This could be const string& directory. I believe
faststring::append() also has an interface for taking string as input.
[Impala-ASF-CR] IMPALA-9116: KUDU-2989. Work around SASL bug when FQDN is >d characters
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14614 ) Change subject: IMPALA-9116: KUDU-2989. Work around SASL bug when FQDN is >=64 characters .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14614 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4898814f2f7ab87151798336414dde7078d28a4a Gerrit-Change-Number: 14614 Gerrit-PatchSet: 1 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 01 Nov 2019 17:20:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9116: Work around SASL bug when FQDN is >d characters in Kudu RPC.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14610 ) Change subject: IMPALA-9116: Work around SASL bug when FQDN is >=64 characters in Kudu RPC. .. Patch Set 2: We usually cherry-pick the Kudu patch so it's easier to track the original change (including the gerrit link to the original Kudu change). I am okay with exception if this is super urgent. -- To view, visit http://gerrit.cloudera.org:8080/14610 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f05f70915ed20c97efd0ae7295b181a010cf0f6 Gerrit-Change-Number: 14610 Gerrit-PatchSet: 2 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 01 Nov 2019 06:28:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9116: Work around SASL bug when FQDN is >d characters in Kudu RPC.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14610 ) Change subject: IMPALA-9116: Work around SASL bug when FQDN is >=64 characters in Kudu RPC. .. Patch Set 1: (1 comment) LGTM. Is the plan to do it on Impala side first and then backport it to Kudu ? We usually do changes the other way (i.e. from Kudu side first and then cherry-picked to Impala side). http://gerrit.cloudera.org:8080/#/c/14610/1/be/src/kudu/rpc/server_negotiation.cc File be/src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/14610/1/be/src/kudu/rpc/server_negotiation.cc@391 PS1, Line 391: (!server_fqdn nit: server_fqdn == nullptr -- To view, visit http://gerrit.cloudera.org:8080/14610 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f05f70915ed20c97efd0ae7295b181a010cf0f6 Gerrit-Change-Number: 14610 Gerrit-PatchSet: 1 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 01 Nov 2019 06:07:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
..
Patch Set 25:
(26 comments)
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator-backend-state.cc@556
PS25, Line 556: LOG(ERROR) << "PublishFilter() rpc failed: " <<
rpc_status.ToString();
return here as there is no point in continuing to print the result status if
RPC failed.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1134
PS24, Line 1134: r is neither an always true filter nor an
: // always false filter, then it must be the case that a
non-empty sidecar slice
> I actually found out that if we are using "std::move(sidecar_slice.ToString
Sorry for the wrong advice. I believe the RVO should take care of the
copy-elision so no need for the std::move. In other words, it should be
sufficient to just call sidecar_slice.ToString();
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1010
PS25, Line 1010: // >>> IMPALA-7984: Port runtime filter from Thrift RPC to
KRPC
To be removed ?!
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1022
PS25, Line 1022: std::
nit: no need for std::
Also, may make sense to move this declaration to somewhere closer to its use
(e.g. line 1068 below).
Same for rpc_params and target_fragment_idxs above.
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1072
PS25, Line 1072: BloomFilterPB& aggregated_filter = state->bloom_filter();
:
aggregated_filter.Swap(rpc_params.mutable_bloom_filter());
This swapping tricks seems unnecessary now that the bloom_filter_directory is a
separate structure.
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1074
PS25, Line 1074: bloom_filter_directory
Is there a reason to assign state->bloom_filter_directory() to this local
variable ? Why cannot we use state->bloom_filter_directory() directly below ?
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1098
PS25, Line 1098: reinterpret_cast(&(bloom_filter_directory[0])
state->bloom_filter_directory().data()
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1153
PS25, Line 1153: bloom_filter_ = BloomFilterPB(params.bloom_filter());
bloom_filter_ = params.bloom_filter();
The above should be sufficient.
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/coordinator.cc@1155
PS25, Line 1155: std::string(
: reinterpret_cast(sidecar_slice.data()), sidecar_slice.size())
Can keep the original suggestion of sidecar_slice.ToString(). C++ ROV should
take care of avoiding the copy so the TODO can be removed. There is no easy way
to avoid copying the sidecar from the network buffer so we have to copy at
least once.
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/runtime-filter-bank.cc
File be/src/runtime/runtime-filter-bank.cc:
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/runtime-filter-bank.cc@190
PS25, Line 190: std::
nit: no need for std::
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/runtime-filter-bank.cc@204
PS25, Line 204: std::
nit: no need for std::
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/runtime-filter-bank.cc@205
PS25, Line 205: ++num_inflight_rpcs_;
DCHECK_GE(num_inflight_rpcs_, 0); before increment.
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/runtime-filter-bank.cc@323
PS25, Line 323: std::
nit: no need for std::
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/timestamp-value.h@170
PS25, Line 170: // Store the binary representation of this TimestampValue in
'tvalue'.
: void ToTColumnValue(TColumnValue* tvalue) const {
: const uint8_t* data = reinterpret_cast(this);
: tvalue->timestamp_val.assign(data, data + Size());
: tvalue->__isset.timestamp_val = true;
: }
Is this not used anymore ?
http://gerrit.cloudera.org:8080/#/c/13882/25/be/src/runtime/timestamp-value.h@183
PS25, Line 183: // Returns a new TimestampValue created from the value in
'tvalue'.
: static TimestampValue FromTColumnValue(const TColumnValue&
tvalue) {
: TimestampValue value;
[Impala-ASF-CR] IMPALA-9026: Use resolved IP address for statestore subscriber
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14388 ) Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/14388/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14388/3//COMMIT_MSG@17 PS3, Line 17: muplitle > typo Done -- To view, visit http://gerrit.cloudera.org:8080/14388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 Gerrit-Change-Number: 14388 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 23 Oct 2019 05:33:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9026: Use resolved IP address for statestore subscriber
Hello Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14388 to look at the new patch set (#4). Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber .. IMPALA-9026: Use resolved IP address for statestore subscriber This change adds a flag (--statestore_subscriber_use_resolved_address) which, if set to true, allows statestore subscribers to use its resolved IP address instead of its hostname as the heartbeat address which statestore sends heartbeats / updates to. This flag is useful in certain situation in which the subscriber's DNS entry may not be present for a valid reason (e.g. a Kubernetes pod whose readiness probe returns false). An example is that there are multiple Impala coordinators but only one of them will be active at a time (for admission control reason) and the rest will serve as backup. In which case, we still want the backup coordinators to receive updates from statestore but not serve any queries. Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 --- M be/src/catalog/catalog-server.cc M be/src/runtime/exec-env.cc M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-subscriber.h M tests/custom_cluster/test_custom_statestore.py 5 files changed, 35 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/14388/4 -- To view, visit http://gerrit.cloudera.org:8080/14388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 Gerrit-Change-Number: 14388 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar
[Impala-ASF-CR] IMPALA-9026: Use resolved IP address for statestore subscriber
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14388 ) Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/14388/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14388/3//COMMIT_MSG@17 PS3, Line 17: muplitle typo -- To view, visit http://gerrit.cloudera.org:8080/14388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 Gerrit-Change-Number: 14388 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 23 Oct 2019 02:17:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9026: Use resolved IP address for statestore subscriber
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14388 )
Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14388/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14388/2//COMMIT_MSG@19
PS2, Line 19:
> nit: line too long
Done
http://gerrit.cloudera.org:8080/#/c/14388/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/14388/2/be/src/runtime/exec-env.cc@222
PS2, Line 222: tmp_file_mgr_(new TmpFileMgr),
: frontend_(new Frontend()),
: async_rpc_pool_(new CallableThreadPool("rpc-pool",
"async-rpc-sender", 8, 1)),
: query_exec_mgr_(new QueryExecMgr()),
: rpc_metrics_(metrics_->GetOrCreateChildG
> whats the reason for moving these from Init() to the constructor?
Failing in Init() below or here are both fatal (i.e. Impala will not start) so
I may as well remove the complication of splitting the initialization of
krpc_address_ into two functions.
In a previous version of the patch I needed the IP address in this function to
initialize the statestore subscriber but has since modified the code but I feel
the simplification above is still warranted.
http://gerrit.cloudera.org:8080/#/c/14388/2/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:
http://gerrit.cloudera.org:8080/#/c/14388/2/be/src/statestore/statestore-subscriber.cc@262
PS2, Line 262: hostname
> document that the hostname can now either be the actual hostname, or the ip
Comments updated in the header filer. Seems simpler to just resolve the
hostname here instead of having the server choose between multiple addresses.
--
To view, visit http://gerrit.cloudera.org:8080/14388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2
Gerrit-Change-Number: 14388
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Wed, 23 Oct 2019 02:16:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9026: Use resolved IP address for statestore subscriber
Hello Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14388 to look at the new patch set (#3). Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber .. IMPALA-9026: Use resolved IP address for statestore subscriber This change adds a flag (--statestore_subscriber_use_resolved_address) which, if set to true, allows statestore subscribers to use its resolved IP address instead of its hostname as the heartbeat address which statestore sends heartbeats / updates to. This flag is useful in certain situation in which the subscriber DNS entry may not be present for a valid reason (e.g. a Kubernetes pod whose readiness probe returns false). An example is that there are muplitle Impala coordinator but only one of them will be active at a time (for admission control reasons) and the rest will serve as backup. In which case, we still want the backup coordinators to receive updates from statestore but not serve any queries. Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 --- M be/src/catalog/catalog-server.cc M be/src/runtime/exec-env.cc M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-subscriber.h M tests/custom_cluster/test_custom_statestore.py 5 files changed, 35 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/14388/3 -- To view, visit http://gerrit.cloudera.org:8080/14388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 Gerrit-Change-Number: 14388 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar
[Impala-ASF-CR] IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
..
Patch Set 24:
(17 comments)
Sorry for the delays in reviews and thanks for the detailed replies. Please see
some more comments below.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator-filter-state.h
File be/src/runtime/coordinator-filter-state.h:
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator-filter-state.h@124
PS24, Line 124: string
Please also document what this field stands for.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1103
PS24, Line 1103:// Workaround for fact that parameters are const& for
Protobuf RPCs.
: BloomFilterPB* non_const_filter =
_cast(params->bloom_filter());
I don't think this makes sense anymore with the implementation using protobuf.
The reason is that the old Thrift implementation embedded the entire string
directory in it but in this new implementation, it's stored in the sidecar so
we aren't saving much if anything by doing the swap trick below.
We can just initialize the BloomFilterPB by copying if necessary.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1108
PS24, Line 1108: if (!bloom_filter_.has_log_bufferpool_space()) {
May be I am missing the details here but why is it not a no-op if the incoming
filter is always false ?
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1110
PS24, Line 1110: bloom_filter_.Swap(non_const_filter);
See comments above.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1132
PS24, Line 1132: bloom_filter_.Swap(non_const_filter);
See comments above.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1140
PS24, Line 1140: sidecar_slice.ToString(),
Seems less expensive if we can pass the slice directly. This avoids doing a
copy from the slice's buffer into the string argument.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1141
PS24, Line 1141: bloom_filter_directory_
May need to do some const casting but please consider using:
bloom_filter_directory_.data()
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/query-state.h@202
PS24, Line 202: const PublishFilterParamsPB*
Why not keep it as const PublishFilterParamsPB& ?
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc
File be/src/runtime/runtime-filter-bank.cc:
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@174
PS24, Line 174: UpdateFilterParamsPB* params = obj_pool_.Add(new
UpdateFilterParamsPB);
> Thank you for pointing this out! You are correct. 'params' can be freed onc
Or you can just allocate it on the stack as it isn't too large:
UpdateFilterParamsPB params;
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@175
PS24, Line 175: UpdateFilterResultPB* res = obj_pool_.Add(new
UpdateFilterResultPB);
: RpcController* controller = obj_pool_.Add(new
RpcController);
> Thank you very much for the suggestion!
Don't think it's safe to free the memory in the completion callback as the KRPC
stack may still have reference to it.
One way to share the parameters is to ensure there is one outstanding RPC per
controller object but it's probably not worth the trouble so I am fine keep
them as-is.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@196
PS24, Line 196: Couldn't send filter to coordinator: "
Substitute("Failed to get proxy to coordinator $0: $1", host_address,
get_proxy_status.msg().msg());
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@240
PS24, Line 240: LOG(WARNING) << "Cannot get inbound sidecar:
LOG(ERROR) << "Failed to get bloom filter sidecar" << ...
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@320
PS24, Line 320: // Wait for all inflight rpcs to complete before closing the
filters.
: {
: std::unique_lock l1(num_inflight_rpcs_lock_);
: while (num_inflight_rpcs_ > 0) {
: krpcs_done_cv_.wait(l1);
: }
: }
:
: lock_guard l2(runtime_filter_lock_);
: closed_ = true;
> That 'propagation stage' above should be 'aggregation stage' instead. Sorry
Actually, it's possible for RuntimeFilterBank::UpdateFilterFromLocal() and
FragmentInstanceState::Close() to be called from different thread
[Impala-ASF-CR] IMPALA-9026: Use resolved IP address for statestore subscriber
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14388 to look at the new patch set (#2). Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber .. IMPALA-9026: Use resolved IP address for statestore subscriber This change adds a flag (FLAGS_statestore_subscriber_use_resolved_address) which, if set to true, allows statestore subscribers to use its resolved IP address instead of its hostname as the heartbeat address which statestore sends heartbeats / updates to. This flag is useful in certain deployment scenarios in which a pod may have an IP address but its DNS entry is not available for a valid reason (e.g. a Kubernetes pod whose readiness probe returns false). An example is that there are more than one instances of Impala coordinator but only one of them will be active at a time (for admission control reasons) and the rest will serve as backup. In which case, we want the backup coordinator to receive updates from statestore but not serve any queries until the primary coordinator fails. Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 --- M be/src/catalog/catalog-server.cc M be/src/runtime/exec-env.cc M be/src/statestore/statestore-subscriber.cc M tests/custom_cluster/test_custom_statestore.py 4 files changed, 33 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/14388/2 -- To view, visit http://gerrit.cloudera.org:8080/14388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 Gerrit-Change-Number: 14388 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-9026: Use resolved IP address for statestore subscriber
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14388 ) Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/14388/1/tests/custom_cluster/test_custom_statestore.py File tests/custom_cluster/test_custom_statestore.py: http://gerrit.cloudera.org:8080/#/c/14388/1/tests/custom_cluster/test_custom_statestore.py@100 PS1, Line 100: ; > flake8: E703 statement ends with a semicolon Done http://gerrit.cloudera.org:8080/#/c/14388/1/tests/custom_cluster/test_custom_statestore.py@102 PS1, Line 102: ; > flake8: E703 statement ends with a semicolon Done -- To view, visit http://gerrit.cloudera.org:8080/14388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 Gerrit-Change-Number: 14388 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Tue, 08 Oct 2019 19:16:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9026: Use resolved IP address for statestore subscriber
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14388 Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber .. IMPALA-9026: Use resolved IP address for statestore subscriber This change adds a flag (FLAGS_statestore_subscriber_use_resolved_address) which, if set to true, allows statestore subscribers to use its resolved IP address instead of its hostname as the heartbeat address which statestore sends heartbeats / updates to. This flag is useful in certain deployment scenarios in which a pod may have an IP address but its DNS entry is not available for a valid reason (e.g. a Kubernetes pod whose readiness probe returns false). An example is that there are more than one instances of Impala coordinator but only one of them will be active at a time (for admission control reasons) and the rest will serve as backup. In which case, we want the backup coordinator to receive updates from statestore but not serve any queries until the primary coordinator fails. Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 --- M be/src/catalog/catalog-server.cc M be/src/runtime/exec-env.cc M be/src/statestore/statestore-subscriber.cc M tests/custom_cluster/test_custom_statestore.py 4 files changed, 33 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/14388/1 -- To view, visit http://gerrit.cloudera.org:8080/14388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 Gerrit-Change-Number: 14388 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho
[Impala-ASF-CR] IMPALA-8926, IMPALA-8989: Fix flaky result spooling tests
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14337 ) Change subject: IMPALA-8926, IMPALA-8989: Fix flaky result spooling tests .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/14337/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/14337/1/tests/custom_cluster/test_admission_controller.py@1337 PS1, Line 1337: # Once the 'lineitem' scan completes, NumCompletedBac > Yeah, maybe discussing in person is best, will find you tomorrow. Fair enough. I suppose all functions waiting for certain condition to become true has a timeout by default so there is no getting away from using some sort of timeout to avoid the test from hanging forever. May be safer to bump the timeout to a higher value such as 60s. -- To view, visit http://gerrit.cloudera.org:8080/14337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ea6bf3d84f174745c8a0b1e0f2b55ce05ee618b Gerrit-Change-Number: 14337 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Thu, 03 Oct 2019 18:14:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8960: Fix test owner privileges::test drop if exists on S3
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14334 ) Change subject: IMPALA-8960: Fix test_owner_privileges::test_drop_if_exists on S3 .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14334 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibfe15ac2d5ba0d8a6d4383be8d01395c74d67332 Gerrit-Change-Number: 14334 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Wed, 02 Oct 2019 23:38:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8926, IMPALA-8989: Fix flaky result spooling tests
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14337 ) Change subject: IMPALA-8926, IMPALA-8989: Fix flaky result spooling tests .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14337/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/14337/1/tests/custom_cluster/test_admission_controller.py@1337 PS1, Line 1337: sleep(30) # Wait for the 'lineitem' scan to complete > The test itself is timing dependent, so I don't think we can get out of usi Not sure if there is any misunderstanding here. I understand the purpose of the test is to validate that NumCompletedBackends == 1 and I am not suggesting you to remove that check. Instead, my suggestion is that the test should wait on a logical condition in which NumCompletedBackends is expected to be 1 before checking NumCompletedBackends == 1 in the query profile. My understanding is that this condition should happen once all instances of F01 reach the "FINISHED" state. Anyhow, this is just a minor point but I think timing dependent tests are usually prone to flakiness so avoiding that if possible would be great. Please feel free to grab me and discuss this offline if it's still not clear. -- To view, visit http://gerrit.cloudera.org:8080/14337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ea6bf3d84f174745c8a0b1e0f2b55ce05ee618b Gerrit-Change-Number: 14337 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 02 Oct 2019 23:38:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8960: Fix test owner privileges::test drop if exists on S3
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14334 )
Change subject: IMPALA-8960: Fix test_owner_privileges::test_drop_if_exists on
S3
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14334/1/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:
http://gerrit.cloudera.org:8080/#/c/14334/1/tests/authorization/test_owner_privileges.py@172
PS1, Line 172: self.execute_query("grant all on uri
'/test-warehouse/libTestUdfs.so' to"
I could be missing some details here but why not use FILESYSTEM_PREFIX instead ?
--
To view, visit http://gerrit.cloudera.org:8080/14334
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfe15ac2d5ba0d8a6d4383be8d01395c74d67332
Gerrit-Change-Number: 14334
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Comment-Date: Wed, 02 Oct 2019 19:43:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8926, IMPALA-8989: Fix flaky result spooling tests
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14337 ) Change subject: IMPALA-8926, IMPALA-8989: Fix flaky result spooling tests .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14337/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/14337/1/tests/custom_cluster/test_admission_controller.py@1337 PS1, Line 1337: sleep(30) # Wait for the 'lineitem' scan to complete > The goal of the test is to actually validate that NumCompletedBackends is i assert_eventually still has a timeout itself so it will suffer the same timing issue in the worst case. Given that this test checks that NumCompletedBackends is incremented by one, isn't it more precise to check that all F01 reached "FINISHED" states by parsing the json output from the debug webpage ? http://:25000/query_finstances?query_id= A proxy for the above is to check the number of in-flight fragments on all nodes. This of course bakes a lot of assumption into the test about the generated plan for the given query. -- To view, visit http://gerrit.cloudera.org:8080/14337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ea6bf3d84f174745c8a0b1e0f2b55ce05ee618b Gerrit-Change-Number: 14337 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 02 Oct 2019 17:45:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8926, IMPALA-8989: Fix flaky result spooling tests
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14337 ) Change subject: IMPALA-8926, IMPALA-8989: Fix flaky result spooling tests .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14337/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/14337/1/tests/custom_cluster/test_admission_controller.py@1337 PS1, Line 1337: sleep(30) # Wait for the 'lineitem' scan to complete Will number of a in-flight fragments a more reliable metric to wait on ? -- To view, visit http://gerrit.cloudera.org:8080/14337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ea6bf3d84f174745c8a0b1e0f2b55ce05ee618b Gerrit-Change-Number: 14337 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Tue, 01 Oct 2019 23:00:22 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
..
Patch Set 24:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1134
PS24, Line 1134: std::string(
: reinterpret_cast(sidecar_slice.data()), sidecar_slice.size());
Why not std::move(sidecar_slice.ToString()) ?
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc
File be/src/runtime/runtime-filter-bank.cc:
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@174
PS24, Line 174: UpdateFilterParamsPB* params = obj_pool_.Add(new
UpdateFilterParamsPB);
Can you please double check if the parameters need to be preserved beyond this
function ? According to
(https://github.com/apache/impala/blob/master/be/src/kudu/rpc/proxy.h#L79-L84),
it can be freed once the async RPC call is done.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@175
PS24, Line 175: UpdateFilterResultPB* res = obj_pool_.Add(new
UpdateFilterResultPB);
: RpcController* controller = obj_pool_.Add(new
RpcController);
I wonder if we can keep these in thread local storage and initialize them once
at init time. I suppose we don't invoke this RPC more than once per fragment
instance thread, right ? Otherwise, it seems a bit wasteful to keep
accumulating these objects in obj_pool_.
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@320
PS24, Line 320: // Wait for all inflight rpcs to complete before closing the
filters.
: {
: std::unique_lock l1(num_inflight_rpcs_lock_);
: while (num_inflight_rpcs_ > 0) {
: krpcs_done_cv_.wait(l1);
: }
: }
:
: lock_guard l2(runtime_filter_lock_);
: closed_ = true;
Do you need to set closed_ to true before waiting for all in-flight RPCs to
drain ? Otherwise, you may break out of the critical section above and then a
thread may sneak in and try to issues RPC again. Or is it impossible because
it's the same fragment instance thread which called
RuntimeFilterBank::UpdateFilterFromLocal() ?
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/service/data-stream-service.cc@137
PS24, Line 137: Substitute("Query State not found for query_id=$0",
: PrintId(ProtoToQueryId(req->dst_query_id()))
This can be refactored into a single string object and reuse them here and
below in RespondAndReleaseRpc().
http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/service/data-stream-service.cc@141
PS24, Line 141: this->
no need for this
--
To view, visit http://gerrit.cloudera.org:8080/13882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a
Gerrit-Change-Number: 13882
Gerrit-PatchSet: 24
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Wed, 25 Sep 2019 16:58:23 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
..
Patch Set 22:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/util/bloom-filter.h@46
PS22, Line 46: namespace bloom_filter_test_util {
: void BfUnion(const impala::BloomFilter& x, const
impala::BloomFilter& y,
: int64_t directory_size, bool* success,
impala::BloomFilterPB* protobuf,
: std::string* directory);
: } // namespace bloom_filter_test_util
:
: namespace either {
: struct TestData;
: } // namespace either
> Can these be moved to the test file ?
I guess not. Sorry for missing the friend declaration below.
--
To view, visit http://gerrit.cloudera.org:8080/13882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a
Gerrit-Change-Number: 13882
Gerrit-PatchSet: 22
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Tue, 24 Sep 2019 17:52:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port runtime filter from Thrift RPC to KRPC
..
Patch Set 22:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/coordinator-backend-state.cc@565
PS22, Line 565: LOG(WARNING)
LOG(ERROR)
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/coordinator-backend-state.cc@572
PS22, Line 572: LOG(WARNING)
LOG(ERROR)
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/coordinator-backend-state.cc@575
PS22, Line 575: LOG(WARNING)
LOG(ERROR)
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/runtime-filter-bank.cc
File be/src/runtime/runtime-filter-bank.cc:
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/runtime-filter-bank.cc@120
PS22, Line 120: INFO
ERROR
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/runtime-filter-bank.cc@126
PS22, Line 126: std::unique_lock l(num_inflight_rpcs_lock_);
DCHECK_GT(num_inflight_rpcs_, 0);
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/runtime-filter-bank.cc@127
PS22, Line 127: num_inflight_rpcs_--
nit: pre-increment/decrement:
--num_inflight_rpcs_;
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/runtime-filter-bank.cc@201
PS22, Line 201: std::unique_lock l(num_inflight_rpcs_lock_);
Please add a comment on why this is necessary: Increment num_inflight_rpcs_ to
make sure that the filter will not be deallocated in Close() until all
in-flight RPC completes.
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/runtime-filter-bank.cc@202
PS22, Line 202: num_inflight_rpcs_++;
++num_inflight_rpcs_;
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/runtime/runtime-filter-bank.cc@321
PS22, Line 321: std::chrono::milliseconds(50))
In theory, we don't need a timeout here as we need to wait till the RPC is
done. That said, it may not hurt to log a statement after waiting for extended
period of time. 50 ms may be a bit iffy if the network is slow or if there is a
lot of outbound traffic. May be 60s ?
One potential improvement is that we may as well cancel the RPC after waiting
for too long.
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:
http://gerrit.cloudera.org:8080/#/c/13882/22/be/src/util/bloom-filter.h@46
PS22, Line 46: namespace bloom_filter_test_util {
: void BfUnion(const impala::BloomFilter& x, const
impala::BloomFilter& y,
: int64_t directory_size, bool* success,
impala::BloomFilterPB* protobuf,
: std::string* directory);
: } // namespace bloom_filter_test_util
:
: namespace either {
: struct TestData;
: } // namespace either
Can these be moved to the test file ?
--
To view, visit http://gerrit.cloudera.org:8080/13882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a
Gerrit-Change-Number: 13882
Gerrit-PatchSet: 22
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Mon, 23 Sep 2019 20:52:45 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8884: track storage read statistics per queue
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14242 )
Change subject: IMPALA-8884: track storage read statistics per queue
..
Patch Set 1: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/disk-io-mgr.cc@278
PS1, Line 278: 1000L * 1000L * 1000L
NANOS_PER_SEC
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.h
File be/src/runtime/io/local-file-reader.h:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.h@28
PS1, Line 28: public:
nit: fix the indent here too
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc
File be/src/runtime/io/local-file-reader.cc:
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc@64
PS1, Line 64: queue->read_size()->Update(bytes_to_read);
Should we only update read_size() below near where read_timer is in case we hit
various errors below ?
http://gerrit.cloudera.org:8080/#/c/14242/1/be/src/runtime/io/local-file-reader.cc@76
PS1, Line 76: Substitute("Could not seek to $0 "
:"for file: $1: $2",
one line
--
To view, visit http://gerrit.cloudera.org:8080/14242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8233ed02b418f22f1d0c031e378288357796f4b4
Gerrit-Change-Number: 14242
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Ho
Gerrit-Comment-Date: Fri, 20 Sep 2019 23:38:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8878: [DOCS] Query profile export to Json in WebUI
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14276 ) Change subject: IMPALA-8878: [DOCS] Query profile export to Json in WebUI .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14276 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10249f0019f51d8f6511ce2c2436f56a33099967 Gerrit-Change-Number: 14276 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jiawei Wang Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Fri, 20 Sep 2019 23:33:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8878: [DOCS] Query profile export to Json in WebUI
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14276 ) Change subject: IMPALA-8878: [DOCS] Query profile export to Json in WebUI .. Patch Set 1: Please address the comment about thrift profile. -- To view, visit http://gerrit.cloudera.org:8080/14276 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10249f0019f51d8f6511ce2c2436f56a33099967 Gerrit-Change-Number: 14276 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jiawei Wang Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Fri, 20 Sep 2019 23:22:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8878: [DOCS] Query profile export to Json in WebUI
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14276 ) Change subject: IMPALA-8878: [DOCS] Query profile export to Json in WebUI .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/14276/1/docs/topics/impala_webui.xml File docs/topics/impala_webui.xml: http://gerrit.cloudera.org:8080/#/c/14276/1/docs/topics/impala_webui.xml@611 PS1, Line 611: text or Json Also thrift -- To view, visit http://gerrit.cloudera.org:8080/14276 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10249f0019f51d8f6511ce2c2436f56a33099967 Gerrit-Change-Number: 14276 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jiawei Wang Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Fri, 20 Sep 2019 23:22:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8634: Catalog client should retry RPCs
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14246 ) Change subject: IMPALA-8634: Catalog client should retry RPCs .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/14246/4/tests/custom_cluster/test_services_rpc_errors.py File tests/custom_cluster/test_services_rpc_errors.py: http://gerrit.cloudera.org:8080/#/c/14246/4/tests/custom_cluster/test_services_rpc_errors.py@80 PS4, Line 80: Validte typo -- To view, visit http://gerrit.cloudera.org:8080/14246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f33ad2b36d301fb64e70a939e71decab0ca993c Gerrit-Change-Number: 14246 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 18 Sep 2019 22:35:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8634: Catalog client should retry RPCs
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14246 ) Change subject: IMPALA-8634: Catalog client should retry RPCs .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc@144 PS1, Line 144: 0 > Sure, this patch or a separate one? Not sure what an appropriate default fo A separate one makes sense. Please also see https://issues.apache.org/jira/browse/KUDU-2192 -- To view, visit http://gerrit.cloudera.org:8080/14246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f33ad2b36d301fb64e70a939e71decab0ca993c Gerrit-Change-Number: 14246 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 18 Sep 2019 22:33:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 18 Sep 2019 22:27:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8634: Catalog client should retry RPCs
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14246 ) Change subject: IMPALA-8634: Catalog client should retry RPCs .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc@142 PS1, Line 142: DEFINE_int32(catalog_client_connection_num_retries, 3, "The number of times connections " > Yeah, 10 seconds does seem a little long. How about 10 retries, with an int Seems reasonable to me. http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc@144 PS1, Line 144: 0 We may want to consider setting this timeout to non-zero at some point. When we convert the Catalog clients to use KRPC, we may be able to rely on using TCP_USER_TIMEOUT which seems more appropriate than socket timeout. http://gerrit.cloudera.org:8080/#/c/14246/1/be/src/runtime/exec-env.cc@180 PS1, Line 180: 1, 0 May help to name these constants as variables. IIUC, this change relies on the sleep and retry in the retry loop of DoRpcWithRetry() so simply don't try recreating new connections on failure now. -- To view, visit http://gerrit.cloudera.org:8080/14246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f33ad2b36d301fb64e70a939e71decab0ca993c Gerrit-Change-Number: 14246 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 18 Sep 2019 06:52:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py File tests/query_test/test_result_spooling.py: http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py@404 PS2, Line 404: assert 'Expected Failure' > Did you mean to assert 'Expected Failure' in sth.. ? or assert False, "Expected Failure" -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 18 Sep 2019 05:35:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14214 )
Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix
DCHECKs
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14214/2/be/src/exec/buffered-plan-root-sink.h
File be/src/exec/buffered-plan-root-sink.h:
http://gerrit.cloudera.org:8080/#/c/14214/2/be/src/exec/buffered-plan-root-sink.h@147
PS2, Line 147: bool IsQueueEmpty() const {
It would be nice if we could add the following DCHECK:
DCHECK(!IsCancelledOrClosed(state));
http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py@404
PS2, Line 404: assert 'Expected Failure'
Did you mean to assert 'Expected Failure' in sth.. ?
--
To view, visit http://gerrit.cloudera.org:8080/14214
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932
Gerrit-Change-Number: 14214
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Wed, 18 Sep 2019 02:03:04 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8825: Add additional counters to PlanRootSink
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14180 ) Change subject: IMPALA-8825: Add additional counters to PlanRootSink .. Patch Set 5: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/14180/5/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14180/5/be/src/service/client-request-state.h@456 PS5, Line 456: Since : /// it does not track rows read from the cache, this counter also tracks the number of : /// materialized rows, so it is used to calculate row_materialization_rate_ nit: It only counts the number of materialized rows and it is used for deriving the row_materialization_rate_. http://gerrit.cloudera.org:8080/#/c/14180/5/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14180/5/be/src/service/client-request-state.cc@117 PS5, Line 117: num_rows_fetched_cache_counter_ nit: num_rows_fetched_from_cache_counter_ or num_result_cache_rows_counter_ or something simpler http://gerrit.cloudera.org:8080/#/c/14180/5/tests/hs2/test_fetch_first.py File tests/hs2/test_fetch_first.py: http://gerrit.cloudera.org:8080/#/c/14180/5/tests/hs2/test_fetch_first.py@163 PS5, Line 163: def __get_runtime_profile(self, op_handle): : """Helper method to get the runtime profile from a given operation handle.""" : get_profile_req = ImpalaHiveServer2Service.TGetRuntimeProfileReq() : get_profile_req.operationHandle = op_handle : get_profile_req.sessionHandle = self.session_handle : get_profile_resp = self.hs2_client.GetRuntimeProfile(get_profile_req) : HS2TestSuite.check_response(get_profile_resp) : return get_profile_resp.profile Feel free to defer the work but we seem to have similar functions elsewhere (https://github.com/apache/impala/blob/master/tests/query_test/test_observability.py#L77) in other tests so it'd be nice to consolidate them in the future. -- To view, visit http://gerrit.cloudera.org:8080/14180 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e101e2f3e2bf8324e149c780d35825ceecc036 Gerrit-Change-Number: 14180 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 13 Sep 2019 22:29:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14214 )
Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix
DCHECKs
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14214/1/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/14214/1/be/src/exec/buffered-plan-root-sink.cc@175
PS1, Line 175: while (IsQueueClosedOrEmpty() && sender_state_ ==
SenderState::ROWS_PENDING
: && !state->is_cancelled() && !timed_out) {
As discussed offline, the contract seems simpler if we make it an invariant
that IsQueueEmpty() cannot be called after the plan root sink has been
cancelled.
IMHO, it seems a bit weird to still loop around if "the queue is closed" part
is true. It seems to be making assumption that state->is_cancelled() is called
afterwards. IMHO, the new interface IsQueueClosedOrEmpty() seems a tad error
prone as it returns true for two possible states and these two states may
potentially lead to drastically different actions (i.e. keep waiting vs
breaking out of the loop).
--
To view, visit http://gerrit.cloudera.org:8080/14214
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932
Gerrit-Change-Number: 14214
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Comment-Date: Fri, 13 Sep 2019 21:42:33 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8825: Add additional counters to PlanRootSink
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14180 ) Change subject: IMPALA-8825: Add additional counters to PlanRootSink .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/14180/1/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14180/1/be/src/service/client-request-state.h@421 PS1, Line 421: /// The number of rows materialized for this query, the counter is not reset if the : /// fetch is restarted. It does not include any rows read from the results cache, since : /// those rows are already materialized. : RuntimeProfile::Counter* rows_materialized_counter_ = nullptr; > yeah, this is pretty similar to num_rows_fetched_counter_ the difference is It seems a tad confusing to users to interpret these two counters. In essence, these counters only differ if the user somehow restarts the fetch. It may be simpler to just track the number of rows fetched from result cache using a separate counter and have a single counter for number of rows materialized. -- To view, visit http://gerrit.cloudera.org:8080/14180 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e101e2f3e2bf8324e149c780d35825ceecc036 Gerrit-Change-Number: 14180 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 13 Sep 2019 18:36:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8825: Add additional counters to PlanRootSink
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14180 ) Change subject: IMPALA-8825: Add additional counters to PlanRootSink .. Patch Set 1: May also want to rebase and address the TODO (i.e. removing sleep(1)) in https://github.com/apache/impala/blob/master/tests/custom_cluster/test_result_spooling.py#L64-L66 -- To view, visit http://gerrit.cloudera.org:8080/14180 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e101e2f3e2bf8324e149c780d35825ceecc036 Gerrit-Change-Number: 14180 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 10 Sep 2019 18:02:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8825: Add additional counters to PlanRootSink
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14180 ) Change subject: IMPALA-8825: Add additional counters to PlanRootSink .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/14180/1/be/src/exec/plan-root-sink.cc File be/src/exec/plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/14180/1/be/src/exec/plan-root-sink.cc@48 PS1, Line 48: profile_->total_time_counter())); nit: indent seems off http://gerrit.cloudera.org:8080/#/c/14180/1/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14180/1/be/src/service/client-request-state.h@421 PS1, Line 421: /// The number of rows materialized for this query, the counter is not reset if the : /// fetch is restarted. It does not include any rows read from the results cache, since : /// those rows are already materialized. : RuntimeProfile::Counter* rows_materialized_counter_ = nullptr; Doesn't this track closely with num_rows_fetched_counter_ ? Would it be sufficient just to track the row materialization time and num rows fetched only ? -- To view, visit http://gerrit.cloudera.org:8080/14180 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e101e2f3e2bf8324e149c780d35825ceecc036 Gerrit-Change-Number: 14180 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 10 Sep 2019 17:57:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5802: use mt scan node for all formats
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14171 ) Change subject: IMPALA-5802: use mt scan node for all formats .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14171 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a91d2e5c2ebb617b7643cd676cb3490c190a68a Gerrit-Change-Number: 14171 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Fri, 06 Sep 2019 22:17:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8803: Coordinator should release admitted memory per-backend
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14104 ) Change subject: IMPALA-8803: Coordinator should release admitted memory per-backend .. Patch Set 9: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/14104/9/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/14104/9/be/src/scheduling/admission-controller.cc@407 PS9, Line 407: Cannot find host address $0 for query $1. Cannot find exec params of host $0 for query $1 http://gerrit.cloudera.org:8080/#/c/14104/7/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/14104/7/tests/custom_cluster/test_executor_groups.py@236 PS7, Line 236: 3 > This test started failing because they assume a max number of queries can b Thanks for the explanation. I believe you mean the assert at line 264 below, right ? -- To view, visit http://gerrit.cloudera.org:8080/14104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88bb11e0ede7574568020e0277dd8ac8d2586dc9 Gerrit-Change-Number: 14104 Gerrit-PatchSet: 9 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Sep 2019 21:13:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8803: Coordinator should release admitted memory per-backend
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14104 ) Change subject: IMPALA-8803: Coordinator should release admitted memory per-backend .. Patch Set 7: (15 comments) http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-resource-state.cc File be/src/runtime/coordinator-backend-resource-state.cc: http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-resource-state.cc@81 PS7, Line 81: num_pending_releasable_or_released_ Would it be simpler to track the negation (i.e. num_in_use_) instead ? We only need to decrement num_in_use_ here vs bumping this counter at multiple places. http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-resource-state.cc@95 PS7, Line 95: is_coordinator_the_last_unreleased_backend If I understand it correctly, there is no adverse effect for this heuristics even in the case where there are no coordinator fragments, right? http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-resource-state.cc@106 PS7, Line 106: } DCHECK_GE(num_pending_, 0); http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-resource-state.cc@117 PS7, Line 117: DCHECK( DCHECK_NE http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-state.h@457 PS7, Line 457: PENDING state or RELESABLE ? http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-state.h@460 PS7, Line 460: /// released. A no-op if the BackendResourceState is closed already. http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-state.h@465 PS7, Line 465: /// BackendStates have been released. This may be called even after CloseAndGetUnreleasedBackends(). http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/runtime/coordinator-backend-state.h@509 PS7, Line 509: BackendResourceState is closed, May help to clarify what may and may not happen after transitioning to the "closed_" state. http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/scheduling/admission-controller.h@851 PS7, Line 851: specificied typo http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/scheduling/admission-controller.h@852 PS7, Line 852: UpdateStatsForHost( nit: This name is a bit too similar to UpdateHostStats() and they seem to serve similar functions too. I wonder if it's more consistent to just overload UpdateHostStats() or rename it to UpdateHostStatsInternal() ? http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/scheduling/admission-controller.cc@406 PS7, Line 406: DCHECK(false) << strings::Substitute( : "Error: Cannot find host address $0 for query $1.", PrintThrift(host_addr), : PrintId(schedule.query_id())); Shouldn't this be LOG(ERROR) or something in addition to DCHECK(false); ? http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/scheduling/admission-controller.cc@408 PS7, Line 408: PrintId(schedule.query_id())); Should we add a continue; in this case ? http://gerrit.cloudera.org:8080/#/c/14104/7/be/src/scheduling/admission-controller.cc@1531 PS7, Line 1531: } DCHECK_EQ(num_release_backends_.find(schedule->query_id()), num_release_backends_.end()); http://gerrit.cloudera.org:8080/#/c/14104/7/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/14104/7/tests/custom_cluster/test_executor_groups.py@236 PS7, Line 236: 3 Why this change ? http://gerrit.cloudera.org:8080/#/c/14104/7/tests/custom_cluster/test_result_spooling.py File tests/custom_cluster/test_result_spooling.py: http://gerrit.cloudera.org:8080/#/c/14104/7/tests/custom_cluster/test_result_spooling.py@62 PS7, Line 62: self.wait_for_state(handle, self.client.QUERY_STATES['FINISHED'], timeout) Is there a potential race here ? If I understand it correctly, we may reach the 'FINISHED' state after Open() completes in the exec node. For the merging exchange, it may only fetch the first row batch from each sender. So there seems to be a chance which not all results have been spooled yet given the limit 2000. -- To view, visit http://gerrit.cloudera.org:8080/14104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88bb11e0ede7574568020e0277dd8ac8d2586dc9 Gerrit-Change-Number: 14104 Gerrit-PatchSet: 7 Gerrit-Owner: Sahil Takiar
[Impala-ASF-CR] IMPALA-8907: TestResultSpooling.test slow query is flaky
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14170 ) Change subject: IMPALA-8907: TestResultSpooling.test_slow_query is flaky .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idcb4a6b38f85a9497f80e2674e1c1fa512be5940 Gerrit-Change-Number: 14170 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Wed, 04 Sep 2019 19:23:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14101 ) Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14101/4/tests/custom_cluster/test_exchange_eos.py File tests/custom_cluster/test_exchange_eos.py: http://gerrit.cloudera.org:8080/#/c/14101/4/tests/custom_cluster/test_exchange_eos.py@41 PS4, Line 41: test_exchange_eos > Might be worth it to add a few comments about how this test works, eg. why Done -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 26 Aug 2019 21:51:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14101 ) Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. Patch Set 5: Code-Review+2 Carry Thomas' +2 -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 26 Aug 2019 21:51:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Hello Thomas Tauber-Marshall, Lars Volker, Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14101 to look at the new patch set (#5). Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. IMPALA-8845: Cancel receiver's streams on exchange node's EOS When an exchange node reaches its row count limit, the current code will not notify the sender fragments about it. Consequently, sender fragments may keep sending row batches to the exchange node but they won't be dequeued anymore. The sender fragments may end up blocking in the RPC indefinitely until either the query is cancelled or closed. This change fixes the problem above by cancelling the underlying receiver's streams of an exchange node once it reaches the row count limit. This will unblock all senders whose TransmitData() RPCs haven't been replied to yet. Any future row batches sent to this receiver will also be immediately replied to with a response indicating that this receiver is already closed so the sender will stop sending any more row batches to it. Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 --- M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/runtime/krpc-data-stream-mgr.cc M be/src/runtime/krpc-data-stream-recvr.cc M be/src/runtime/krpc-data-stream-recvr.h A tests/custom_cluster/test_exchange_eos.py 6 files changed, 103 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/14101/5 -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-8819: BufferedPlanRootSink should handle non-default fetch sizes
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14129 ) Change subject: IMPALA-8819: BufferedPlanRootSink should handle non-default fetch sizes .. Patch Set 3: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/14129/3/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/14129/3/be/src/exec/buffered-plan-root-sink.h@147 PS3, Line 147: current_batch_ == nullptr !current_batch_; http://gerrit.cloudera.org:8080/#/c/14129/3/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/14129/3/be/src/exec/buffered-plan-root-sink.cc@114 PS3, Line 114: current_batch_ != nullptr) !current_batch_ http://gerrit.cloudera.org:8080/#/c/14129/3/be/src/exec/buffered-plan-root-sink.cc@138 PS3, Line 138: bool* eos Is it assumed that the caller will initialize this to false or should we explicitly set it to false before entering the while loop below ? -- To view, visit http://gerrit.cloudera.org:8080/14129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8dd4b397ab6457a4f85e635f239b2c67130fcce4 Gerrit-Change-Number: 14129 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 26 Aug 2019 16:35:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8819: BufferedPlanRootSink should handle non-default fetch sizes
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14129 )
Change subject: IMPALA-8819: BufferedPlanRootSink should handle non-default
fetch sizes
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h
File be/src/exec/buffered-plan-root-sink.h:
http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h@85
PS2, Line 85: 1024
QueryState::DEFAULT_BATCH_SIZE ?
http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h@136
PS2, Line 136: = nullptr;
nit: unnecessary for unique_ptr
http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h@144
PS2, Line 144: /// 'current_batch' points to.
This needs to be called with 'lock_' held right ?
http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h@145
PS2, Line 145: bool IsQueueEmpty() {
bool IsQueueEmpty const {
http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.cc@143
PS2, Line 143: // If 'num_results' <= 0 then by default fetch BATCH_SIZE
rows.
> I did some profiling, and I think the answer is that it depends on the netw
Thanks for doing the experiment. 10x BATCH_SIZE seems like a reasonable
starting point.
http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.cc@162
PS2, Line 162: if (current_batch_ == nullptr)
if (!current_batch_)
http://www.cplusplus.com/reference/memory/unique_ptr/operator%20bool/
http://gerrit.cloudera.org:8080/#/c/14129/2/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/14129/2/tests/query_test/test_result_spooling.py@264
PS2, Line 264: for _ in range(int(ceil(float(self._num_rows) /
float(fetch_size:
Given the assert at line 268, why not just do:
while rows_fetched < self._num_rows:
--
To view, visit http://gerrit.cloudera.org:8080/14129
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8dd4b397ab6457a4f85e635f239b2c67130fcce4
Gerrit-Change-Number: 14129
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Sat, 24 Aug 2019 01:08:07 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8691: Query option to disable data cache
Michael Ho has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14015 ) Change subject: IMPALA-8691: Query option to disable data cache .. IMPALA-8691: Query option to disable data cache This change adds a query option to disable the data cache for a given session. By default, this option is set to false. When it's set to true, all queries will by-pass the data cache. This allows users to avoid polluting the cache for accesses to tables which they don't want to cache. A follow-up change will add a per-table query hint to allow caching disabled for a given table only. There is some small refactoring in the code to make it clearer the type of caching being referred to in the code. As the code stands now, we have both HDFS caching (for local reads) and the data cache (for remote reads). BufferOpts has been extended to allow users to explicitly state intention for using either/both of the caches. Change-Id: I39122ac38435cedf94b2b39145863764d0b5b6c8 Reviewed-on: http://gerrit.cloudera.org:8080/14015 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M be/src/exec/base-sequence-scanner.cc M be/src/exec/hdfs-orc-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/parquet/hdfs-parquet-scanner.cc M be/src/exec/parquet/parquet-column-readers.cc M be/src/exec/parquet/parquet-page-index.cc M be/src/exec/scan-node.cc M be/src/exec/scan-node.h M be/src/exec/scanner-context.cc M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/hdfs-file-reader.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h M be/src/runtime/io/scan-range.cc M be/src/runtime/tmp-file-mgr.cc M be/src/scheduling/scheduler-test-util.cc M be/src/scheduling/scheduler.cc M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M tests/custom_cluster/test_data_cache.py 25 files changed, 171 insertions(+), 80 deletions(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/14015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I39122ac38435cedf94b2b39145863764d0b5b6c8 Gerrit-Change-Number: 14015 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8803: Coordinator should release admitted memory per-backend
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14104 ) Change subject: IMPALA-8803: Coordinator should release admitted memory per-backend .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@846 PS1, Line 846: last_backend || coordinator_last_backend > My understanding of this part of the code is a bit murky, so correct me if Fair point. I suppose SetNonErrorTerminalState() may not be the right function to call. With result spooling enabled, once the last row has been sent to the plan root sink of the coordinator fragment, we can consider transitioning to a new non-terminal state called EOS which indicates the end of execution. In which case, we can start cancelling all backends and release all backend resources. This may require factoring out some of the logic in HandleExecStateTransition(). We can consider doing this as part of or after IMPALA-6984 -- To view, visit http://gerrit.cloudera.org:8080/14104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88bb11e0ede7574568020e0277dd8ac8d2586dc9 Gerrit-Change-Number: 14104 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 23 Aug 2019 06:36:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14101 ) Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. Patch Set 4: Code-Review+1 Carrying Sahil's +1 -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Thu, 22 Aug 2019 06:09:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/14101 ) Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. IMPALA-8845: Cancel receiver's streams on exchange node's EOS When an exchange node reaches its row count limit, the current code will not notify the sender fragments about it. Consequently, sender fragments may keep sending row batches to the exchange node but they won't be dequeued anymore. The sender fragments may end up blocking in the RPC indefinitely until either the query is cancelled or closed. This change fixes the problem above by cancelling the underlying receiver's streams of an exchange node once it reaches the row count limit. This will unblock all senders whose TransmitData() RPCs haven't been replied to yet. Any future row batches sent to this receiver will also be immediately replied to with a response indicating that this receiver is already closed so the sender will stop sending any more row batches to it. Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 --- M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/runtime/krpc-data-stream-mgr.cc M be/src/runtime/krpc-data-stream-recvr.cc M be/src/runtime/krpc-data-stream-recvr.h A tests/custom_cluster/test_exchange_eos.py 6 files changed, 96 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/14101/4 -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14101 ) Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/14101/2/tests/custom_cluster/test_exchange_eos.py File tests/custom_cluster/test_exchange_eos.py: http://gerrit.cloudera.org:8080/#/c/14101/2/tests/custom_cluster/test_exchange_eos.py@57 PS2, Line 57: results = client.fetch(query, handle) > need to close the query handle? otherwise it will leak, probably not a big Done -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Thu, 22 Aug 2019 06:09:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8818: Replace deque with spillable queue in BufferedPRS
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14039 ) Change subject: IMPALA-8818: Replace deque with spillable queue in BufferedPRS .. Patch Set 8: Code-Review+1 (1 comment) Not super familiar with the resource profile part of the logic so Tim or Bikram may want to double check. http://gerrit.cloudera.org:8080/#/c/14039/8/be/src/runtime/spillable-row-batch-queue.cc File be/src/runtime/spillable-row-batch-queue.cc: http://gerrit.cloudera.org:8080/#/c/14039/8/be/src/runtime/spillable-row-batch-queue.cc@81 PS8, Line 81: // The pin should be stream at this point. : DCHECK(batch_queue_->is_pinned()); It makes me wonder if the logic will be simpler if we just always use an unpinned stream. For the most part, if the result row fits into a single buffer, this is the same as having a pinned stream. -- To view, visit http://gerrit.cloudera.org:8080/14039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10f9e72374cdf9501c0e5e2c5b39c13688ae65a9 Gerrit-Change-Number: 14039 Gerrit-PatchSet: 8 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Aug 2019 22:16:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14101 ) Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/14101/2/be/src/exec/exchange-node.cc File be/src/exec/exchange-node.cc: http://gerrit.cloudera.org:8080/#/c/14101/2/be/src/exec/exchange-node.cc@162 PS2, Line 162: stream_recvr_->CancelStream(); > does this mess up some of the logging done in KrpcDataStreamMgr::Cancel? it Reworded that log statement a bit to clarify the situation. We want to close the receiver without unregistering it so incoming senders won't hang. In fact, the same can already happen today if a fragment instance is first cancelled before being closed. http://gerrit.cloudera.org:8080/#/c/14101/2/tests/custom_cluster/test_exchange_eos.py File tests/custom_cluster/test_exchange_eos.py: http://gerrit.cloudera.org:8080/#/c/14101/2/tests/custom_cluster/test_exchange_eos.py@40 PS2, Line 40: @CustomClusterTestSuite.with_args(cluster_size=9, num_exclusive_coordinators=1) > is there a simpler cluster setup that re-produces this issue? e.g. with a s I tried with a smaller cluster size but couldn't quite reproduce it reliably. http://gerrit.cloudera.org:8080/#/c/14101/2/tests/custom_cluster/test_exchange_eos.py@57 PS2, Line 57: results = client.fetch(query, handle) > nit: validate the fetch was successful and close the query handle? Done -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Tue, 20 Aug 2019 21:51:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Hello Sahil Takiar, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14101 to look at the new patch set (#3). Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. IMPALA-8845: Cancel receiver's streams on exchange node's EOS When an exchange node reaches its row count limit, the current code will not notify the sender fragments about it. Consequently, sender fragments may keep sending row batches to the exchange node but they won't be dequeued anymore. The sender fragments may end up blocking in the RPC indefinitely until either the query is cancelled or closed. This change fixes the problem above by cancelling the underlying receiver's streams of an exchange node once it reaches the row count limit. This will unblock all senders whose TransmitData() RPCs haven't been replied to yet. Any future row batches sent to this receiver will also be immediately replied to with a response indicating that this receiver is already closed so the sender will stop sending any more row batches to it. Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 --- M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/runtime/krpc-data-stream-mgr.cc M be/src/runtime/krpc-data-stream-recvr.cc M be/src/runtime/krpc-data-stream-recvr.h A tests/custom_cluster/test_exchange_eos.py 6 files changed, 95 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/14101/3 -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar
[Impala-ASF-CR] IMPALA-8803: Coordinator should release admitted memory per-backend
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14104 ) Change subject: IMPALA-8803: Coordinator should release admitted memory per-backend .. Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/14104/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14104/1//COMMIT_MSG@24 PS1, Line 24: * Resources are released at most every 1 seconds, this prevents short at most once http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@403 PS1, Line 403: UNRELEASED Will "IN_USE" be slightly clear ? http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@481 PS1, Line 481: RELEASE_BACKEND_STATES_DELAY Please add _MS suffix http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@505 PS1, Line 505: num_not_unreleased_ nits: double-negative may not be very comprehensible. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@823 PS1, Line 823: num_pending_++ Coding convention for Impala recommends pre-increment http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@846 PS1, Line 846: last_backend || coordinator_last_backend It seems a bit ad-hoc to detect these cases here in this function. I wonder if it makes sense to have a new state in coordinator called "ALL_ROWS_PRODUCED / EOS" which is accessible if result spooling is enabled. Once reaching this state, the thread will call SetNonErrorTerminalState() which end up calling HandleExecStateTransition(). In this way, we can handle these cases in ReleaseAdmissionControlResources(), which seems more natural as it marks the end of all non-coordinator fragments. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.cc@370 PS1, Line 370: DCHECK(false) << strings::Substitute( : "Error: Cannot find host address $0 for query $1.", PrintThrift(host_addr), : PrintId(schedule.query_id())); What happens in non-debug builds ? Should we log here instead and add a continue statement ? We can keep the DCHECK(false) though. -- To view, visit http://gerrit.cloudera.org:8080/14104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88bb11e0ede7574568020e0277dd8ac8d2586dc9 Gerrit-Change-Number: 14104 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Aug 2019 18:47:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8691: Query option to disable data cache
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14015 ) Change subject: IMPALA-8691: Query option to disable data cache .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/14015/1/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/14015/1/be/src/runtime/io/request-ranges.h@180 PS1, Line 180: BufferOpts(bool try_hdfs_cache, bool try_data_cache) > Yeah, it would minimise the chance of transposing or passing in the wrong a Done http://gerrit.cloudera.org:8080/#/c/14015/1/tests/custom_cluster/test_data_cache.py File tests/custom_cluster/test_data_cache.py: http://gerrit.cloudera.org:8080/#/c/14015/1/tests/custom_cluster/test_data_cache.py@79 PS1, Line 79: def test_data_cache_disablement(self, vector): > Can we move one or both of the other tests to exhaustive to make up for the Done -- To view, visit http://gerrit.cloudera.org:8080/14015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39122ac38435cedf94b2b39145863764d0b5b6c8 Gerrit-Change-Number: 14015 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Aug 2019 16:24:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8691: Query option to disable data cache
Michael Ho has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/14015 ) Change subject: IMPALA-8691: Query option to disable data cache .. IMPALA-8691: Query option to disable data cache This change adds a query option to disable the data cache for a given session. By default, this option is set to false. When it's set to true, all queries will by-pass the data cache. This allows users to avoid polluting the cache for accesses to tables which they don't want to cache. A follow-up change will add a per-table query hint to allow caching disabled for a given table only. There is some small refactoring in the code to make it clearer the type of caching being referred to in the code. As the code stands now, we have both HDFS caching (for local reads) and the data cache (for remote reads). BufferOpts has been extended to allow users to explicitly state intention for using either/both of the caches. Change-Id: I39122ac38435cedf94b2b39145863764d0b5b6c8 --- M be/src/exec/base-sequence-scanner.cc M be/src/exec/hdfs-orc-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/parquet/hdfs-parquet-scanner.cc M be/src/exec/parquet/parquet-column-readers.cc M be/src/exec/parquet/parquet-page-index.cc M be/src/exec/scan-node.cc M be/src/exec/scan-node.h M be/src/exec/scanner-context.cc M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/hdfs-file-reader.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h M be/src/runtime/io/scan-range.cc M be/src/runtime/tmp-file-mgr.cc M be/src/scheduling/scheduler-test-util.cc M be/src/scheduling/scheduler.cc M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M tests/custom_cluster/test_data_cache.py 25 files changed, 171 insertions(+), 80 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/14015/2 -- To view, visit http://gerrit.cloudera.org:8080/14015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I39122ac38435cedf94b2b39145863764d0b5b6c8 Gerrit-Change-Number: 14015 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8818: Replace deque with spillable queue in BufferedPRS
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14039 ) Change subject: IMPALA-8818: Replace deque with spillable queue in BufferedPRS .. Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/buffered-tuple-stream-test.cc@323 PS7, Line 323: "-1" nit: use the name of the test instead. Same below. http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/spillable-row-batch-queue.h File be/src/runtime/spillable-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/spillable-row-batch-queue.h@100 PS7, Line 100: BufferedTupleStream* batch_queue_; Shouldn't this use unique_ptr ? May be I am missing something but as the code stands now, aren't we leaking 'batch_queue_' ? http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/spillable-row-batch-queue.cc File be/src/runtime/spillable-row-batch-queue.cc: http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/spillable-row-batch-queue.cc@53 PS7, Line 53: batch_queue_ = new Use unique_ptr http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/service/query-options.cc@907 PS7, Line 907: query_options->max_pinned_result_spooling_memory nits: code seems more legible if we factor out these two options into local variables: int64_t max_pinned_mem = ... int64_t max_unpinned_mem = ... http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/service/query-options.cc@921 PS7, Line 921: if (query_options->max_pinned_result_spooling_memory == 0 : && query_options->max_unpinned_result_spooling_memory != 0) May be I mis-read it but isn't this the same check as the one on line 907 ? http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/service/query-options.cc@928 PS7, Line 928: if (query_options->max_unpinned_result_spooling_memory != 0 : && query_options->max_unpinned_result_spooling_memory : < query_options->max_pinned_result_spooling_memory) Same question: isn't this the same as the one at line 912 ? http://gerrit.cloudera.org:8080/#/c/14039/7/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14039/7/common/thrift/ImpalaService.thrift@419 PS7, Line 419: MAX_PINNED_RESULT_SPOOLING_MEMORY = 86 Also, please specify the behavior when setting this to 0. Same for any value < 0. -- To view, visit http://gerrit.cloudera.org:8080/14039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10f9e72374cdf9501c0e5e2c5b39c13688ae65a9 Gerrit-Change-Number: 14039 Gerrit-PatchSet: 7 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 20 Aug 2019 00:49:51 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/14101 ) Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. IMPALA-8845: Cancel receiver's streams on exchange node's EOS When an exchange node reaches its row count limit, the current code will not notify the sender fragments about it. Consequently, sender fragments may keep sending row batches to the exchange node but they won't be dequeued anymore. The sender fragments may end up blocking in the RPC indefinitely until either the query is cancelled or closed. This change fixes the problem above by cancelling the underlying receiver's streams of an exchange node once it reaches the row count limit. This will unblock all senders whose TransmitData() RPCs haven't been replied to yet. Any future row batches sent to this receiver will also be immediately replied to with a response indicating that this receiver is already closed so the sender will stop sending any more row batches to it. Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 --- M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/runtime/krpc-data-stream-recvr.cc M be/src/runtime/krpc-data-stream-recvr.h A tests/custom_cluster/test_exchange_eos.py 5 files changed, 91 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/14101/2 -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sahil Takiar
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14101 ) Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/14101/1/tests/custom_cluster/test_exchange_eos.py File tests/custom_cluster/test_exchange_eos.py: http://gerrit.cloudera.org:8080/#/c/14101/1/tests/custom_cluster/test_exchange_eos.py@22 PS1, Line 22: from tests.verifiers.metric_verifier import MetricVerifier > flake8: F401 'tests.common.test_vector.ImpalaTestDimension' imported but un Done http://gerrit.cloudera.org:8080/#/c/14101/1/tests/custom_cluster/test_exchange_eos.py@25 PS1, Line 25: class TestExchangeEos(CustomClusterTestSuite): > flake8: E302 expected 2 blank lines, found 1 Done -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Mon, 19 Aug 2019 21:04:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8845: Cancel receiver's streams on exchange node's EOS
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14101 Change subject: IMPALA-8845: Cancel receiver's streams on exchange node's EOS .. IMPALA-8845: Cancel receiver's streams on exchange node's EOS When an exchange node reaches its row count limit, the current code will not notify the sender fragments about it. Consequently, sender fragments may keep sending row batches to the exchange node but they won't be dequeued anymore. The sender fragments may end up blocking in the RPC indefinitely until either the query is cancelled or closed. This change fixes the problem above by cancelling the underlying receiver's streams of an exchange node once it reaches the row count limit. This will unblock all senders whose TransmitData() RPCs haven't been replied to yet. Any future row batches sent to this receiver will also be immediately replied to with a response indicating that this receiver is already closed so the sender will stop sending any more row batches to it. Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 --- M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/runtime/krpc-data-stream-recvr.cc M be/src/runtime/krpc-data-stream-recvr.h A tests/custom_cluster/test_exchange_eos.py 5 files changed, 91 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/14101/1 -- To view, visit http://gerrit.cloudera.org:8080/14101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I10c805e9d63ed8af9f458bf71e8ef5ea9376b939 Gerrit-Change-Number: 14101 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho
[Impala-ASF-CR] IMPALA-8818: Replace deque with spillable queue in BufferedPRS
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14039 )
Change subject: IMPALA-8818: Replace deque with spillable queue in BufferedPRS
..
Patch Set 4:
(10 comments)
LGTM. Mostly minor comments for the first pass. Will do a second pass again
today.
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/buffered-plan-root-sink.h
File be/src/exec/buffered-plan-root-sink.h:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/buffered-plan-root-sink.h@49
PS4, Line 49: Intializes
typo
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/buffered-plan-root-sink.cc@19
PS4, Line 19: #include "runtime/deque-row-batch-queue.h"
Can be deleted ?
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/exec-node.cc@373
PS4, Line 373: node_type_name + " (id=" + std::to_string(id_) + ")"
Use substitute for better legibility.
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/buffered-tuple-stream.h
File be/src/runtime/buffered-tuple-stream.h:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/buffered-tuple-stream.h@223
PS4, Line 223: const std::string
Did you mean to pass-by-value so the compiler will do copy-elison ?
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/buffered-tuple-stream.cc
File be/src/runtime/buffered-tuple-stream.cc:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/buffered-tuple-stream.cc@212
PS4, Line 212: string caller_label
Doesn't match the signature in header
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/deque-row-batch-queue.h
File be/src/runtime/deque-row-batch-queue.h:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/deque-row-batch-queue.h@33
PS4, Line 33: class DequeRowBatchQueue {
Can this class and files be deleted after this patch ?
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/sorter.h
File be/src/runtime/sorter.h:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/sorter.h@210
PS4, Line 210: //
///
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.h
File be/src/runtime/spillable-row-batch-queue.h:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.h@57
PS4, Line 57: std::string
nit: std::string&
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.h@107
PS4, Line 107: std::string
nit: std::string& name
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.cc
File be/src/runtime/spillable-row-batch-queue.cc:
http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.cc@101
PS4, Line 101: DCHECK((eos && IsEmpty()) || (!eos && !IsEmpty()));
DCHECK_EQ(eos, IsEmpty());
--
To view, visit http://gerrit.cloudera.org:8080/14039
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I10f9e72374cdf9501c0e5e2c5b39c13688ae65a9
Gerrit-Change-Number: 14039
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 15 Aug 2019 18:47:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8685,IMPALA-8677: Use consistent scheduling for small clusters
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14026 ) Change subject: IMPALA-8685,IMPALA-8677: Use consistent scheduling for small clusters .. Patch Set 5: Code-Review+1 (1 comment) I will let Lars finish off the review. http://gerrit.cloudera.org:8080/#/c/14026/5/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/14026/5/be/src/scheduling/scheduler.cc@781 PS5, Line 781: set Can this be an unordered_set ? It doesn't look like we rely on the order anymore, right ? -- To view, visit http://gerrit.cloudera.org:8080/14026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icfdb2cc53d7206e316ea8a1cc28ad443f246f741 Gerrit-Change-Number: 14026 Gerrit-PatchSet: 5 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Tue, 13 Aug 2019 23:19:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8691: Query option to disable data cache
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14015 ) Change subject: IMPALA-8691: Query option to disable data cache .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14015/1/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/14015/1/be/src/runtime/io/request-ranges.h@180 PS1, Line 180: BufferOpts(bool try_hdfs_cache, bool try_data_cache) > Can we use enums or a bitmask or something? Two booleans related to caching Using a bitmask doesn't hide the complexity of having two caches. Callers still have to make the call on which if any of the two caches it should use. Eventually, the two caches should converge but that's a bigger change. That said, if you think using a bitmask / enum makes things less confusing, I am happy to change it. USE_HDFS_CACHE | USE_DATA_CACHE -- To view, visit http://gerrit.cloudera.org:8080/14015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39122ac38435cedf94b2b39145863764d0b5b6c8 Gerrit-Change-Number: 14015 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 08 Aug 2019 21:46:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5149: Provide query profile in JSON format
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 ) Change subject: IMPALA-5149: Provide query profile in JSON format .. Patch Set 14: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1 Gerrit-Change-Number: 13801 Gerrit-PatchSet: 14 Gerrit-Owner: Jiawei Wang Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: David Knupp Gerrit-Reviewer: David Rorke Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jiawei Wang Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 07 Aug 2019 17:01:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Fix TestResultSpooling::test multi batches
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14022 ) Change subject: IMPALA-8781: Fix TestResultSpooling::test_multi_batches .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I939eedba37003f5c720cea96e5c3532e2cc6312c Gerrit-Change-Number: 14022 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 06 Aug 2019 21:56:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5149: Provide query profile in JSON format
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13801 )
Change subject: IMPALA-5149: Provide query profile in JSON format
..
Patch Set 13:
(14 comments)
LGTM. Some more minor nits. The patch can be +2'ed after they are addressed.
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/service/impala-server.cc@848
PS13, Line 848: $1
offset: $1
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/service/impala-server.cc@2026
PS13, Line 2026: json_profile_rapid(
json_profile. No need for "_rapid".
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc@1166
PS13, Line 1166: RuntimeProfile* profile_a2 = RuntimeProfile::Create(,
"ProfileAb");
The name of the profile should probably match the variable name.
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc@1204
PS13, Line 1204: for (rapidjson::Value::ConstValueIterator itr =
content["counters"].Begin();
: itr != content["counters"].End(); ++itr) {
Use iterator style like the following. Same at other places.
for (auto& itr : content) {
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile-test.cc@1218
PS13, Line 1218: }
Do we want to check there are no other counter types in content ?
if (is normal counter) {
elif (is high water mark counter) {
} else {
DCHECK(false) ?
}
or
DCHECK(counter is either normal counter || counters is high watermark counter)
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.h@622
PS13, Line 622: get the counter detailed information based on its name
A map of counter's name to counter
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.h@625
PS13, Line 625: child_counter_map
This needs to be documented too.
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/13801/13/be/src/util/runtime-profile.cc@704
PS13, Line 704: the other information
What does all the other information mean ?
http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@571
PS13, Line 571:download_link =
"query_profile_plain_text?query_id={0}".format(
: query_id)
nit: one line
http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@587
PS13, Line 587: self.ROOT_URL + download_link, query,
self.IMPALAD_TEST_PORT)
nit: indent 4 for line continuation
http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@594
PS13, Line 594: Download
Downloaded ?
http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@595
PS13, Line 595: Response text
Json pofile
http://gerrit.cloudera.org:8080/#/c/13801/13/tests/webserver/test_web_pages.py@597
PS13, Line 597: assert query_id in json_res["contents"]["profile_name"]
assert query_id in json_res["contents"]["profile_name"], json_res to help
debugging in case the assert fires.
http://gerrit.cloudera.org:8080/#/c/13801/13/www/query_profile.tmpl
File www/query_profile.tmpl:
http://gerrit.cloudera.org:8080/#/c/13801/13/www/query_profile.tmpl@28
PS13, Line 28: (Choose Format)
(available formats):
--
To view, visit http://gerrit.cloudera.org:8080/13801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 13
Gerrit-Owner: Jiawei Wang
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: David Rorke
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jiawei Wang
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Tue, 06 Aug 2019 01:32:27 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8691: Query option to disable data cache
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14015 Change subject: IMPALA-8691: Query option to disable data cache .. IMPALA-8691: Query option to disable data cache This change adds a query option to disable the data cache for a given session. By default, this option is set to false. When it's set to true, all queries will by-pass the data cache. This allows users to avoid polluting the cache for accesses to tables which they don't want to cache. A follow-up change will add a per-table query hint to allow caching disabled for a given table only. There is some small refactoring in the code to make it clearer the type of caching being referred to in the code. As the code stands now, we have both HDFS caching (for local reads) and the data cache (for remote reads). BufferOpts has been extended to allow users to explicitly state intention for using either of the caches. Change-Id: I39122ac38435cedf94b2b39145863764d0b5b6c8 --- M be/src/exec/base-sequence-scanner.cc M be/src/exec/hdfs-orc-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/parquet/hdfs-parquet-scanner.cc M be/src/exec/parquet/parquet-column-readers.cc M be/src/exec/parquet/parquet-page-index.cc M be/src/exec/scan-node.cc M be/src/exec/scan-node.h M be/src/exec/scanner-context.cc M be/src/runtime/io/disk-io-mgr-stress.cc M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/hdfs-file-reader.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h M be/src/runtime/io/scan-range.cc M be/src/runtime/tmp-file-mgr.cc M be/src/scheduling/scheduler-test-util.cc M be/src/scheduling/scheduler.cc M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M tests/custom_cluster/test_data_cache.py 26 files changed, 157 insertions(+), 78 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/14015/1 -- To view, visit http://gerrit.cloudera.org:8080/14015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I39122ac38435cedf94b2b39145863764d0b5b6c8 Gerrit-Change-Number: 14015 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho
[Impala-ASF-CR] IMPALA-7984: Port UpdateFilter() and PublishFilter() to KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13882 ) Change subject: IMPALA-7984: Port UpdateFilter() and PublishFilter() to KRPC .. Patch Set 12: A quick high level reminder about using sidecar for the filter: - the RPC framework assumes the lifetime of the binary buffer referenced by the sidecar is valid till the end of an RPC. If we used to copy the entire filter as a string into the Thrift data structure, it's okay for the filter to disappear / freed before the RPC completes. However, with RPC sidecar, we need to keep the filter alive until the RPC is done. - the KRPC also implements the functionality to cancel an asynchronous in-flight RPC. Not that it's necessarily needed in this patch but that functionality allows the RPC to end early so that the binary blob referred to by sidecar can be freed once the completion callback is invoked. -- To view, visit http://gerrit.cloudera.org:8080/13882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a Gerrit-Change-Number: 13882 Gerrit-PatchSet: 12 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 05 Aug 2019 18:18:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5149: Provide query profile in JSON format
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13801 )
Change subject: IMPALA-5149: Provide query profile in JSON format
..
Patch Set 12:
(9 comments)
We don't really enforce any compatibility for our runtime profile in general
now so counters may get added and deleted across releases. So, the json profile
output may change as Impala code changes.
In the future, when we have a better compatibility story for our profile, it
may make sense to have a version string in the output.
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/service/impala-server.cc@845
PS12, Line 845: if (!parse_ok){
The header comment says the output stream is not modified on error. May be
worth double checking that json_output not modified on parse error.
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.h@618
PS12, Line 618: void ToJsonCounters(rapidjson::Value* parent,
rapidjson::Document* d,
: const string& counter_name, const CounterMap& counter_map,
: const ChildCounterMap& child_counter_map) const;
Please document each of the parameters.
http://gerrit.cloudera.org:8080/#/c/13801/11/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
http://gerrit.cloudera.org:8080/#/c/13801/11/be/src/util/runtime-profile.h@126
PS11, Line 126: Get the actual Counter derived type
Returns the name of the counter type.
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc@766
PS12, Line 766: _rapid
I noticed that a lot of the Value variables have the "_rapid" suffix. Is
"_json" a more appropriate suffix ?
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc@784
PS12, Line 784: info_strings_.find(key)->second.
DCHECK(info_strings_.find(key) != info_strings_.end());
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc@802
PS12, Line 802:
nit: unnecessary blank line
http://gerrit.cloudera.org:8080/#/c/13801/12/be/src/util/runtime-profile.cc@869
PS12, Line 869:
nit: unnecessary blank line
http://gerrit.cloudera.org:8080/#/c/13801/12/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
http://gerrit.cloudera.org:8080/#/c/13801/12/tests/webserver/test_web_pages.py@563
PS12, Line 563: ("Text", "Json")
["Text", "Json"]
http://gerrit.cloudera.org:8080/#/c/13801/12/tests/webserver/test_web_pages.py@563
PS12, Line 563: download_format
profile_format
--
To view, visit http://gerrit.cloudera.org:8080/13801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1
Gerrit-Change-Number: 13801
Gerrit-PatchSet: 12
Gerrit-Owner: Jiawei Wang
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: David Rorke
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jiawei Wang
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Fri, 02 Aug 2019 07:08:45 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 ) Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283 Gerrit-Change-Number: 13907 Gerrit-PatchSet: 8 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Fri, 02 Aug 2019 04:43:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7984: Port UpdateFilter() and PublishFilter() to KRPC
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port UpdateFilter() and PublishFilter() to KRPC
..
Patch Set 11:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h
File be/src/runtime/backend-client.h:
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h@19
PS11, Line 19: #define IMPALA_BACKEND_CLIENT_H
Should this file be deleted after this change ?
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/data-stream-service.cc@136
PS11, Line 136: "qs.get() == nullptr";
Is it a case in which it's legitimate for query state to be not found here ?
This error message could be clearer:
Query State not found for .
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-internal-service.h
File be/src/service/impala-internal-service.h:
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-internal-service.h@18
PS11, Line 18: #ifndef IMPALA_SERVICE_IMPALA_INTERNAL_SERVICE_H
Shouldn't this file be deleted after this change ?
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-internal-service.cc
File be/src/service/impala-internal-service.cc:
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-internal-service.cc@36
PS11, Line 36: ImpalaInternalService::ImpalaInternalService() {
Same question. Shouldn't this file be deleted after this change ? Also, we can
also skip initializing the backend service Thrift server during Impala
initialization.
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-server.h@342
PS11, Line 342: /// ImpalaInternalService rpcs
Stale comment. ImpalaInternalService should not exist anymore after this change.
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:
http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/util/bloom-filter.h@79
PS11, Line 79: Thrift representation.
Stale comment
--
To view, visit http://gerrit.cloudera.org:8080/13882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a
Gerrit-Change-Number: 13882
Gerrit-PatchSet: 11
Gerrit-Owner: Fang-Yu Rao
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Thu, 01 Aug 2019 20:30:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and
cancellation
..
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
File testdata/workloads/functional-query/queries/QueryTest/result-spooling.test:
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@21
PS6, Line 21: presered
preserved
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@62
PS6, Line 62: avg(int_col)
May want to double check but will the following exercise the expression
evaluation logic more in plan root sink ?
select avg(int_col) + avg(bigint_col) from ...;
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@64
PS6, Line 64: "%s returned a different number of " \
:"results when
result spooling was " \
:"enabled" % query
We may not be very consistent on this but some code review comments in the past
suggested me to use format instead (similar to Substitute in our C++ code). I
suppose it's not as type sensitive (%s vs %d ?).
"{0} return ...".format(query)
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@67
PS6, Line 67: "%s returned different results when result " \
: "spooling was enabled" %
query
I seem to recall that the assert will print out the values which trigger the
failure. Can you please double check if that's the case ? If not, we may wanna
include the result sets in the error message for debugging purposes.
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@96
PS6, Line 96:
Do we also add a test case with result caching enabled ?
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py
File tests/util/cancel_util.py:
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@26
PS6, Line 26: The
nit: long line
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@37
PS6, Line 37: def fetch_results():
: threading.current_thread().fetch_results_error = None
: threading.current_thread().query_profile = None
: try:
: new_client = ImpalaTestSuite.create_impala_client()
: new_client.fetch(query, handle)
: except ImpalaBeeswaxException as e:
: threading.current_thread().fetch_results_error = e
nit: not your change but it may be slightly easier to read if this function is
not nested
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@64
PS6, Line 64: # Before accessing fetch_results_error we need to join the
fetch thread
: thread.join()
Aren't we calling thread join twice if 'join_before_close' is True ?
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 6
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Thu, 01 Aug 2019 01:26:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 ) Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl .. Patch Set 17: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 17 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 31 Jul 2019 00:53:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 ) Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/13907/4/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test File testdata/workloads/functional-query/queries/QueryTest/result-spooling.test: http://gerrit.cloudera.org:8080/#/c/13907/4/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@69 PS4, Line 69: queriess typo http://gerrit.cloudera.org:8080/#/c/13907/4/tests/query_test/test_result_spooling.py File tests/query_test/test_result_spooling.py: http://gerrit.cloudera.org:8080/#/c/13907/4/tests/query_test/test_result_spooling.py@27 PS4, Line 27: 'select * from lineitem order by l_orderkey' I suppose this generates more than row batches, right ? Should this also be run in the context of non-cancellation test ? -- To view, visit http://gerrit.cloudera.org:8080/13907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283 Gerrit-Change-Number: 13907 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Tue, 30 Jul 2019 07:25:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 ) Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl .. Patch Set 15: Code-Review+1 (2 comments) Not sure if Tim wants to do another pass ? http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/plan-root-sink.h File be/src/exec/plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/plan-root-sink.h@94 PS15, Line 94: Check UpdateAndCheck ? http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/runtime/blocking-row-batch-queue.h File be/src/runtime/blocking-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/runtime/blocking-row-batch-queue.h@48 PS15, Line 48: Adds the following timers to the RuntimeProfile: 'RowBatchQueueGetWaitTime' and : /// 'RowBatchQueuePutWaitTime' Stale ? -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 15 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 30 Jul 2019 07:14:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8534: data cache for dockerised tests
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13934 ) Change subject: IMPALA-8534: data cache for dockerised tests .. Patch Set 6: Code-Review+2 Nice. -- To view, visit http://gerrit.cloudera.org:8080/13934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c75d4a5c1eea7a540d051bb175537163dec0e29 Gerrit-Change-Number: 13934 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Mon, 29 Jul 2019 21:06:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and
BufferedPRS impl
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@123
PS11, Line 123: // For now, if num_results < batch->num_rows(), we
terminate returning results
: // early.
: if (num_results > 0 && num_results < batch->num_rows()) {
> I think we should avoid CHECK on codepaths that you could get to by togglin
Makes sense. CHECK() seems a bit an overkill. Sorry for the bad suggestion.
--
To view, visit http://gerrit.cloudera.org:8080/13883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be
Gerrit-Change-Number: 13883
Gerrit-PatchSet: 11
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 29 Jul 2019 20:56:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and
BufferedPRS impl
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@123
PS11, Line 123: // For now, if num_results < batch->num_rows(), we
terminate returning results
: // early.
: if (num_results > 0 && num_results < batch->num_rows()) {
> > the case in which the total number of result rows is not a multiple of ro
Isn't passing different fetch size possible if the result cache is enabled ? In
general, this seems to be making very big assumption about what the client will
pass in. Not sure if this is exactly a good practice.
I suppose my point is that we should fail stop or at least indicate some sort
of failures to the clients for this known to be wrong case in the code. CHECK()
is the fail-stop approach, which seems acceptable as this sink type is not
anticipated to be used. An alternative would be to log this error case and
return an error status to the clients.
--
To view, visit http://gerrit.cloudera.org:8080/13883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be
Gerrit-Change-Number: 13883
Gerrit-PatchSet: 11
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 29 Jul 2019 19:48:52 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and
BufferedPRS impl
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@123
PS11, Line 123: // For now, if num_results < batch->num_rows(), we
terminate returning results
: // early.
: if (num_results > 0 && num_results < batch->num_rows()) {
> Yeah, we wouldn't publicly tell users to set SPOOL_QUERY_RESULTS=true until
I am more worried about the case in which the total number of result rows is
not a multiple of row batch size. The client may call GetNext() multiple times.
In which case, the client may have incomplete result, which arguably is more
confusing than returning no result at all.
I am a bit hesitant for checking in like this. At the minimum, we should add a
CHECK(false) << "Not implemented"; in the if-stmt body.
--
To view, visit http://gerrit.cloudera.org:8080/13883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be
Gerrit-Change-Number: 13883
Gerrit-PatchSet: 11
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 29 Jul 2019 04:58:37 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8339: Add local executor blacklist to coordinators
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13868 ) Change subject: IMPALA-8339: Add local executor blacklist to coordinators .. Patch Set 6: Code-Review+1 (3 comments) Not sure if Lars wants to take one more pass ? http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.h File be/src/scheduling/executor-blacklist.h: http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.h@43 PS6, Line 43: WHen When http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.cc File be/src/scheduling/executor-blacklist.cc: http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.cc@191 PS6, Line 191: 1.2 This can be a variable for clarity. http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/statestore/statestore.h@183 PS6, Line 183: time Please specify the time unit. -- To view, visit http://gerrit.cloudera.org:8080/13868 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacb6e73b84042c33cd475b82470a975d04ee9b74 Gerrit-Change-Number: 13868 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Sat, 27 Jul 2019 00:21:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 ) Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl .. Patch Set 12: (2 comments) http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@103 PS11, Line 103: DataSink::Close(state); : } : > Yeah, for the slow path I guess it doesn't matter much, but shouldn't there Yes, that's s the assumption now but that could change in the future. NotifyAll() here should work in either cases. It won't be fun debugging why users of this type of sink can sometimes hang. http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@123 PS11, Line 123: // If the query was cancelled while the sink was waiting for rows to become available, : // or if the query was cancelled before the current call to GetNext, set eos and then : // return. The queue could be empty if the sink was closed > You do, I just decided to defer this part to a future patch. Added a TODO t May be I am misunderstanding the details but isn't the TODO part a core functionally ? Leaving this part out seems to mean this sink is broken / incomplete. -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 12 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 26 Jul 2019 18:42:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and
BufferedPRS impl
..
Patch Set 11:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.h
File be/src/exec/buffered-plan-root-sink.h:
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.h@64
PS11, Line 64: //
nit: ///
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@27
PS11, Line 27: 10
This should be a static constant
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@54
PS11, Line 54: while (batch_queue_->IsFull()) {
: is_full_.Wait(l);
: RETURN_IF_CANCELLED(state);
: }
May hang for a while if the sink is already cancelled when we get here, it may
be more safer to do:
while (!state->cancelled() && batch_queue->IsFull()) {
is_full_.Wait(l);
}
RETURN_IF_CANCELLED(state);
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@103
PS11, Line 103: rows_available_.NotifyOne();
: consumer_eos_.NotifyOne();
: is_full_.NotifyOne();
I think keeping NotifyAll() make sense for the cancellation path. Same for
Close(). NotifyOne() makes assumption about the number of waiters on these
condition variables, which seems dicey for the cancellation path and it doesn't
hurt to do a NotifyAll() for the slow path.
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@123
PS11, Line 123: // For now, if num_results < batch->num_rows(), we
terminate returning results
: // early.
: if (num_results > 0 && num_results < batch->num_rows()) {
Not sure I understand this part ? Shouldn't this function still need to insert
up to 'num_results' rows into 'results' in this case ?
In other words, if the row batch at the front of the queue is not completely
consumed, don't you need to track how many rows have been consumed from it ?
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@127
PS11, Line 127: consumer_eos_.NotifyOne();
May need to do is_full_.NotifyOne() here in case the sender was blocked. Seems
better to refactor this code (if possible) to always return at line 147.
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/scan-node.cc
File be/src/exec/scan-node.cc:
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/scan-node.cc@262
PS11, Line 262: ADD_TIMER(parent->runtime_profile(),
"RowBatchQueueGetWaitTime"),
: ADD_TIMER(parent->runtime_profile(),
"RowBatchQueuePutWaitTime"))
The code seems easier to follow if we keep the ADD_TIMER() macro at the old
call sites and simply reference those timers here.
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/util/blocking-queue.h@108
PS11, Line 108: get_wait_timer_)
get_wait_timer_ != nullptr
--
To view, visit http://gerrit.cloudera.org:8080/13883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be
Gerrit-Change-Number: 13883
Gerrit-PatchSet: 11
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 26 Jul 2019 02:31:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8339: Add local executor blacklist to coordinators
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13868 ) Change subject: IMPALA-8339: Add local executor blacklist to coordinators .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.cc File be/src/scheduling/executor-blacklist.cc: http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.cc@29 PS3, Line 29: 0 disables blacklisting > My problem with setting it to 120 is that it makes it possible for users to I see. May be it's simpler to not have this padding flag at all and use a constant padding percentage instead (e.g. 20 ~ 30%). The consequence of having a padding value too small is that we may end up removing a bad executor from a node too quickly. However, this is not catastrophic once we implement the transparent query retry in the follow-up patch, right ? I am okay with a a flag which disables blacklisting in case something goes tremendously wrong. -- To view, visit http://gerrit.cloudera.org:8080/13868 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacb6e73b84042c33cd475b82470a975d04ee9b74 Gerrit-Change-Number: 13868 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Fri, 26 Jul 2019 02:30:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5149: Provide query profile in JSON format
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13801 )
Change subject: IMPALA-5149: Provide query profile in JSON format
..
Patch Set 9:
(23 comments)
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-hs2-server.cc@988
PS9, Line 988: }else {
nit: missing blank space after }
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@101
PS9, Line 101: #include
: #include
: #include
: #include
Why not group these together with other rapidjson #include above ? There seems
to be some duplicates.
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@783
PS9, Line 783: DCHECK(format == TRuntimeProfileFormat::STRING);
DCHECK_EQ(format, TRuntimeProfileFormat::STRING);
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@818
PS9, Line 818: DCHECK(format == TRuntimeProfileFormat::STRING);
DCHECK_EQ(format, TRuntimeProfileFormat::STRING);
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@126
PS9, Line 126: void ToJson(rapidjson::Document& document, rapidjson::Value*
val) const {
void ToJson(rapidjson::Document& document, rapidjson::Value* val) const
override {
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@131
PS9, Line 131: val->AddMember("current_value", current_value(),
document.GetAllocator());
Actually, do you need to output this in json ? It seems sufficient to just call
Counter::ToJson() which should add the higher water mark stored in "value_" ?
"current_value_" seems to be internal state used for supporting TryAdd() only.
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@165
PS9, Line 165: void ToJson(rapidjson::Document& document, rapidjson::Value*
val) const {
: Counter::ToJson(document, val);
: rapidjson::Value::MemberIterator type_itr =
val->FindMember("kind");
: DCHECK(type_itr != val->MemberEnd());
: type_itr->value.SetString("DerivedCounter");
: val->AddMember("counter_fn_val", counter_fn_(),
document.GetAllocator());
: }
If you take the suggestion in Counter::ToJson(), you probably don't need this
function.
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@219
PS9, Line 219: rapidjson::Value::MemberIterator type_itr =
val->FindMember("kind");
: DCHECK(type_itr != val->MemberEnd());
: type_itr->value.SetString("AveragedCounter");
See comments in Counter::ToJson() on why we may not need this.
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@222
PS9, Line 222: if (unit_ == TUnit::type::DOUBLE_VALUE){
: val->AddMember("current_double_sum", current_double_sum_,
document.GetAllocator());
: } else {
: val->AddMember("current_int_sum", current_int_sum_,
document.GetAllocator());
: }
Same question as above. Do you need to store the internal states (i.e. sum) in
the json output object ? Isn't "average" the only state we need ?
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@290
PS9, Line 290: void ToJson(rapidjson::Document& document, rapidjson::Value*
val) const {
virtual override
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@292
PS9, Line 292: val->RemoveMember("kind");
Why is "kind" removed ? May be worth adding a comment here.
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@406
PS9, Line 406: void ToJson(rapidjson::Document& document, rapidjson::Value*
value);
Please add a comment about the output format.
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@452
PS9, Line 452: void ToJson(rapidjson::Document& document, rapidjson::Value*
val);
It'd be nice to add a comment about the output format.
http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@583
PS9, Line 583: void ToJson(rapidjson::Document& document, rapidjson::Value*
val) const {
: Counter::ToJson(document, val);
: rapidjson::Value::MemberIterator type_itr =
val->FindMember("kind");
: type_itr->value.SetString("ConcurrentTimerCounter");
: val->AddMember("concurrent_stop_watch_total_time",
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 ) Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl .. Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/exec/buffered-plan-root-sink.cc@121 PS9, Line 121: *eos = true; Are you missing a consumer_eos_.NotifyAll() here ? http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/exec/plan-root-sink.h File be/src/exec/plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/exec/plan-root-sink.h@84 PS9, Line 84: validation rules a May be helpful to spell out the details here. This function seems to be a bit of a misnomer as it does a couple of things: - validate the collection slots - update the number of row batches sent to this sink - check if the number of rows sent to this sink exceeds limit and if so, return error status Not sure what a good name is. ValidateBatchAndCheckLimit() ?? http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/exec/plan-root-sink.h@84 PS9, Line 84: approriate typo http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/exec/plan-root-sink.h@107 PS9, Line 107: Send() stale http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/runtime/blocking-row-batch-queue.cc File be/src/runtime/blocking-row-batch-queue.cc: http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/runtime/blocking-row-batch-queue.cc@66 PS9, Line 66: row_batches_put_timer_->Set(batch_queue_->total_put_wait_time()); : row_batches_get_timer_->Set(batch_queue_->total_get_wait_time()); I believe the TODO has more to do with not using this pattern of setting the timer at the end. Instead of waiting till the end to set the timer, it may be better for the batch_queue_ to directly use the two timers so we don't have to wait until the scan node is closed before we can tell how much time is spent in the queue. http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/runtime/deque-row-batch-queue.h File be/src/runtime/deque-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/runtime/deque-row-batch-queue.h@57 PS9, Line 57: remanining typo. Also this line doesn't quite parse correctly. http://gerrit.cloudera.org:8080/#/c/13883/9/be/src/runtime/deque-row-batch-queue.h@68 PS9, Line 68: int max_batches_; const -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 9 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 25 Jul 2019 01:15:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8339: Add local executor blacklist to coordinators
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13868 )
Change subject: IMPALA-8339: Add local executor blacklist to coordinators
..
Patch Set 3:
(19 comments)
Looking good.
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/admission-controller.cc@737
PS3, Line 737: std::s
nit: std:: not needed
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/cluster-membership-mgr.cc@272
PS3, Line 272: already be on the blacklist or probation.
This may be a case worth double checking. If a node is restarted and
re-register with the Statestore, is it guaranteed that the "delete" update will
happen before the "create" update ? Can updates arrive out of order ?
If it's somehow possible that the node being added is still on blacklist. It
may be safer to call FindAndRemove() unconditionally and log it if we happen to
find it on the blacklist in the "create" path.
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/cluster-membership-mgr.cc@273
PS3, Line 273: DCHECK
DCHECK_EQ
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/cluster-membership-mgr.cc@350
PS3, Line 350: return;
Given the for-loop above, can an executor belong to more than one executor
groups ? If so, why is it okay to return early if it's not found in only one
of the groups ?
Also, may help to comment on the intention for doing this check. I suspect it's
to skip the rather heavy weight copying of the membership state below.
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/cluster-membership-mgr.cc@355
PS3, Line 355: recovering_membership_.get() != nullptr
This is repeated at multiple places in this function. May make sense to factor
it out to a local variable for clarity.
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.h
File be/src/scheduling/executor-blacklist.h:
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.h@32
PS3, Line 32: Remove()
FindAndRemove()
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.h@38
PS3, Line 38: When a blacklisted executor has passed this timeout
: /// and Maintenance() is called, the executor is put on
'probation'
May help to also document what if an executor on blacklist is removed during
cluster membership updates.
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.h@42
PS3, Line 42: THere
nit: typo
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.h@79
PS3, Line 79: unblacklisted
May be clearer to call it "probation_list" or something so it can relates to
the state in which the backends on this list are in.
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.h@124
PS3, Line 124: pzrobation
typo
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.cc
File be/src/scheduling/executor-blacklist.cc:
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.cc@29
PS3, Line 29: 0 disables blacklisting
This seems a bit weird. Setting it to 0 means the blacklist timeout ==
statestore_max_missed_heartbeats * statestore_heartbeat_frequency_ms, right ?
Would it make more sense to set this to 120 or something by default so it means
(statestore_max_missed_heartbeats * statestore_heartbeat_frequency_ms) * (this
flag / 100.0) ? So setting this flag to 0 means the timeout is 0 which makes
sense to mean blacklisting is disabled.
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.cc@39
PS3, Line 39: ExecutorBlacklist::Blacklist(const TBackendDescriptor& be_desc) {
Should this check if blacklisting is disabled at the beginning ?
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.cc@54
PS3, Line 54: UnixMillis()
Why not MonotonicMillis() ? This field shouldn't need to be shared across
multiple hosts, right ? MonotonicMillis() makes sure the clock will not go
backward in case the TSC on different cores are slightly out of sync.
http://gerrit.cloudera.org:8080/#/c/13868/3/be/src/scheduling/executor-blacklist.cc@83
PS3, Line 83: auto eq = [_desc](const Entry& existing) {
: // The IP addresses must already match, so it is sufficient
to check the port.
: DCHECK_EQ(existing.be_desc.ip_address, be_desc.ip_address);
: return existing.be_desc.address.port == be_desc.address.port;
: };
This seems to be used at more than one functions. May be worth factoring it out
as a separate function.
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue interface and BufferedPRS impl
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 ) Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue interface and BufferedPRS impl .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/runtime/non-blocking-row-batch-queue.h File be/src/runtime/non-blocking-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/runtime/non-blocking-row-batch-queue.h@32 PS3, Line 32: NonBlockingRowBatchQueue > Adding support for TryAddBatch and TryGetBatch to BlockingQueue isn't exact As discussed offline, we may be able to get away with TryPutBatch() only. I am also fine with using std::queue in BufferedPlanRootSInk if changing BlockingQueue causes other complication. -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 24 Jul 2019 01:58:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8779, IMPALA-8780: RowBatchQueue interface and BufferedPRS impl
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 ) Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue interface and BufferedPRS impl .. Patch Set 4: (4 comments) Unifying the RowBatchQueue implementation and cleaning up the interface makes sense to me. Please see replies below for a suggestion to do it without splitting it into two classes in this patch. Also, some of the clean ups in KRPC code can be done in a follow-up patch. http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.h@29 PS3, Line 29: The blocking behavior follows : /// the same semantics as BlockingPlanRootSink. > Yeah, it probably makes more sense once we change BufferedPlanRootSink to u Yes, I guess at some point soon, we may want to consolidate on the implementation but I am fine with keeping the two classes for now. http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.h@42 PS3, Line 42: RowBatchQueue* batch_queue) > Dependency injection. I'm not sure if this is the right way to do this in C Thanks for the link. I can see the advantage being that different users of this class can pass in different implementation of RowBatchQueue interface for their own purposes. However, given the rather rigid use case of PlanRootSink right now, it seems like an unnecessary complication until the need for this arises. Also, please see comments elsewhere in which this ctor is invoked. The RowBatchQueue object seems to be leaked right now as the code stands. Given the limited charter of BufferedPlanRootSink, the code seems simpler if the batch_queue is owned by BufferedPlanRootSink instead and that also makes the reasoning of the lifetime of RowBatchQueue object clearer (i.e. it won't outlive that of the owning BufferedPlanRootSink). http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/exec/buffered-plan-root-sink.cc@45 PS4, Line 45: is_full_.Wait(l); > The existing BlockingQueue doesn't expose its lock, which makes the synchro I guess the answer to my question is that we access the BlockingQueue with 'lock_' held so we really need a non-blocking interface so we won't block other threads from consuming the row batch. The 'lock_' is necessary for synchronization multiple threads calling GetNext() / Close() / Send() concurrently. I suppose using RowBatchQueue::TryAddBatch() will fit the purpose, right ? http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/runtime/non-blocking-row-batch-queue.h File be/src/runtime/non-blocking-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/runtime/non-blocking-row-batch-queue.h@32 PS3, Line 32: NonBlockingRowBatchQueue > Adding TryAddBatch() makes sense to me. Yes, I agree the clean up of the RowBatchQueue interface is a good thing and you seem to also agree that refactoring RowBatchQueue into two classes may be overkill. How about you keep the necessary clean-up for the RowBatchQueue interface but instead of splitting it into two classes, we can keep the original RowBatchQueue instead. Like your TODO suggested above, we can hide the implementation of RowBatchQueue by instantiating a BlockingQueue object in RowBatchQueue class instead of inheriting it from BlockingQueue. In addition, a new interface called TryAddBatch() will be added to support the non-blocking behavior. Some modification may be needed in the BlockingQueue class to support the non-blocking insert behavior. So, the end result is: - we will still have a single RowBatchQueue class for this patch albeit with a better defined interface. - when we get around to implement a version of the queue backed by BTS, we can do the refactoring like this patch and RowBatchQueue can naturally become BlockingRowBatchQueue. The cleanup of KRPC sender / receiver can be done as a follow-on patch. What do you think ? -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 23 Jul 2019 19:34:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8656: Add RowBatchQueue interface and BufferedPlanRootSink impl
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 ) Change subject: IMPALA-8656: Add RowBatchQueue interface and BufferedPlanRootSink impl .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/exec/buffered-plan-root-sink.cc@44 PS4, Line 44: if Should this be while() ? Otherwise, I don't see how you will add the batch into the queue after waking up from is_full_ below. http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/exec/buffered-plan-root-sink.cc@45 PS4, Line 45: is_full_.Wait(l); Looking at this code again, the thread will block when the queue is full and unblock when a row batch is dequeued in GetNext() or if the sink is cancelled. So, why cannot we just use the existing blocking queue for it ? I could be missing something though. For cancellation, we can simply call Shutdown() on this queue and the thread will immediately be unblocked. http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/runtime/non-blocking-row-batch-queue.cc File be/src/runtime/non-blocking-row-batch-queue.cc: http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/runtime/non-blocking-row-batch-queue.cc@44 PS4, Line 44: unique_ptr result = move(batch_queue_.front()); : batch_queue_.pop(); Does it assume this function is called only when the queue is not empty ? -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 23 Jul 2019 06:59:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8701: [DOCS] Document --idle client poll time s flag
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13896 ) Change subject: IMPALA-8701: [DOCS] Document --idle_client_poll_time_s flag .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13896 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32ace786904f564b9c5fa3ed594e2b679b76d5c6 Gerrit-Change-Number: 13896 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Tue, 23 Jul 2019 06:44:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8656: Add RowBatchQueue interface and BufferedPlanRootSink impl
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8656: Add RowBatchQueue interface and
BufferedPlanRootSink impl
..
Patch Set 3:
(25 comments)
High level comment about the refactoring of the RowBatchQueue seems unnecessary
at this stage given the similarity of the two versions. It seems sufficient to
just add a TryAddBatch() interface to get most of the functionality needed now
but I could be missing something.
http://gerrit.cloudera.org:8080/#/c/13883/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13883/3//COMMIT_MSG@9
PS3, Line 9: a blocking and
: non-blocking implementation
It may help to spell out the exact difference in the two categories.
My interpretation is that a blocking queue will have limited capacity so it
will block once capacity is reached.
Non-blocking queue seems to lead to two possible interpretations:
- it still has limited capacity but when capacity is reached, it will somehow
not block but return something
- the queue "does not have a capacity" so it will never block
I guess we eventually want to get to some version of the second interpretation
but it seems that this patch is an implementation of the first version.
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.h
File be/src/exec/buffered-plan-root-sink.h:
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.h@29
PS3, Line 29: The blocking behavior follows
: /// the same semantics as BlockingPlanRootSink.
Now that I look at it again, I wonder if "BlockingPlanRootSink" is just a
BufferedPlanRootSink with a buffer size of 1 (or 0?). Once the buffering limit
is reached for this class, it will revert to the behavior of a
BlockingPlanRootSink so it seems as though BlockingPlanRootSink is just a
special version of BufferedPlanRootSink.
I guess having two classes makes more sense once we actually back the buffering
mechanism with buffered tuple stream with spilling so the buffering limit is
removed. As the code stands now, it makes me wonder if the two separate
PlanRootSink classes are needed.
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.h@42
PS3, Line 42: RowBatchQueue* batch_queue)
Why is this not internally owned by this class ? Why does it need to be passed
in via ctor this way ?
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.h@65
PS3, Line 65: boost::mutex
std::mutex. IIUC, the general direction is to move away from boost library.
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/buffered-plan-root-sink.cc@38
PS3, Line 38: output_batch->AcquireState(batch);
This is not making a copy. This is actually transferring the ownership of the
resource of 'batch' to 'output_batch'.
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/data-sink.cc@114
PS3, Line 114: new NonBlockingRowBatchQueue(10))
Who owns this object ? It doesn't look like it's being added to pool. We
usually make ownership explicit via unique_ptr / scoped_ptr or we add an object
to an object pool and document that it's owned by certain object pool so the
scope and the ownership of it is clear.
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/plan-root-sink.h
File be/src/exec/plan-root-sink.h:
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/plan-root-sink.h@61
PS3, Line 61: = 0;
Is it still valid in this patch ? Given PlanRootSink::Send() is not pure
virtual anymore, it seems appropriate to document what it does and its return
value.
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/scan-node.cc
File be/src/exec/scan-node.cc:
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/exec/scan-node.cc@315
PS3, Line 315: Shutdown();
Close();
Should stick to the interface of RowBatchQueue if possible and hide the
implementation details.
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/runtime/blocking-row-batch-queue.h
File be/src/runtime/blocking-row-batch-queue.h:
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/runtime/blocking-row-batch-queue.h@54
PS3, Line 54: public BlockingQueue, RowBatchBytesFn>,
: public RowBatchQueue {
Not a big fan of multi-inheritance.
Is there any reason why this class cannot just contain an instance of
BlockingQueue, RowBatchBytesFn> ? In other words,
why cannot we address the TODO mentioned above in this patch directly ?
http://gerrit.cloudera.org:8080/#/c/13883/3/be/src/runtime/blocking-row-batch-queue.h@67
PS3, Line 67: /// Adds a batch to the queue. This is blocking if the queue is
full.
Please also document the
[Impala-ASF-CR] IMPALA-8701: [DOCS] Document --idle client poll time s flag
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13896 ) Change subject: IMPALA-8701: [DOCS] Document --idle_client_poll_time_s flag .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/13896/1/docs/topics/impala_client.xml File docs/topics/impala_client.xml: http://gerrit.cloudera.org:8080/#/c/13896/1/docs/topics/impala_client.xml@144 PS1, Line 144: If a client session is idle for the duration of : --idle_client_poll_time_s seconds, the network connection of the : idle session is closed. Not sure if that's entirely accurate. This is how frequently poll will happen to check if a connection is idle and close it if it's idle. A connection is idle if all sessions associated with it are idle. http://gerrit.cloudera.org:8080/#/c/13896/1/docs/topics/impala_client.xml@155 PS1, Line 155: The session will only be closed The connection will only be closed if all sessions associated with it are all idle or closed. Sessions cannot be idle unless the flag --idle_session_timeout is greater than 0. -- To view, visit http://gerrit.cloudera.org:8080/13896 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32ace786904f564b9c5fa3ed594e2b679b76d5c6 Gerrit-Change-Number: 13896 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Mon, 22 Jul 2019 23:40:49 + Gerrit-HasComments: Yes
