[Impala-ASF-CR] IMPALA-6151: add query-level fragment/backend counters

2017-11-07 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8461 )

Change subject: IMPALA-6151: add query-level fragment/backend counters
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8461/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8461/5/be/src/service/impala-server.cc@953
PS5, Line 953:   ImpaladMetrics::NUM_QUERIES_REGISTERED->Increment(1L);
> On the /queries webpage, I think "num_in_flight_queries" is this same numbe
Oh, I see, I didn't look at the queries json. The queries page seems internally 
inconsistent - num_in_flight_queries includes "queries in flight" and "queries 
waiting to be closed". But yeah, I deliberately avoided using the term "in 
flight" because it's unclear whether it should include queries that are waiting 
to be closed.

I don't like inventing a new term but this does seem less ambiguous going 
forward.

It would be nice to re-evaluate all the names but probably best to do that all 
at once instead of piecemeal, since it will require considering impact on other 
tools, like you mentioned.



--
To view, visit http://gerrit.cloudera.org:8080/8461
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3df350414733e98d1ec28adc1c98f45bb0c4e3e9
Gerrit-Change-Number: 8461
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 07 Nov 2017 18:24:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6151: add query-level fragment/backend counters

2017-11-07 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8461 )

Change subject: IMPALA-6151: add query-level fragment/backend counters
..


Patch Set 6: Code-Review+2

Carry +2


--
To view, visit http://gerrit.cloudera.org:8080/8461
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3df350414733e98d1ec28adc1c98f45bb0c4e3e9
Gerrit-Change-Number: 8461
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 07 Nov 2017 18:24:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-07 Thread Zoltan Borok-Nagy (Code Review) 
Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h@516
PS7, Line 516:   /// Given a query option name this function gets the option 
level for it to use when
 :   /// displaying from Impala shell and adds the level to the 
ConfigVariable parameter
 :   /// in numeric format. For values see TQueryOptionLevel enum.
nit: For me, this comment is too complex and tells too much implementation 
details. How about something like this:
This function sets the option level for parameter 'option' based on the mapping 
stored in 'query_option_levels_'.
The option level is used by the Impala shell when it displays the options.


http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc@1228
PS7, Line 1228: const string& option_key
Why do we pass option_key here? It is already there in config, isn't it?


http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py@620
PS7, Line 620: self.imp_client.default_query_options,
 :   self.set_query_options,
 :   self.imp_client.query_option_levels
These members are already available in _print_with_set through self. And as far 
as I can see the same members are passed on every callsites.
I think it would be easier to read if _print_with_set didn't take these params.



--
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: 7
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 07 Nov 2017 16:25:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5473: [DOCS] Document TLS min version & cipher options

2017-11-07 Thread Michael Brown (Code Review) 
Michael Brown has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8401 )

Change subject: IMPALA-5473: [DOCS] Document TLS min version & cipher options
..


Patch Set 3: Code-Review+1

Sailesh, if you are happy with this, want to +2 it? Thanks!


--
To view, visit http://gerrit.cloudera.org:8080/8401
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1705262f8c01e38c616541d1c48f5d0cad5498e
Gerrit-Change-Number: 8401
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 07 Nov 2017 16:34:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs

2017-11-07 Thread Zach Amsden (Code Review) 
Zach Amsden has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8456 )

Change subject: IMPALA-6148: Specifying thirdparty deps as URLs
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@112
PS2, Line 112:   return v
Why not cache v instead?



--
To view, visit http://gerrit.cloudera.org:8080/8456
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422
Gerrit-Change-Number: 8456
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 07 Nov 2017 16:45:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5473: [DOCS] Document TLS min version & cipher options

2017-11-07 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8401 )

Change subject: IMPALA-5473: [DOCS] Document TLS min version & cipher options
..


Patch Set 3: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8401
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1705262f8c01e38c616541d1c48f5d0cad5498e
Gerrit-Change-Number: 8401
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 07 Nov 2017 17:05:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-11-07 Thread Thomas Tauber-Marshall (Code Review) 
Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc@204
PS3, Line 204: if (iequals(key, "idle_session_timeout")) {
Its unfortunate this is special cased here. Could you add a comment explaining 
why that's necessary.


http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc@213
PS3, Line 213: key, value,
 :   _->set_query_options,
single line


http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/impala-server.cc@189
PS3, Line 189: DEFINE_int32(idle_session_timeout, 0, "The time, in seconds, 
that a session may be idle"
Note how this interacts with the query option.


http://gerrit.cloudera.org:8080/#/c/8490/3/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:

http://gerrit.cloudera.org:8080/#/c/8490/3/common/thrift/ImpalaService.thrift@292
PS3, Line 292:   // The time, in seconds, that a session may be idle for before 
it is closed (and all
Note how this interacts with the flag.



--
To view, visit http://gerrit.cloudera.org:8080/8490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 07 Nov 2017 17:03:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4835 (prep only): create io subfolder and namespace

2017-11-07 Thread Joe McDonnell (Code Review) 
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 6:

(1 comment)

One small comment

http://gerrit.cloudera.org:8080/#/c/8424/6/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8424/6/be/src/runtime/io/disk-io-mgr.h@184
PS6, Line 184: /// TODO: Break this up the .h/.cc into multiple files under an 
/io subdirectory.
Can remove this TODO



--
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: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 07 Nov 2017 19:39:42 +
Gerrit-HasComments: Yes

<    1   2