[Impala-ASF-CR] IMPALA-3613: Avoid topic updates to unregistered subscriber instances
Hello Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8449 to look at the new patch set (#3). Change subject: IMPALA-3613: Avoid topic updates to unregistered subscriber instances .. IMPALA-3613: Avoid topic updates to unregistered subscriber instances Bug: Without this patch, when a subscriber repeatedly reconnects to the statestore, the latter queues the initial heartbeat message and a bunch of topic updates to every instance of the registered subscriber. These queued updates are eventually picked up by the heartbeating/topic update threads and the corresponding RPCs are made to the subscribers. The subscriber then rejects these updates since they were meant for an earlier registration. This is usually possible if the subscriber has some network problems leading to failing RPCs. Such a node is eventually marked by the statestore as bad, but depending on the configurations set, the issue can snowball into DDOS kind of attack when the entire thread pool of heartbeating/topic updates is filled with instances from the problematic host. This can result in the statestore missing timely heartbeats to other subscribers making them reconnect. This worsens the situation and the resulting topic updates for the reconnects will fully saturate the network on the statestore host, until the statestore daemon is restarted. Fix: This patch maps topic updates/heartbeats to a specific subscriber registered instance rather to a subscriber id (that stays same across reconnects). That way, when we encounter a topic update that was meant to a stale subscriber, we can simply reject it. Testing: Tested this locally by adding relevant logging. I made the subscribers to reconnect aggressively(a) and delaying heartbeats from the statestore side (b,c). (a) --statestore_subscriber_timeout_seconds=1 (b) --statestore_max_missed_heartbeats=1000 (c) --statestore_heartbeat_frequency_ms=6 Change-Id: I0329ae7d23dc6e9b04b7bc3ee8d89cbc73756f65 --- M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-subscriber.h M be/src/statestore/statestore.cc M be/src/statestore/statestore.h 4 files changed, 78 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/8449/3 -- To view, visit http://gerrit.cloudera.org:8080/8449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0329ae7d23dc6e9b04b7bc3ee8d89cbc73756f65 Gerrit-Change-Number: 8449 Gerrit-PatchSet: 3 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/8455 ) Change subject: IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc@394 PS1, Line 394: if (!status_.ok()) return; same http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc@1082 PS1, Line 1082: if (!query_status_.ok()) return; lock_? -- To view, visit http://gerrit.cloudera.org:8080/8455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed Gerrit-Change-Number: 8455 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Bharath Vissapragada Gerrit-Comment-Date: Fri, 03 Nov 2017 06:44:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Install OpenJDK-dbg for development environments.
Impala Public Jenkins has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/8431 )
Change subject: Install OpenJDK-dbg for development environments.
..
Install OpenJDK-dbg for development environments.
Updates the boostrap script to install the OpenJDK debug symbols.
OpenJDK comes with a gdb stack unwinder that errors out if the OpenJDK
debugging symbols aren't installed. This fixes the following error
message in gdb:
Installing openjdk unwinder
Traceback (most recent call last):
File
"/usr/share/gdb/auto-load/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server/libjvm.so-gdb.py",
line 52, in
class Types(object):
File
"/usr/share/gdb/auto-load/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server/libjvm.so-gdb.py",
line 66, in Types
nmethodp_t = gdb.lookup_type('nmethod').pointer()
gdb.error: No type named nmethod.
I tested this by installing the package manually on Ubuntu16.04 and
using gdb a bit.
Change-Id: I9aa3a5e1bbba55a4b0b489e4a4dfa39e35fc0ae0
Reviewed-on: http://gerrit.cloudera.org:8080/8431
Reviewed-by: Jim Apple
Tested-by: Impala Public Jenkins
---
M bin/bootstrap_system.sh
1 file changed, 2 insertions(+), 1 deletion(-)
Approvals:
Jim Apple: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8431
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9aa3a5e1bbba55a4b0b489e4a4dfa39e35fc0ae0
Gerrit-Change-Number: 8431
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] Install OpenJDK-dbg for development environments.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8431 ) Change subject: Install OpenJDK-dbg for development environments. .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8431 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9aa3a5e1bbba55a4b0b489e4a4dfa39e35fc0ae0 Gerrit-Change-Number: 8431 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 03 Nov 2017 06:41:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3613: Avoid topic updates to unregistered subscriber instances
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/8449 )
Change subject: IMPALA-3613: Avoid topic updates to unregistered subscriber
instances
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore-subscriber.h
File be/src/statestore/statestore-subscriber.h:
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore-subscriber.h@165
PS2, Line 165: typedef TUniqueId RegistrationId;
> You can move this typedef to statestore.h and use the same type in statesto
Done
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@383
PS2, Line 383: SubscriberId
> Where ever the 'SubscriberId' is
> required, we just get it from the Subscriber object anyway, and
> that object can be retrieved using the unique registration id.
Not sure I follow. If you see OfferUpdate()/DoSubscriberUpdate(), we only keep
track of ScheduledSubscriberUpdate objects and get the corresponding Subscriber
based on ScheduledSubscriberUpdate.subscriber_Id. So we don't have a Subscriber
object handy to get the subscriber_id in all cases.
Also, if we remove SubscriberId everywhere, this means that we need to change
the subscribers_ map structure to map from RegistrationId -> Subscriber
objects. Doing so we can't look up by subscriber_id, which is required in
RegisterSubscriber() (At that point, we don't assign a RegistrationId yet to
the new instance)
SubscriberMap::iterator subscriber_it = subscribers_.find(subscriber_id);
if (subscriber_it != subscribers_.end()) {
UnregisterSubscriber(subscriber_it->second.get());
}
We can still figure out a way, but it seemed unnecessarily complex to me.
Thoughts?
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@481
PS2, Line 481: subscriber exists
> Could you clarify what "exists" means here exactly? It could be confused wi
Done
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@482
PS2, Line 482: registration_ids
> nit: registration_id
Done
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@484
PS2, Line 484: std::shared_ptr* subscriber
> Add a comment about what is returned in this out parameter.
clarified
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc@414
PS2, Line 414: onst SubscriberId& subscriber_id,
: const TUniqueId& registration_id
> It looks like it just makes sense to pass 'const Subscriber&' here? Is ther
Not sure I understand. We get the subscriber/registration_id from the
ScheduledSubscriberUpdate object and not the Subscriber object. Do you mean we
should pass directly pass ScheduledSubscriberUpdate instead? If thats the case,
the signature seems kinda weird :)
http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc@634
PS2, Line 634: if (!RegisteredSubscriberExists(subscriber_to_update.first,
subscriber_to_update.second,
> I'm a little worried that we're contending for a mutex two more times in th
Good point. I too think (theoretically) spinlock is probably a better choice to
avoid context switching. Also, ~1000 entries seem like a reasonable estimate
for foreseeable future :). Also, I think for string based hashing of ~1000
entries, it is reasonable to assume a O(1) average case lookup (even though the
worst case is O(N)).
I created a microbenchmark to see if the lock type makes a difference. In the
benchmark, I measured how long it takes to get 100 heartbeats for a given
subscriber (with heart beating thread pool sizes of 10/100).
I didn't see any noticeable difference for 100 subscribers, but beyond that,
the test runs into flaky socket connection issues. I admit that this is not
representative of the real world use case because in real clusters, the
statestore CPU would be much busier and the context switching could be more
expensive.
--
To view, visit http://gerrit.cloudera.org:8080/8449
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0329ae7d23dc6e9b04b7bc3ee8d89cbc73756f65
Gerrit-Change-Number: 8449
Gerrit-PatchSet: 2
Gerrit-Owner: Bharath Vissapragada
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Fri, 03 Nov 2017 06:22:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8455 Change subject: IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation .. IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation The Coordinator::UpdateFilter() and Coordinator::PublishFilter() functions do not check for query failure/cancellation. So if runtime filters are being aggregated/published during a failure, they will not be cancelled and still be sent out which may take a while depending on the size of the cluster. Also, these functions could potentially hold very large amounts of untracked memory. This patch fixes it by checking for cancellation in both the functions. Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator.cc 2 files changed, 3 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8455/1 -- To view, visit http://gerrit.cloudera.org:8080/8455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed Gerrit-Change-Number: 8455 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
[Impala-ASF-CR] Install OpenJDK-dbg for development environments.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8431 ) Change subject: Install OpenJDK-dbg for development environments. .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8431 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9aa3a5e1bbba55a4b0b489e4a4dfa39e35fc0ae0 Gerrit-Change-Number: 8431 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 03 Nov 2017 03:30:46 + Gerrit-HasComments: No
[Impala-ASF-CR] Install OpenJDK-dbg for development environments.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8431 ) Change subject: Install OpenJDK-dbg for development environments. .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1430/ -- To view, visit http://gerrit.cloudera.org:8080/8431 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9aa3a5e1bbba55a4b0b489e4a4dfa39e35fc0ae0 Gerrit-Change-Number: 8431 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 03 Nov 2017 03:13:49 + Gerrit-HasComments: No
[Impala-ASF-CR] Install OpenJDK-dbg for development environments.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8431 ) Change subject: Install OpenJDK-dbg for development environments. .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8431 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9aa3a5e1bbba55a4b0b489e4a4dfa39e35fc0ae0 Gerrit-Change-Number: 8431 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 03 Nov 2017 03:13:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8430 ) Change subject: IMPALA-6136: Part 1: Query duration should not be normally negative. .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8430 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90 Gerrit-Change-Number: 8430 Gerrit-PatchSet: 3 Gerrit-Owner: Zoram Thanga Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Fri, 03 Nov 2017 02:22:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8430 ) Change subject: IMPALA-6136: Part 1: Query duration should not be normally negative. .. IMPALA-6136: Part 1: Query duration should not be normally negative. The second commit for IMPALA-5599, 1640aa97, introduced a regression where calculating the duration of an in-progress query can result in negative values. This can happen for two reasons: 1. The value of ClientRequestState::end_time_us_ is not initialized in the constructor, and may have some random value until ClientRequestState::Done() is called. 2. If ClientRequestState::end_time_us_ happens to have a positive value less than ClientRequestState::start_time_us_, the query duration will be negative. Here, since the query is still in flight, we need to use the local current time as the end time. The fix has been manually verified by executing long-running queries and checking the query profiles to ensure the durations are not some random junks. A new test case will be added to check for sane time values in a follow-on commit. Since we are using Unix time (system clock) for time stamps, it is still possible for end_time_us_ to be less than start_time_us_ if the system clock gets reset while the query is executing. If there are clients that assume that a query duration is never negative, we really should switch to a monotonic clock to time queries. Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90 Reviewed-on: http://gerrit.cloudera.org:8080/8430 Reviewed-by: Michael Ho Tested-by: Impala Public Jenkins --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-http-handler.cc M be/src/service/impala-server.h 4 files changed, 13 insertions(+), 2 deletions(-) Approvals: Michael Ho: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8430 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90 Gerrit-Change-Number: 8430 Gerrit-PatchSet: 4 Gerrit-Owner: Zoram Thanga Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zoram Thanga
[Impala-ASF-CR] Correct log line in start-impala-cluster.py.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8432 ) Change subject: Correct log line in start-impala-cluster.py. .. Correct log line in start-impala-cluster.py. Updated logging in start-impala-cluster to accurately specify how many impala nodes were started, and how many of these were coordinators or executors or both. The new logging looks like: Impala Cluster Running with 1 nodes (1 coordinators, 1 executors). Previously, when invoking this script with --cluster_size=1, it would report "1 nodes and 3 coordinators" which was wrong (because there was only 1 coordinator) and confusing (because it seemed like a coordinator was a separate thing from a node). I also removed an unused import. I have run core and exhaustive tests with these change, as part of testing other changes. Nothing untoward happened. Change-Id: I7ceece1c05b9a4ca9f0a08fa30d195f811490c0e Reviewed-on: http://gerrit.cloudera.org:8080/8432 Reviewed-by: Alex Behm Tested-by: Impala Public Jenkins --- M bin/start-impala-cluster.py 1 file changed, 8 insertions(+), 3 deletions(-) Approvals: Alex Behm: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8432 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I7ceece1c05b9a4ca9f0a08fa30d195f811490c0e Gerrit-Change-Number: 8432 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] Correct log line in start-impala-cluster.py.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8432 ) Change subject: Correct log line in start-impala-cluster.py. .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8432 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ceece1c05b9a4ca9f0a08fa30d195f811490c0e Gerrit-Change-Number: 8432 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 03 Nov 2017 02:11:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 5: (13 comments) http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.h@520 PS2, Line 520: void AddOptionLevelToConfigDescription(beeswax::ConfigVariable& option, : const string& option_key) const; > Remove 'default_query_options_' and make AddOptionLevelToConfigDescription( Done http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1221 PS2, Line 1221: > Why pass in 'default_query_options_'? Isn't it already visible in AddOption Done http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1224 PS2, Line 1224: } : } : : void ImpalaServer::AddOptionLevelToConfigDescript > Shouldn't you call AddOptionLevelToConfigDescription() with 'support_start_ As far as I know this support_start_over options is deliberately not added to the list of options in query-options.h because we don't want the same functionality to work on it like for the others. Here I can do something like string_map["support_start_over"]=false before the for loop without any problems I think. Let me give it a try. http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@106 PS2, Line 106: != 2 o > It is safer to use "!=" here. Done http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@107 PS2, Line 107: return "" > nit: maybe we should be more defensive and return "" as well if parsed_desc Done http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@127 PS2, Line 127: PRINT_REGULAR_OPTIONS_ONLY = 1 : PRINT_ALL_OPTIONS = 2 > nit: why not use a bool flag instead? I found it more verbose to have 2 const instead of a bool. e.g.1: having a PRINT_ALL_OPTIONS=false doesn't tell us what we print just that not all the options. e.g.2: PRINT_REGULAR_OPTIONS_ONLY=false has the same issue as below. If you don't agree with me then I'm fine to turn these into a single bool. http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@225 PS2, Line 225: =PR > nit: remove spaces around '=' Done http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@250 PS2, Line 250: then > 'then'? Done http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@252 PS2, Line 252: for option_name, option_value in default_options.iteritems(): > This could be "for option_name, option_value in default_options.items():" Done http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@255 PS2, Line 255: continue > I do not understand the meaning of this return. Nice catch, thx! Done http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@266 PS2, Line 266: options, set_options): > Why not use 'option_value'? Done http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@263 PS2, Line 263: advanced_options[option_name] = option_value : return (regular_options, advanced_options, deprecated_options) : : def _print_option_group(self, default_options, set_options): > How about this instead? yeah, I wanted to do some extra handling for the else, like writing some error log or such, but the decided not to do so. Thx! http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py@351 PS2, Line 351:result = shell1.get_result() : assert "Query options (defaults shown in []):" in result.stdout : assert "ABORT_ON_ERROR" in result.stdout > SUPPORT_START_OVER should appear here in result.stdout, right? No. SUPPORT_START_OVER as I discussed with Tim is a tricky one and we don't really like when the user alters it's value as it might result some queries to fail. So we decided to push it to the 'ADVANCED' group so it will only be displayed with 'SET ALL' -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Attila Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8447 to look at the new patch set (#5). Change subject: IMPALA-2181: Add query option levels for display .. IMPALA-2181: Add query option levels for display Three display levels are introduced for each query option: REGULAR, ADVANCED and DEPRECATED. When the query options are diplayed in Impala shell using SET then only the REGULAR options are shown. A new command called SET ALL shows all the options grouped by their option levels. Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 --- M be/src/service/child-query.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M shell/impala_client.py M shell/impala_shell.py M tests/shell/test_shell_interactive.py 9 files changed, 243 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/5 -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-5976: Remove equivalence class computation in FE
Tianyi Wang has posted comments on this change. (
http://gerrit.cloudera.org:8080/8317 )
Change subject: IMPALA-5976: Remove equivalence class computation in FE
..
Patch Set 4:
(62 comments)
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java:
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@281
PS3, Line 281:
> Let's please avoid code changes unrelated to the purpose of this patch as m
Done.
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@91
PS3, Line 91: /**
> We need a precise definition. The original definition was more precise.
I think If the predicate transfer is the starting point to consider whether
there is a value transfer between slots, it should be defined as so. The
original sentence "is computed based on..." is not really a definition to me.
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@93
PS3, Line 93: *
> Not sure this part adds value. What's the significance of these statements
Removed. I intended to use these statements to show that the symmetric closure
is an equivalence relation.
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@277
PS3, Line 277: // Tracks all warnings (e.g. non-fatal errors) that were
generated during analysis.
> The SCC-condensed graph representation of all slot value transfers.
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@278
PS3, Line 278: // These are passed to the backend and eventually propagated
to the shell. Maps from
> public/private?
Done. private.
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1146
PS3, Line 1146: conjunctIds.add(e.getId());
> a mutual value transfer
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1467
PS3, Line 1467: List tids = Lists.newArrayList();
> how about: if every source slot has a value transfer to a slot in destTid
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1513
PS3, Line 1513: boolean hasOuterJoinedTuple = false;
> Let's simplify the example and make it as clear as possible:
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1618
PS3, Line 1618: // check if we already created this predicate
> Need a definition of equivalence class in the Analyzer class comment. You d
I think they are the same.
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1636
PS3, Line 1636:* equivalent slot sets of lhsTids and rhsTids.
> Garbled sentence, please clean up
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1956
PS3, Line 1956: public boolean isVisible(TupleId tid) {
> the registered value transfers
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1973
PS3, Line 1973: new
WritableGraph(globalState_.descTbl.getMaxSlotId().asInt() + 1);
> missing space in string
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1982
PS3, Line 1982: RandomAccessibleGraph reference =
> Add value-transfer edges to 'g' based on the registered equi-join conjuncts
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2059
PS3, Line 2059: if (tblRef.getJoinOp() == JoinOperator.LEFT_OUTER_JOIN
> of the given slot id
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2062
PS3, Line 2062: g.addEdge(outerSlot.asInt(), innerSlot.asInt());
> slot -> sid
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2066
PS3, Line 2066: }
> Unfortunate that we have to do this. Should probably clean this up later by
Ack
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2073
PS3, Line 2073:* Time complexity: O(V) where V = number of slots
> Simpler to avoid "image set" and just use your second sentence to describe.
Done
http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2087
PS3, Line 2087:* Time complexity: O(V) where V = number of slots
> Se
[Impala-ASF-CR] IMPALA-5976: Remove equivalence class computation in FE
Hello Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8317 to look at the new patch set (#4). Change subject: IMPALA-5976: Remove equivalence class computation in FE .. IMPALA-5976: Remove equivalence class computation in FE Equivalent class is used to get the equivalencies between slots. It is ill-defined and the current implementation is inefficient. This patch removes it and directly uses the information from the value transfer graph instead. Value transfer graph is reimplemented using Tarjan's strongly connected component algorithm and BFS with adjacency lists to speed up on both condensed and sparse graphs. Testing: It passes the existing tests. In planner tests the equivalence between SCC-condensed graph and uncondensed graph is checked. A test case is added for a helper class IntArrayList. On a query with 1800 union operations, the equivalence class computation time is reduced from 7m57s to 65ms and the planning time is reduced from 8m5s to 13s. Change-Id: If4cb1d8be46efa8fd61a97048cc79dabe2ffa51a --- M fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java M fe/src/main/java/org/apache/impala/analysis/AggregateInfoBase.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java M fe/src/main/java/org/apache/impala/analysis/InPredicate.java M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java M fe/src/main/java/org/apache/impala/analysis/IsNotEmptyPredicate.java M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java M fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java M fe/src/main/java/org/apache/impala/analysis/NullLiteral.java M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java M fe/src/main/java/org/apache/impala/analysis/SlotRef.java M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java M fe/src/main/java/org/apache/impala/analysis/Subquery.java M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java A fe/src/main/java/org/apache/impala/util/Graph.java A fe/src/main/java/org/apache/impala/util/IntArrayList.java A fe/src/main/java/org/apache/impala/util/IntIterator.java A fe/src/test/java/org/apache/impala/util/IntArrayListTest.java 36 files changed, 1,049 insertions(+), 755 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/8317/4 -- To view, visit http://gerrit.cloudera.org:8080/8317 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If4cb1d8be46efa8fd61a97048cc79dabe2ffa51a Gerrit-Change-Number: 8317 Gerrit-PatchSet: 4 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tianyi Wang
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Attila Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8447 to look at the new patch set (#4). Change subject: IMPALA-2181: Add query option levels for display .. IMPALA-2181: Add query option levels for display Three display levels are introduced for each query option: REGULAR, ADVANCED and DEPRECATED. When the query options are diplayed in Impala shell using SET then only the REGULAR options are shown. A new command called SET ALL shows all the options grouped by their option levels. Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 --- M be/src/service/child-query.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M shell/impala_client.py M shell/impala_shell.py M tests/shell/test_shell_interactive.py 9 files changed, 245 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/4 -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 4 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6054: Parquet dictionary pages should be freed on dictionary construction
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/8436 )
Change subject: IMPALA-6054: Parquet dictionary pages should be freed on
dictionary construction
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc@959
PS1, Line 959: if (tmp_buffer.TryAllocate(uncompressed_size)) {
It turned out that this can violate a dcheck, if uncompressed_size is 0, which
seems valid if every value of a column in a partition is NULL. This is the case
in a partition of functional_parquet.alltypesagg/tinyint_col, which lead to a
crash in one of the tests. I think that there should be a table/column with all
NULL values by design to test this case explicitly.
--
To view, visit http://gerrit.cloudera.org:8080/8436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d9d5f4da1028d961155dafdac0028a1c3641004
Gerrit-Change-Number: 8436
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Lars Volker
Gerrit-Comment-Date: Fri, 03 Nov 2017 00:44:57 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6087: Revisit tests withheld from TPC-DS suite for use of TRUNCATE
Tim Wood has posted comments on this change. ( http://gerrit.cloudera.org:8080/8372 ) Change subject: IMPALA-6087: Revisit tests withheld from TPC-DS suite for use of TRUNCATE .. Patch Set 4: > Patch Set 4: > > > Patch Set 4: > > > > What about Mostafa's question: > > > How many iterations did you try for those tests? > > I thought I answered it: > > For example, q47 failed in 4 different test runs on 9/18 because of an LSD > > difference. ...[I]t's passing now that I replaced the old expected output > > with the new > > Am I not understanding "iterations" in this context? Taking a guess at the question, here are the numbers of attempts of each of these tests in my local workspace since I started on this test suite: q26 59 q28 29 q47 39 q57 31 q59 28 q63 28 -- To view, visit http://gerrit.cloudera.org:8080/8372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I79d2e34621639c8f8c4c4eb0b0944eaefca13a7a Gerrit-Change-Number: 8372 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Wood Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Mostafa Mokhtar Gerrit-Reviewer: Tim Wood Gerrit-Comment-Date: Fri, 03 Nov 2017 00:16:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8038 ) Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. IMPALA-5736: Add impala-shell argument to set default query options Query options can be set from command line and impala rc as key=value pairs, where key is case insensitive. Examples: command line: impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200 .impalarc: [impala.query_options] EXPLAIN_LEVEL=2 MT_DOP=2 The options set in command line will update the ones in impalarc one by one, so the result of the example above will be: EXPLAIN_LEVEL=2 MT_DOP=1 MAX_ERRORS=200 Additional changes: - 0 and 1 are accepted as bools in section [impala] to make it more consistent with [impala.query_options] - options that are expected to be bool but are not 0/1/true/false lead to error instead of warning Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Reviewed-on: http://gerrit.cloudera.org:8080/8038 Reviewed-by: Michael Brown Tested-by: Impala Public Jenkins --- M bin/rat_exclude_files.txt M shell/impala_shell.py M shell/option_parser.py A tests/shell/impalarc_with_error A tests/shell/impalarc_with_query_options A tests/shell/impalarc_with_warnings M tests/shell/test_shell_commandline.py M tests/shell/test_shell_interactive.py 8 files changed, 159 insertions(+), 46 deletions(-) Approvals: Michael Brown: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 19 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 ) Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. Patch Set 18: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 18 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 03 Nov 2017 00:11:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Attila Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8447 to look at the new patch set (#3). Change subject: IMPALA-2181: Add query option levels for display .. IMPALA-2181: Add query option levels for display Three display levels are introduced for each query option: REGULAR, ADVANCED and DEPRECATED. When the query options are diplayed in Impala shell using SET then only the REGULAR options are shown. A new command called SET ALL shows all the options grouped by their option levels. Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 --- M be/src/service/child-query.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M shell/impala_client.py M shell/impala_shell.py M tests/shell/test_shell_interactive.py 9 files changed, 245 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/3 -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 3 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8215 ) Change subject: IMPALA-5142 EventSequence displays negative elapsed time. .. IMPALA-5142 EventSequence displays negative elapsed time. EventSequence::EventList which stores the Event in increasing time order may have at times Events that are not stored in increasing time order. This may happen when concurrently EventSequence::MarkEvent() is called for different Events in the same EventSequence. The incorrect time order of Event in EventList results in a negative value being displayed, which is the issue reported in this JIRA. The issue can be fixed either be re-ordering the lock in EventSequence::MarkEvent() or by sorting the EventList before printing. Reordering of lock in EventSequence::MarkEvent() will involve holding the lock across clock_gettime() so that approach is not taken. This patch fixes the issue by sorting the EventList in GetEvents() as this function is expected to return sorted list of events based on time. Testing: Ran all the front-end/backend and end-end tests. Change-Id: I8c944396d96473b17b453da3e913ffc56680a896 Reviewed-on: http://gerrit.cloudera.org:8080/8215 Reviewed-by: Dan Hecht Tested-by: Impala Public Jenkins --- M be/src/util/runtime-profile-counters.h 1 file changed, 6 insertions(+), 0 deletions(-) Approvals: Dan Hecht: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896 Gerrit-Change-Number: 8215 Gerrit-PatchSet: 8 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pranay Singh
[Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8215 ) Change subject: IMPALA-5142 EventSequence displays negative elapsed time. .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896 Gerrit-Change-Number: 8215 Gerrit-PatchSet: 7 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pranay Singh Gerrit-Comment-Date: Thu, 02 Nov 2017 22:53:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8428 ) Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable .. Patch Set 1: I can take another look once you push the new patch set. I don't think we need to change the time duration thing :) -- To view, visit http://gerrit.cloudera.org:8080/8428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 Gerrit-Change-Number: 8428 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 02 Nov 2017 22:51:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6087: Revisit tests withheld from TPC-DS suite for use of TRUNCATE
Tim Wood has posted comments on this change. ( http://gerrit.cloudera.org:8080/8372 ) Change subject: IMPALA-6087: Revisit tests withheld from TPC-DS suite for use of TRUNCATE .. Patch Set 4: > Patch Set 4: > > What about Mostafa's question: > > How many iterations did you try for those tests? I thought I answered it: > For example, q47 failed in 4 different test runs on 9/18 because of an LSD > difference. ...[I]t's passing now that I replaced the old expected output > with the new Am I not understanding "iterations" in this context? -- To view, visit http://gerrit.cloudera.org:8080/8372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I79d2e34621639c8f8c4c4eb0b0944eaefca13a7a Gerrit-Change-Number: 8372 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Wood Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Mostafa Mokhtar Gerrit-Reviewer: Tim Wood Gerrit-Comment-Date: Thu, 02 Nov 2017 22:40:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5564: Release lock during planning. (wip)
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8434 ) Change subject: IMPALA-5564: Release lock during planning. (wip) .. Patch Set 1: (1 comment) Thanks for the reviews! I'll report back about cancellations and how this looks in tools like CM that look at profiles. Cancellations isn't something I considered yet, so thanks for the tip! http://gerrit.cloudera.org:8080/#/c/8434/1/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/8434/1/be/src/service/client-request-state.h@342 PS1, Line 342: is_planning_ > maybe make that is_planning_finished_ (or similar), and initialize it to fa I started out with this being planning_finished_ (or some such). I ran into the fact that some calls (like GetSchemas()) avoid the ExecuteInternal() path entirely: there's no planning to be done, but yet they also have a ClientRequestState, and they use get_result_metadata() and so on. In my case, the JDBC tests exposed this very clearly, but many other tests would have as well. So, I decided to have state around only the query paths that actually do planning, so as to avoid saying that "GetSchemas()" has finished planning, since that makes less sense. I could also think about this as a query state, but changing the query state enums seems much more delicate than this approach. -- To view, visit http://gerrit.cloudera.org:8080/8434 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e3fc4c28d7a5ad8546d48bcd22c03fbce502e5b Gerrit-Change-Number: 8434 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 02 Nov 2017 22:32:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8430 ) Change subject: IMPALA-6136: Part 1: Query duration should not be normally negative. .. Patch Set 3: Code-Review+2 Carry +2 -- To view, visit http://gerrit.cloudera.org:8080/8430 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90 Gerrit-Change-Number: 8430 Gerrit-PatchSet: 3 Gerrit-Owner: Zoram Thanga Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Thu, 02 Nov 2017 22:31:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8430 ) Change subject: IMPALA-6136: Part 1: Query duration should not be normally negative. .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1429/ -- To view, visit http://gerrit.cloudera.org:8080/8430 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90 Gerrit-Change-Number: 8430 Gerrit-PatchSet: 3 Gerrit-Owner: Zoram Thanga Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Thu, 02 Nov 2017 22:31:27 + Gerrit-HasComments: No
[Impala-ASF-CR] Correct log line in start-impala-cluster.py.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8432 ) Change subject: Correct log line in start-impala-cluster.py. .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1428/ -- To view, visit http://gerrit.cloudera.org:8080/8432 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ceece1c05b9a4ca9f0a08fa30d195f811490c0e Gerrit-Change-Number: 8432 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 02 Nov 2017 22:22:17 + Gerrit-HasComments: No
[Impala-ASF-CR] Correct log line in start-impala-cluster.py.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8432 ) Change subject: Correct log line in start-impala-cluster.py. .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8432 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ceece1c05b9a4ca9f0a08fa30d195f811490c0e Gerrit-Change-Number: 8432 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 02 Nov 2017 22:21:43 + Gerrit-HasComments: No
[Impala-ASF-CR] Correct log line in start-impala-cluster.py.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8432 ) Change subject: Correct log line in start-impala-cluster.py. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8432/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/8432/1/bin/start-impala-cluster.py@398 PS1, Line 398: executors = options.cluster_size - options.num_coordinators > Could executors be negative due to the same bug? No. I think it's handled by line 343-344. $bin/start-impala-cluster.py --impalad_args="--stress_metadata_loading_pause_injection_ms=5000" --log_level=2 --cluster_size=1 --num_coordinators=1 --use_exclusive_coordinators Cannot start an Impala cluster with no executors -- To view, visit http://gerrit.cloudera.org:8080/8432 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ceece1c05b9a4ca9f0a08fa30d195f811490c0e Gerrit-Change-Number: 8432 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 02 Nov 2017 22:08:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6087: Revisit tests withheld from TPC-DS suite for use of TRUNCATE
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8372 ) Change subject: IMPALA-6087: Revisit tests withheld from TPC-DS suite for use of TRUNCATE .. Patch Set 4: What about Mostafa's question: > How many iterations did you try for those tests? -- To view, visit http://gerrit.cloudera.org:8080/8372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I79d2e34621639c8f8c4c4eb0b0944eaefca13a7a Gerrit-Change-Number: 8372 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Wood Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Mostafa Mokhtar Gerrit-Reviewer: Tim Wood Gerrit-Comment-Date: Thu, 02 Nov 2017 21:31:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4835 (prep only): create io subfolder and namespace
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/8424 ) Change subject: IMPALA-4835 (prep only): create io subfolder and namespace .. Patch Set 4: Made a first pass through this and it makes sense so far. I will make a second pass soon. -- To view, visit http://gerrit.cloudera.org:8080/8424 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If807f93a47d8027a43e56dd80b1b535d0bb74e1b Gerrit-Change-Number: 8424 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 02 Nov 2017 21:24:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6087: Revisit tests withheld from TPC-DS suite for use of TRUNCATE
Tim Wood has posted comments on this change. ( http://gerrit.cloudera.org:8080/8372 ) Change subject: IMPALA-6087: Revisit tests withheld from TPC-DS suite for use of TRUNCATE .. Patch Set 4: Any further comments on this review? Any reason not to +2? Thanks. -- To view, visit http://gerrit.cloudera.org:8080/8372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I79d2e34621639c8f8c4c4eb0b0944eaefca13a7a Gerrit-Change-Number: 8372 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Wood Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Mostafa Mokhtar Gerrit-Reviewer: Tim Wood Gerrit-Comment-Date: Thu, 02 Nov 2017 21:23:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 ) Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. Patch Set 18: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1427/ -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 18 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Thu, 02 Nov 2017 20:20:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 ) Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. Patch Set 18: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 18 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Thu, 02 Nov 2017 20:19:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8428 ) Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h File be/src/util/condition-variable.h: http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@64 PS1, Line 64: template > IMHO it is easier for the callers to specify a time duration than forcing t Ah ok, I didn't see those callsites when I grepped the codebase. This makes sense then, no need to change it. -- To view, visit http://gerrit.cloudera.org:8080/8428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 Gerrit-Change-Number: 8428 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 02 Nov 2017 20:15:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6137: fix text scanner split delim mem mgmt
Hello anujphadke, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8438 to look at the new patch set (#4). Change subject: IMPALA-6137: fix text scanner split delim mem mgmt .. IMPALA-6137: fix text scanner split delim mem mgmt The bug was that the buffer pointed to by byte_buffer_ptr_ could be freed by ReleaseCompletedResources() before CheckForSplitDelimiter() was called. The simple fix is to copy out the single byte that is needed each time the buffer is filled. Testing: Ran exhaustive query tests under ASAN with --disable_mem_pools=true. Before the change test_text_split_delimiters reliably caused an ASAN failure when run with --disable_mem_pools=true. We should get this coverage automatically once the I/O mgr switches to using the buffer pool, which uses ASAN poisoning on freed buffers. Change-Id: Iddbb5cf6acc8f0b0e0b4c205c334f21e03d06f1c --- M be/src/exec/hdfs-text-scanner.cc M be/src/exec/hdfs-text-scanner.h 2 files changed, 41 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8438/4 -- To view, visit http://gerrit.cloudera.org:8080/8438 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iddbb5cf6acc8f0b0e0b4c205c334f21e03d06f1c Gerrit-Change-Number: 8438 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 ) Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. Patch Set 18: > Csaba, it's hard to see, but the build failed the Apache RAT check. > If you search 'FAILED JOB' at > https://jenkins.impala.io/job/gerrit-verify-dryrun/1425/console > you see a pointer to https://jenkins.impala.io/job/rat-check-ub1604/111/ > . In there is: > > 22:05:14 + bin/check-rat-report.py bin/rat_exclude_files.txt > rat.xml > 22:05:15 UNAPPROVED: tests/shell/impalarc_with_error > 22:05:15 UNAPPROVED: tests/shell/impalarc_with_query_options > 22:05:15 UNAPPROVED: tests/shell/impalarc_with_warnings > > I think these exclusions can be added to bin/rat_exclude_files.txt > in this section, alongside the existing shell test data exclusions: > > # http://www.apache.org/legal/src-headers.html: "Test data for > which the addition of a > # source header would cause the tests to fail." Thanks for the hint! I have added a test impalarc files to bin/rat_exclude_files.txt. -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 18 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Thu, 02 Nov 2017 20:13:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6137: fix text scanner split delim mem mgmt
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8438 ) Change subject: IMPALA-6137: fix text scanner split delim mem mgmt .. Patch Set 2: (2 comments) The newest patch is my attempt at a minimal fix that addresses the edge case we discussed and works around the problem that FillByteBuffer() is overridable. http://gerrit.cloudera.org:8080/#/c/8438/2/be/src/exec/hdfs-text-scanner.h File be/src/exec/hdfs-text-scanner.h: http://gerrit.cloudera.org:8080/#/c/8438/2/be/src/exec/hdfs-text-scanner.h@143 PS2, Line 143: virtual Status FillByteBuffer(MemPool* pool, bool* eosr, int num_bytes = 0) > *sigh* I found out that this is virtual because the text scanner plugin mec I don't think I want to change the extension mechanism as part of this bug fix - filed IMPALA-6146. http://gerrit.cloudera.org:8080/#/c/8438/2/be/src/exec/hdfs-text-scanner.cc File be/src/exec/hdfs-text-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8438/2/be/src/exec/hdfs-text-scanner.cc@717 PS2, Line 717: byte_buffer_last_byte_ > I couldn't see an obvious code path that would take us there. Maybe some sp I couldn't come up with a test that would cause this, but I added logic to avoid reading byte_buffer_last_byte_ when it wasn't initialised to a valid value. -- To view, visit http://gerrit.cloudera.org:8080/8438 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iddbb5cf6acc8f0b0e0b4c205c334f21e03d06f1c Gerrit-Change-Number: 8438 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Thu, 02 Nov 2017 20:12:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6137: fix text scanner split delim mem mgmt
Hello anujphadke, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8438 to look at the new patch set (#3). Change subject: IMPALA-6137: fix text scanner split delim mem mgmt .. IMPALA-6137: fix text scanner split delim mem mgmt The bug was that the buffer pointed to by byte_buffer_ could be freed by ReleaseCompletedResources() before CheckForSplitDelimiter() was called. The simple fix is to copy out the single byte that is needed each time the buffer is filled. Testing: Ran exhaustive query tests under ASAN with --disable_mem_pools=true. Before the change test_text_split_delimiters reliably caused an ASAN failure when run with --disable_mem_pools=true. We should get this coverage automatically once the I/O mgr switches to using the buffer pool, which uses ASAN poisoning on freed buffers. Change-Id: Iddbb5cf6acc8f0b0e0b4c205c334f21e03d06f1c --- M be/src/exec/hdfs-text-scanner.cc M be/src/exec/hdfs-text-scanner.h 2 files changed, 41 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8438/3 -- To view, visit http://gerrit.cloudera.org:8080/8438 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iddbb5cf6acc8f0b0e0b4c205c334f21e03d06f1c Gerrit-Change-Number: 8438 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8038 to look at the new patch set (#18). Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. IMPALA-5736: Add impala-shell argument to set default query options Query options can be set from command line and impala rc as key=value pairs, where key is case insensitive. Examples: command line: impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200 .impalarc: [impala.query_options] EXPLAIN_LEVEL=2 MT_DOP=2 The options set in command line will update the ones in impalarc one by one, so the result of the example above will be: EXPLAIN_LEVEL=2 MT_DOP=1 MAX_ERRORS=200 Additional changes: - 0 and 1 are accepted as bools in section [impala] to make it more consistent with [impala.query_options] - options that are expected to be bool but are not 0/1/true/false lead to error instead of warning Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 --- M bin/rat_exclude_files.txt M shell/impala_shell.py M shell/option_parser.py A tests/shell/impalarc_with_error A tests/shell/impalarc_with_query_options A tests/shell/impalarc_with_warnings M tests/shell/test_shell_commandline.py M tests/shell/test_shell_interactive.py 8 files changed, 159 insertions(+), 46 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/18 -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 18 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 ) Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. Patch Set 17: Csaba, it's hard to see, but the build failed the Apache RAT check. If you search 'FAILED JOB' at https://jenkins.impala.io/job/gerrit-verify-dryrun/1425/console you see a pointer to https://jenkins.impala.io/job/rat-check-ub1604/111/ . In there is: 22:05:14 + bin/check-rat-report.py bin/rat_exclude_files.txt rat.xml 22:05:15 UNAPPROVED: tests/shell/impalarc_with_error 22:05:15 UNAPPROVED: tests/shell/impalarc_with_query_options 22:05:15 UNAPPROVED: tests/shell/impalarc_with_warnings I think these exclusions can be added to bin/rat_exclude_files.txt in this section, alongside the existing shell test data exclusions: # http://www.apache.org/legal/src-headers.html: "Test data for which the addition of a # source header would cause the tests to fail." -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 17 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Thu, 02 Nov 2017 19:57:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8368 ) Change subject: IMPALA-2235: Fix current db when shell auto-reconnects .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/8368/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8368/2/shell/impala_shell.py@729 PS2, Line 729: def _validate_database(self, immediately=False): > Is there a case where you ever want immediately=True? i.e., could we get ri Yes, in precmd after reconnecting to the cluster (this commit). Every other _validate_database invocation use the default value which maintains the old behaviour of the method. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py File tests/common/cluster_controller.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py@34 PS2, Line 34: breakpad > Comment needs update. I guess now it's used for all custom cluster tests th Yeah, that's the idea. I'll remove the comment. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py@42 PS2, Line 42: self.tmp_dir = tempfile.mkdtemp() > I'm thinking about whether this is factored in the best way and whether the Seems reasonable, will do it http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py File tests/custom_cluster/test_shell_interactive_reconnect.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py@27 PS2, Line 27: class TestShellInteractiveReconnect(ClusterController): > Any reason not to add this simply to tests/shell/test_shell_interactive.py? Yes, it is in the custom cluster test suite because I am starting/restarting the cluster. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py@40 PS2, Line 40: @pytest.mark.execute_serially > I think all CustomCluster tests run serially? This one seems like it could I guess you are right, my idea was what if another test case restarts the cluster during this test, so this impala shell automatically reconnects to the cluster and it makes this test fail or pass. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 02 Nov 2017 19:52:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8215 ) Change subject: IMPALA-5142 EventSequence displays negative elapsed time. .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896 Gerrit-Change-Number: 8215 Gerrit-PatchSet: 7 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Pranay Singh Gerrit-Comment-Date: Thu, 02 Nov 2017 19:24:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8215 ) Change subject: IMPALA-5142 EventSequence displays negative elapsed time. .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896 Gerrit-Change-Number: 8215 Gerrit-PatchSet: 6 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Pranay Singh Gerrit-Comment-Date: Thu, 02 Nov 2017 19:24:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8215 ) Change subject: IMPALA-5142 EventSequence displays negative elapsed time. .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1426/ -- To view, visit http://gerrit.cloudera.org:8080/8215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896 Gerrit-Change-Number: 8215 Gerrit-PatchSet: 7 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pranay Singh Gerrit-Comment-Date: Thu, 02 Nov 2017 19:24:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@252 PS2, Line 252: for option_name in default_options: This could be "for option_name, option_value in default_options.items():" http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@255 PS2, Line 255: return (regular_options, advanced_options, deprecated_options) I do not understand the meaning of this return. What is the goal of this block? If it is to add options with unknown level to advanced, then you could use the get function with default value: level = query_option_levels(option_name, self.QUERY_OPTION_ADVANCED) -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 02 Nov 2017 18:38:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6137: fix text scanner split delim mem mgmt
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8438 ) Change subject: IMPALA-6137: fix text scanner split delim mem mgmt .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/8438/2/be/src/exec/hdfs-text-scanner.h File be/src/exec/hdfs-text-scanner.h: http://gerrit.cloudera.org:8080/#/c/8438/2/be/src/exec/hdfs-text-scanner.h@143 PS2, Line 143: virtual Status FillByteBuffer(MemPool* pool, bool* eosr, int num_bytes = 0) *sigh* I found out that this is virtual because the text scanner plugin mechanism (used by the Lzo plugin) allows text scanners to override this method. So any fix I make within this method needs to be mirrored in the plugin version. This is pretty broken so I'm going to change the extension mechanism to be a bit less brittle. -- To view, visit http://gerrit.cloudera.org:8080/8438 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iddbb5cf6acc8f0b0e0b4c205c334f21e03d06f1c Gerrit-Change-Number: 8438 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Thu, 02 Nov 2017 18:22:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.h@520 PS2, Line 520: void AddOptionLevelToConfigDescription(beeswax::ConfigVariable& option, : const TQueryOptions& default_query_options_, const string& option_key); Remove 'default_query_options_' and make AddOptionLevelToConfigDescription() const. -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 02 Nov 2017 18:12:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Zoltan Borok-Nagy has posted comments on this change. (
http://gerrit.cloudera.org:8080/8428 )
Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable
..
Patch Set 1:
(3 comments)
> (1 comment)
>
> Most of this looks mechanical, and it looked fine. You changed some
> 1-line if's into 3-line ifs, which may be against house style.
>
> We don't seem to have any tests for condition-variable.h. How did
> you test this?
OK, I'll change the 3-line ifs back to 1-lines.
After the modifications, I ran the backend test suites on my local desktop.
Should I create tests for condition-variable.h? We don't seem to have unit
tests yet.
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h
File be/src/util/condition-variable.h:
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@38
PS1, Line 38: void Wait(boost::unique_lock& lock) {
> I'm not yet familiar with this part of C++; why is 'inline' getting removed
About the 'struct' keyword: it is something that C requires, but in C++ it can
be omitted. If there were a function named 'timespec', then we would have to
use the 'struct' keyword in C++ as well to resolve ambiguity.
An example for that is 'stat' in POSIX, which is also a function and a struct.
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@38
PS1, Line 38: void Wait(boost::unique_lock& lock) {
> "inline" is implied by the function being defined within the class body: ht
Done
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@64
PS1, Line 64: template
> It looks like this is only ever used with microseconds - maybe we should na
IMHO it is easier for the callers to specify a time duration than forcing them
to calculate a time point in the future. At least I think this is how it is
used most of the time, so I would choose this direction.
We also have calls with milliseconds(thrift-server.cc, impala-server.cc), and
seconds (fragment-instance-state.cc) as well.
We can also narrow the scope by templates (though it is a bit ugly):
template
... subsecond_duration ...
Or, we can fix the callsites to always use microseconds.
What do you think of it?
--
To view, visit http://gerrit.cloudera.org:8080/8428
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9
Gerrit-Change-Number: 8428
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 02 Nov 2017 18:02:26 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py@351 PS2, Line 351:result = shell1.get_result() : assert "Query options (defaults shown in []):" in result.stdout : assert "ABORT_ON_ERROR" in result.stdout SUPPORT_START_OVER should appear here in result.stdout, right? -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 02 Nov 2017 18:01:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/8447 )
Change subject: IMPALA-2181: Add query option levels for display
..
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1221
PS2, Line 1221: default_query_options_
Why pass in 'default_query_options_'? Isn't it already visible in
AddOptionLevelToConfigDescription()?
http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1224
PS2, Line 1224: ConfigVariable support_start_over;
: support_start_over.__set_key("support_start_over");
: support_start_over.__set_value("false");
: default_configs_.push_back(support_start_over);
Shouldn't you call AddOptionLevelToConfigDescription() with
'support_start_over' as well?
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py
File shell/impala_client.py:
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@106
PS2, Line 106: is not
It is safer to use "!=" here.
"is not" works for small integers with the CPython interpreter but it is
implementation specific behavior and might be changed in the future.
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@107
PS2, Line 107: return ""
nit: maybe we should be more defensive and return "" as well if
parsed_description[0] != "OptionLevel".
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@127
PS2, Line 127: PRINT_REGULAR_OPTIONS_ONLY = 1
: PRINT_ALL_OPTIONS = 2
nit: why not use a bool flag instead?
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@225
PS2, Line 225: =
nit: remove spaces around '='
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@250
PS2, Line 250: that
'then'?
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@266
PS2, Line 266: default_options[option_name]
Why not use 'option_value'?
http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@263
PS2, Line 263: elif level == self.QUERY_OPTION_ADVANCED:
: advanced_options[option_name] = option_value
: else:
: advanced_options[option_name] =
default_options[option_name]
How about this instead?
else:
advanced_options[option_name] = option_value
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Laszlo Gaal
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 02 Nov 2017 17:32:23 +
Gerrit-HasComments: Yes
