[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..


Patch Set 14: Verified+1 Code-Review+2

Carrying Michael's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 14
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 29 Mar 2019 23:49:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Andrew Sherman, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..

IMPALA-7800: Time out new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_client_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 5 minutes. The old behavior
can be restored by setting --accepted_client_cnxn_timeout=0,
i.e., no timeout. The timeout applies only to client facing thrift
servers, i.e., HS2 and Beeswax servers.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
7 files changed, 249 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/14
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 14
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12579/13/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:

http://gerrit.cloudera.org:8080/#/c/12579/13/be/src/rpc/TAcceptQueueServer.cpp@192
PS13, Line 192:
> nit: This can be in the while loop.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 14
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 29 Mar 2019 23:48:46 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..


Patch Set 14: -Verified


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 14
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 29 Mar 2019 23:50:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Andrew Sherman, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..

IMPALA-7800: Time out new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_client_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 5 minutes. The old behavior
can be restored by setting --accepted_client_cnxn_timeout=0,
i.e., no timeout. The timeout applies only to client facing thrift
servers, i.e., HS2 and Beeswax servers.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
7 files changed, 250 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/13
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 13
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..


Patch Set 12:

(1 comment)

Fixed flake8 warning.

http://gerrit.cloudera.org:8080/#/c/12579/12/tests/custom_cluster/test_frontend_connection_limit.py
File tests/custom_cluster/test_frontend_connection_limit.py:

http://gerrit.cloudera.org:8080/#/c/12579/12/tests/custom_cluster/test_frontend_connection_limit.py@86
PS12, Line 86: e
> flake8: F841 local variable 'e' is assigned to but never used
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 12
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 28 Mar 2019 18:16:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Andrew Sherman, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..

IMPALA-7800: Time out new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_client_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 5 minutes. The old behavior
can be restored by setting --accepted_client_cnxn_timeout=0,
i.e., no timeout. The timeout applies only to client facing thrift
servers, i.e., HS2 and Beeswax servers.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
7 files changed, 250 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/12
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 12
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..


Patch Set 11:

(2 comments)

Thanks Michael. Please see PS 12.

http://gerrit.cloudera.org:8080/#/c/12579/11/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:

http://gerrit.cloudera.org:8080/#/c/12579/11/be/src/rpc/TAcceptQueueServer.cpp@130
PS11, Line 130: timeout
> nit: timeout_ms
Done


http://gerrit.cloudera.org:8080/#/c/12579/10/tests/custom_cluster/test_frontend_connection_limit.py
File tests/custom_cluster/test_frontend_connection_limit.py:

http://gerrit.cloudera.org:8080/#/c/12579/10/tests/custom_cluster/test_frontend_connection_limit.py@90
PS10, Line 90:  except Exception as e:
> My understanding is that xfail will just mark it as "unexpected pass" witho
This would work, too. Changed as you suggest.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 11
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 28 Mar 2019 18:04:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 10:

(9 comments)

Thanks Michael. Uploading PS 11

http://gerrit.cloudera.org:8080/#/c/12579/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12579/10//COMMIT_MSG@7
PS10, Line 7: Reject new connections after --fe_service_threads
> Does this need to be updated too ?
Done


http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.h
File be/src/rpc/TAcceptQueueServer.h:

http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.h@65
PS10, Line 65: timeout
> nit: timeout_ms
Done


http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.h@116
PS10, Line 116:   /// Amount of time after which a connection request will be 
timed out.
> in milliseconds
Done


http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:

http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.cpp@263
PS10, Line 263:   if (queue_timeout_ms_ != 0)
  : entry->expiration_time_ = MonotonicMillis() + 
queue_timeout_ms_;
> May need to guard against negative value although it's user's fault in that
Done


http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/thrift-server.h@149
PS10, Line 149: int64_t queue_timeout_ms = 0);
> May make sense to document the meaning of this argument too.
Done


http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/thrift-server.h@197
PS10, Line 197:   /// Amount of time an accepted client connection will be kept 
in the accept
  :   /// queue before it is timed out. Used in TAcceptQueueServer.
> May want to document that if it's 0, it means there is no timeout (similar
Done


http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/thrift-server.h@277
PS10, Line 277:   }
> nit: missing blank line after this function.
Done


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

http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/service/impala-server.cc@239
PS10, Line 239: accepted_cnxn_timeout
> Should this have a more specific name such as "accepted_client_cnxn_timeout
Changed.


http://gerrit.cloudera.org:8080/#/c/12579/10/tests/custom_cluster/test_frontend_connection_limit.py
File tests/custom_cluster/test_frontend_connection_limit.py:

http://gerrit.cloudera.org:8080/#/c/12579/10/tests/custom_cluster/test_frontend_connection_limit.py@90
PS10, Line 90:  except Exception as e:
> How do we catch cases in which the test passes without raising the exceptio
The test will be marked as failed in that case (XFAIL). Added an else block 
with client.close() to not make test verify step wait.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 10
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 26 Mar 2019 22:58:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Time out new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Andrew Sherman, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Time out new connections after --fe_service_threads
..

IMPALA-7800: Time out new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_client_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 5 minutes. The old behavior
can be restored by setting --accepted_client_cnxn_timeout=0,
i.e., no timeout. The timeout applies only to client facing thrift
servers, i.e., HS2 and Beeswax servers.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
7 files changed, 255 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/11
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 11
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 10: Code-Review+1

Carrying Andrew's +1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 10
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 26 Mar 2019 20:20:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Andrew Sherman, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 5 minutes. The old behavior
can be restored by setting --accepted_cnxn_timeout=0, i.e., no
timeout. The timeout applies only to client facing thrift
servers, i.e., HS2 and Beeswax servers.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
7 files changed, 248 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/10
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 10
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 8:

(5 comments)

Thanks for your comments, Andrew. Please see PS 9.

http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.h
File be/src/rpc/TAcceptQueueServer.h:

http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.h@97
PS8, Line 97:   /// Name of the thrift server
> Add terminating period (nit).
Done


http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.h@113
PS8, Line 113:   /// Number of connections rejected due to timeout
> Add terminating period (nit).
Done


http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:

http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.cpp@220
PS8, Line 220:   } catch (string s) {
> I know this isn't part of this change, but can we really ever get string ex
Good question. I am not certain about string exceptions, but the intent here 
seems to be to catch non-thrift exceptions.


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

http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/service/impala-server.cc@241
PS8, Line 241: "the queue before we time it out and reject the connection 
request. A value of 0 "
> In the flags that configure the accept queue we call it "the post-accept, p
Changed the wording. Please let me know if this looks better.


http://gerrit.cloudera.org:8080/#/c/12579/8/tests/custom_cluster/test_frontend_connection_limit.py
File tests/custom_cluster/test_frontend_connection_limit.py:

http://gerrit.cloudera.org:8080/#/c/12579/8/tests/custom_cluster/test_frontend_connection_limit.py@89
PS8, Line 89:   raise ImpalaBeeswaxException(e.message, e)
> I am confused about this. Why do we raise an exception here?  I see that th
We need to catch the exception to close() the session so that the query gets 
unregistered, and re-raise the exception. Otherwise, the async query hangs 
around and makes the test fail. The comment in the above line tries to allude 
to that. Please let me know if I should expand the comment.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 8
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 25 Mar 2019 21:34:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8332: Remove Impala Shell warnings part 1

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12837 )

Change subject: IMPALA-8332: Remove Impala Shell warnings part 1
..


Patch Set 1: Code-Review+1

LGTM


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31f9a0bb12ca6a1da9129eacd29ac105b883e01b
Gerrit-Change-Number: 12837
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 22 Mar 2019 18:05:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 5 minutes. The old behavior
can be restored by setting --accepted_cnxn_timeout=0, i.e., no
timeout. The timeout applies only to client facing thrift
servers, i.e., HS2 and Beeswax servers.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
7 files changed, 245 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/8
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 8
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 7:

(1 comment)

Fixed

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

http://gerrit.cloudera.org:8080/#/c/12579/7/be/src/service/impala-server.cc@240
PS7, Line 240: "(Advanced) The amount of time in milliseconds an accepted 
connection will wait in the "
> line too long (92 > 90)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 19 Mar 2019 02:39:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 5 minutes. The old behavior
can be restored by setting --accepted_cnxn_timeout=0, i.e., no
timeout. The timeout applies only to client facing thrift
servers, i.e., HS2 and Beeswax servers.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
7 files changed, 245 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/7
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 6:

(6 comments)

Addressed your comments, Michael. Please have another look. Thanks.

http://gerrit.cloudera.org:8080/#/c/12579/6/be/src/rpc/TAcceptQueueServer.h
File be/src/rpc/TAcceptQueueServer.h:

http://gerrit.cloudera.org:8080/#/c/12579/6/be/src/rpc/TAcceptQueueServer.h@88
PS6, Line 88: void CleanupAndClose(const std::string& error,
> Please add a brief comment about what this function does and what those par
Done


http://gerrit.cloudera.org:8080/#/c/12579/6/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:

http://gerrit.cloudera.org:8080/#/c/12579/6/be/src/rpc/TAcceptQueueServer.cpp@29
PS6, Line 29: DEFINE_int64(accepted_cnxn_timeout, 300,
: "(Advanced) The amount of time in seconds an accepted 
connection will wait in the "
: "queue before we time it out and reject the connection 
request. A value of 0 "
: "means there is no timeout.");
> Sorry but now that I realize this may affect all existing Thrift based serv
Changed the implementation to pass the timeout value via the constructor chain 
from ImpalaServer. Now only HS2 and Beeswax servers have the timeout.


http://gerrit.cloudera.org:8080/#/c/12579/6/be/src/rpc/TAcceptQueueServer.cpp@195
PS6, Line 195: wait_time
> nit: coding convention in Impala usually keeps the unit as the suffix of th
Done


http://gerrit.cloudera.org:8080/#/c/12579/6/be/src/rpc/TAcceptQueueServer.cpp@205
PS6, Line 205: msecs
> nit: we either use ms or milliseconds.
Done


http://gerrit.cloudera.org:8080/#/c/12579/6/be/src/rpc/TAcceptQueueServer.cpp@207
PS6, Line 207: (wait_result == THRIFT_ETIMEDOUT)
> Do we care about any potential non-zero result value ? Should we bail out i
It does not seem to return any other non-zero value besides THRIFT_TIMEDOUT. 
https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/concurrency/Monitor.cpp#L78


http://gerrit.cloudera.org:8080/#/c/12579/6/be/src/rpc/TAcceptQueueServer.cpp@265
PS6, Line 265:   if (FLAGS_accepted_cnxn_timeout != 0)
 : entry->expiration_time_ =
 : MonotonicMillis() + FLAGS_accepted_cnxn_timeout * 
MILLIS_PER_SEC;
> nit styling:
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 19 Mar 2019 02:36:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 300 seconds. The old behavior
can be restored by setting --accepted_cnxn_timeout=0, i.e., no
timeout.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
4 files changed, 212 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/6
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/12579/5/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:

http://gerrit.cloudera.org:8080/#/c/12579/5/be/src/rpc/TAcceptQueueServer.cpp@198
PS5, Line 198: if (entry->expiration_time_ != 0)
 :   wait_time = entry->expiration_time_ - 
MonotonicMillis();
> styling nit:
Done


http://gerrit.cloudera.org:8080/#/c/12579/5/be/src/rpc/TAcceptQueueServer.cpp@200
PS5, Line 200: LOG(INFO) << "All " << maxTasks_ << " server threads are 
in use. "
 :   << "Waiting for " << wait_time << " msecs.";
> Does it make sense to log only if we are timing out a request below ?
Why I am keeping it here is so we can diagnose if clients are potentially 
waiting for long periods for fe threads, even if timeout was infinite. A purely 
diagnostic log. So we could say, "If you want client connections to fail 
faster, please set connection timeout to x seconds".

I can use a LOG_EVERY_N kind of macro to reduce the log pollution. Let me know 
what you think.


http://gerrit.cloudera.org:8080/#/c/12579/5/be/src/rpc/TAcceptQueueServer.cpp@263
PS5, Line 263:   MonotonicMillis() + FLAGS_accepted_cnxn_timeout * 
MILLIS_PER_SEC;
> nit: indent 4
Done


http://gerrit.cloudera.org:8080/#/c/12579/5/be/src/rpc/TAcceptQueueServer.cpp@267
PS5, Line 267: connection_setup_pool.Offer(
> In theory, this could block too but it's still counted towards the expirati
This is a very interesting question. Regardless of where we spent time, 
expiration_time_ could've elapsed by the time we got around to the wait in 
SetupConnection. I'm leaning towards waiting for a MAX(1, expiration_time_ - 
MonotonicMillis()). What do you think?


http://gerrit.cloudera.org:8080/#/c/12579/5/tests/custom_cluster/test_frontend_connection_limit.py
File tests/custom_cluster/test_frontend_connection_limit.py:

http://gerrit.cloudera.org:8080/#/c/12579/5/tests/custom_cluster/test_frontend_connection_limit.py@1
PS5, Line 1:
> nit: blank line
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 05 Mar 2019 23:46:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 300 seconds. The old behavior
can be restored by setting --accepted_cnxn_timeout=0, i.e., no
timeout.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
4 files changed, 209 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/5
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 3:

(10 comments)

Thanks for the review. Uploading PS 4

http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:

http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@28
PS3, Line 28: 120,
> Will it be safer to have a higher default timeout (e.g. 300 seconds) to avo
I've thought about this a bit, and came up with this value which I think is at 
the threshold of tolerance for a user trying to run an interactive query. But 
300s is fine for me to.


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@151
PS3, Line 151: const string& error)
> nit: we tend to put the constant parameters followed by non-constant parame
Done


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@193
PS3, Line 193: 0LL
> nit: 'LL' not needed.
Done


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@196
PS3, Line 196: if (entry->expiration_time_)
> nit: if (entry->expiration_time_ != 0) {
Done


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@197
PS3, Line 197: wait_time = entry->expiration_time_ - MonotonicMillis();
> Shouldn't this be inside the while loop below ?
Done


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@199
PS3, Line 199: LOG(INFO) << "All " << maxTasks_ << " server threads are 
in use. "
 :   << "Waiting for " << wait_time << " msecs.";
> Will this lead to log spam ?
Potentially, if we're constantly maxing out fe threads and new connections keep 
coming. I can reduce the log frequency. Let me know if you feel strongly about 
this.


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@204
PS3, Line 204: Timing out connection request.";
> Any chance we can print some details (e.g. IP address, port) here ?
I haven't seen a 'simple' way to do this, but definitely would like to add more 
info about the client as well as 'which' server this is, e.g., beeswax or hs2.


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@239
PS3, Line 239: shared_ptr
> This can be "unique_ptr" now that TTransport lives inside TAcceptQueueEntry
Tried this, but since unique_ptr is not copyable compilation fails at line 242 
(calling a destroyed object). Tried to massage it a bit, but readability gets 
compromised. Will stick with shared_ptr for now.

The shared_ptr gets moved to the SetupConnection thread anyway so hopefully 
this is not too obscure.


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@260
PS3, Line 260: if (FLAGS_accepted_cnxn_timeout)
> nit: if (FLAGS_accepted_cnxn_timeout != 0) {
Done


http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@261
PS3, Line 261: MILLIS_PER_SEC;
> long line.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 01 Mar 2019 00:38:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 120 seconds. The old behavior
can be restored by setting --accepted_cnxn_timeout=0, i.e., no
timeout.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
4 files changed, 209 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/4
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-8240: Event processor should keep trying when metastore is unavailable.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12601 )

Change subject: IMPALA-8240: Event processor should keep trying when metastore 
is unavailable.
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12601/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/12601/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@328
PS1, Line 328:   LOG.error("Unable to fetch the next batch of metastore 
events", fetchEx);
Can you make the error log state that the metastore might be unavailable, and 
that we're going to keep retrying?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I570a52462e6d3b634b2c227dfcb98e20ad2a0023
Gerrit-Change-Number: 12601
Gerrit-PatchSet: 1
Gerrit-Owner: Anurag Mantripragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vihang Karajgaonkar 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 26 Feb 2019 22:45:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Marshall, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 120 seconds. The old behavior
can be restored by setting --accepted_cnxn_timeout=0, i.e., no
timeout.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
4 files changed, 204 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/3
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12579 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 2:

(11 comments)

Fixed

http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py
File tests/custom_cluster/test_frontend_connection_limit.py:

http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@19
PS2, Line 19: import sys
> flake8: F401 'sys' imported but unused
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@23
PS2, Line 23: from getpass import getuser
> flake8: F401 'getpass.getuser' imported but unused
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@64
PS2, Line 64:
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@64
PS2, Line 64:
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@64
PS2, Line 64:
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@64
PS2, Line 64:
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@65
PS2, Line 65:
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@65
PS2, Line 65:
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@65
PS2, Line 65:
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@65
PS2, Line 65:
> flake8: E251 unexpected spaces around keyword / parameter equals
Done


http://gerrit.cloudera.org:8080/#/c/12579/2/tests/custom_cluster/test_frontend_connection_limit.py@91
PS2, Line 91:
> flake8: E261 at least two spaces before inline comment
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 26 Feb 2019 18:39:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/12579


Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control how the server
should treat new connection requests if we have run out of the
configured number of server threads.

If --accepted_cnxn_timeout > 0, new connection requests are
rejected by the server if we can't get a server thread within
the specified timeout.

We set the default timeout to be 120 seconds. The old behavior
can be restored by setting --accepted_cnxn_timeout=0, i.e., no
timeout.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core and exhaustive tests.

Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M common/thrift/metrics.json
A tests/custom_cluster/test_frontend_connection_limit.py
4 files changed, 206 insertions(+), 26 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12579/2
--
To view, visit http://gerrit.cloudera.org:8080/12579
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64
Gerrit-Change-Number: 12579
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has abandoned this change. ( http://gerrit.cloudera.org:8080/12226 
)

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Abandoned

Changing the design.
--
To view, visit http://gerrit.cloudera.org:8080/12226
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I4b9d48aaff9e3b5854b5121798211c641c58a559
Gerrit-Change-Number: 12226
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-8187: UDF samples hide symbols by default

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12451 )

Change subject: IMPALA-8187: UDF samples hide symbols by default
..


Patch Set 3: Code-Review+1

(1 comment)

Thanks for doing this! LGTM.

http://gerrit.cloudera.org:8080/#/c/12451/3/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:

http://gerrit.cloudera.org:8080/#/c/12451/3/tests/query_test/test_udfs.py@440
PS3, Line 440: create function `{0}`.unexported() returns BIGINT 
LOCATION '{1}'
Should this be unexported_symbol()?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie17b74b9bce9cc6962393017879b65409ce23b28
Gerrit-Change-Number: 12451
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 13 Feb 2019 00:33:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7565: Set TAcceptQueueServer connection setup pool to be multi-threaded by default

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12249 )

Change subject: IMPALA-7565: Set TAcceptQueueServer connection_setup_pool to be 
multi-threaded by default
..


Patch Set 1: Code-Review+1

LGTM


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I053120d4c3153ddbe5261acd28388be6cd191908
Gerrit-Change-Number: 12249
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 25 Jan 2019 00:43:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12226 )

Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..


Patch Set 6:

> I meant "the timeout can also be turned off by setting it to 0 or
 > something by user". We will have a non-zero timeout by default.

Given the feedback so far, I wonder if we need to go back to the drawing board 
and re-think the approach?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b9d48aaff9e3b5854b5121798211c641c58a559
Gerrit-Change-Number: 12226
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 23 Jan 2019 20:54:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/12226


Change subject: IMPALA-7800: Reject new connections after --fe_service_threads
..

IMPALA-7800: Reject new connections after --fe_service_threads

The current implementation of the FE thrift server waits
indefinitely to open the new session, if the maximum number of
FE service threads specified by --fe_service_threads has been
allocated.

This patch introduces a startup flag to control
how the server should treat new connection requests if we have
run out of the configured number of server threads.

By default we retain the current behavior. If
--reject_request_on_max_fe_threads=true, however, the new
connection requests are rejected by the server.

Testing:

Added a new custom cluster test suite to exercise the
new code.

Ran core tests.

Change-Id: I4b9d48aaff9e3b5854b5121798211c641c58a559
---
M be/src/rpc/TAcceptQueueServer.cpp
A tests/custom_cluster/test_frontend_connection_limit.py
2 files changed, 115 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/12226/6
--
To view, visit http://gerrit.cloudera.org:8080/12226
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4b9d48aaff9e3b5854b5121798211c641c58a559
Gerrit-Change-Number: 12226
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12142 )

Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
..


Patch Set 3: Code-Review+1

(4 comments)

http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:

http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h@294
PS1, Line 294:   Status DoCancelQueryFInstancesRrpcWithRetry(
> Up to now I have just been using what the clang-format rules dictate. In cl
Actually, I wasn't aware that we had these style rules defined in the 
clang-format files. I was only commenting on this aspect when viewing the 
changed code in relation to the existing code. I would just stick with what the 
existing code is following.


http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:

http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h@104
PS1, Line 104:   const TNetworkAddress& krpc_host, int 
per_fragment_instance_idx,
> There is no trailing whitespace I could find.
Ok. Thanks.


http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:

http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@163
PS1, Line 163:   optional UniqueIdPB query_id = 1;
> OK you made me think!
That makes perfect sense. Thanks for the link!


http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@179
PS1, Line 179:   // Cancellation is asynchronous (in the sense that this call 
may return before the
> When the coordinator cancels a query it sends CancelQueryFInstancesRequestP
Thanks for clarifying that.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 03 Jan 2019 19:45:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7468: Port CancelQueryFInstances() to KRPC.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12142 )

Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC.
..


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:

http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h@294
PS1, Line 294:   Status DoCancelQueryFInstancesRrpcWithRetry(
Nit: The prevailing style seems to place reference ('&') and pointer ('*') 
symbols next to the pointed-to-type, rather than next to the variable name. 
Comment applies for the entire review.


http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:

http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h@104
PS1, Line 104:   const TNetworkAddress& krpc_host, int 
per_fragment_instance_idx,
Nit: I wonder if there are trailing whitespaces in these lines? I don't 
understand why some lines are marked as changed...


http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:

http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.h@433
PS1, Line 433:   void ComputeFragmentExecParams(const BackendConfig 
executor_config,
Nit: Please retain existing placement of '&' and '*'.

Also: Are you intentionally passing executor_config by value? Here and in the 
following functions?


http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.cc@333
PS1, Line 333: ComputeFragmentExecParams(executor_config, plan_exec_info,
Nit: Trailing spaces?


http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:

http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@163
PS1, Line 163:   optional UniqueIdPB query_id = 1;
Don't understand how the query id can be optional...


http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@179
PS1, Line 179:   // Cancellation is asynchronous.
Commit message says cancellation is synchronous. Do you want to change this 
comment until async cancellation is implemented?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969
Gerrit-Change-Number: 12142
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 02 Jan 2019 19:29:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6664: Tag log statements with fragment ids.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12129 )

Change subject: IMPALA-6664: Tag log statements with fragment ids.
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12129/1/be/src/common/logging.cc
File be/src/common/logging.cc:

http://gerrit.cloudera.org:8080/#/c/12129/1/be/src/common/logging.cc@55
PS1, Line 55: void PrependFragment(std::string* s, bool* changed) {
Nice. I think the next step would be to (re)define VLOG_QUERY or something like 
it, that would be called for debug logging in the query/FINST lifecycle code?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6634ef9d1a7346339f24f2d40a7a3aa36a535da8
Gerrit-Change-Number: 12129
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 02 Jan 2019 17:52:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7960: Revert "IMPALA-5929: Remove redundant explicit casts to string"

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12073 )

Change subject: IMPALA-7960: Revert "IMPALA-5929: Remove redundant explicit 
casts to string"
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f0da62a7ff86f05859a2acbec13a726a9bd6f4c
Gerrit-Change-Number: 12073
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 12 Dec 2018 00:25:40 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6741: Add timestamp of fragment instance's status updates

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12000 )

Change subject: IMPALA-6741: Add timestamp of fragment instance's status updates
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae3dcddc292d694d7003d10ed0caccfceed7d8fa
Gerrit-Change-Number: 12000
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Balazs Jeszenszky 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 29 Nov 2018 19:16:22 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6741: Add timestamp of fragment instance's status updates

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12000 )

Change subject: IMPALA-6741: Add timestamp of fragment instance's status updates
..


Patch Set 2: Code-Review+1

LGTM.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae3dcddc292d694d7003d10ed0caccfceed7d8fa
Gerrit-Change-Number: 12000
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Balazs Jeszenszky 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 29 Nov 2018 00:07:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6741: Add timestamp of fragment instance's status updates

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12000 )

Change subject: IMPALA-6741: Add timestamp of fragment instance's status updates
..


Patch Set 1:

(3 comments)

Thanks for doing this.

http://gerrit.cloudera.org:8080/#/c/12000/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/12000/1/be/src/runtime/coordinator-backend-state.cc@47
PS1, Line 47: "Last report time";
I am nit-picking somewhat, but this is really 'Last received time' from the 
coordinator's PoV, right? I wonder if it might be better to rename it as such, 
so that we don't have to explain what the metric means. I don't feel too 
strongly about this, so I leave it to you.


http://gerrit.cloudera.org:8080/#/c/12000/1/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/12000/1/tests/query_test/test_observability.py@95
PS1, Line 95:   if report_time < report_time_dict.get(node.name, 
datetime.min):
What happens here the first time through, when node.name does not yet exist in 
the dict?


http://gerrit.cloudera.org:8080/#/c/12000/1/tests/query_test/test_observability.py@111
PS1, Line 111: assert time_backward <= ceil(elapsed_time / 
MIN_NTP_POLL_PERIOD)
Should this be num_time_backward? time_backward is a boolean, and shouldn't be 
used outside of the above while loop.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae3dcddc292d694d7003d10ed0caccfceed7d8fa
Gerrit-Change-Number: 12000
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Balazs Jeszenszky 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 28 Nov 2018 21:25:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7714: remove unsafe code from signal handlers

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11777 )

Change subject: IMPALA-7714: remove unsafe code from signal handlers
..


Patch Set 5: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11777/5/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/11777/5/be/src/common/init.cc@192
PS5, Line 192:   sys_write(STDOUT_FILENO, msg, strlen(msg));
One question: Does this mean that we can't log the shutdown message to Impala 
log file? Or does STDOUT_FILENO get duped to impalad.INFO?


http://gerrit.cloudera.org:8080/#/c/11777/5/be/src/util/minidump.cc
File be/src/util/minidump.cc:

http://gerrit.cloudera.org:8080/#/c/11777/5/be/src/util/minidump.cc@102
PS5, Line 102:   sys_write(STDOUT_FILENO, msg, strlen(msg));
Same question as in the other .cc file.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52037d6510b9b34ec33d3a8b5492226aee1b9d92
Gerrit-Change-Number: 11777
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 25 Oct 2018 17:10:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-589: Add a sql function to return the impalad coordinator hostname for diagnostic purposes.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11459 )

Change subject: IMPALA-589: Add a sql function to return the impalad 
coordinator hostname for diagnostic purposes.
..


Patch Set 1: Code-Review+1

(1 comment)

Change looks good to me. It would be great to have the function take an 
optional query id argument so we can use it on a running query as well. But I 
realize that that's a lot of work, and the same info is discoverable from the 
CM.

http://gerrit.cloudera.org:8080/#/c/11459/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/11459/1/be/src/exprs/expr-test.cc@4961
PS1, Line 4961:   ObjectPool pool;
This does not seem to be used.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 19 Sep 2018 23:06:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3819: Clarify stale block metadata warning message

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11433 )

Change subject: IMPALA-3819: Clarify stale block metadata warning message
..


Patch Set 1: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11433/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/11433/1/be/src/exec/hdfs-scan-node-base.cc@913
PS1, Line 913:   runtime_state_->LogError(ErrorMsg(TErrorCode::GENERAL, 
Substitute(
> These warnings show up in the runtime profile (warnings section) and the sh
Ok. Your explanation makes sense. I rest my case :)


http://gerrit.cloudera.org:8080/#/c/11433/1/be/src/exec/hdfs-scan-node-base.cc@914
PS1, Line 914:   "Read $0 of data across network that was expected to 
be local. Block locality "
> Same as above.
Ok.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9eb2672393c5ff4e0670d247d684278283012a3
Gerrit-Change-Number: 11433
Gerrit-PatchSet: 1
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 12 Sep 2018 20:39:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3819: Clarify stale block metadata warning message

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11433 )

Change subject: IMPALA-3819: Clarify stale block metadata warning message
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11433/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/11433/1/be/src/exec/hdfs-scan-node-base.cc@913
PS1, Line 913:   runtime_state_->LogError(ErrorMsg(TErrorCode::GENERAL, 
Substitute(
If this does not result in incorrect query results, I wonder if logging it as 
an error is appropriate. IMO this is probably a WARN or INFO message.


http://gerrit.cloudera.org:8080/#/c/11433/1/be/src/exec/hdfs-scan-node-base.cc@914
PS1, Line 914:   "Read $0 of data across network that was expected to 
be local. Block locality "
Worth prefixing the message with the query/Finst Id?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9eb2672393c5ff4e0670d247d684278283012a3
Gerrit-Change-Number: 11433
Gerrit-PatchSet: 1
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 12 Sep 2018 20:17:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1760: Implement shutdown command

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10744 )

Change subject: IMPALA-1760: Implement shutdown command
..


Patch Set 18:

(3 comments)

Thanks for doing this.

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

http://gerrit.cloudera.org:8080/#/c/10744/18/be/src/service/impala-server.h@108
PS18, Line 108: /// 2. The startup grace period starts, during which:
Can this be based instead on a successful startup indicator/flag that gets set 
when the daemon is considered to be fully started up?


http://gerrit.cloudera.org:8080/#/c/10744/18/be/src/service/impala-server.h@119
PS18, Line 119: ///executing). If it is quiesced then it cleanly shut downs 
by exiting the process.
nit: ...cleanly shuts down...


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

http://gerrit.cloudera.org:8080/#/c/10744/18/be/src/service/impala-server.cc@2408
PS18, Line 2408:   if (set_grace) {
nit: online?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0
Gerrit-Change-Number: 10744
Gerrit-PatchSet: 18
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 10 Sep 2018 18:20:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7542: fix find-fragment-instances to find all "root threads"

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11396 )

Change subject: IMPALA-7542: fix find-fragment-instances to find all "root 
threads"
..


Patch Set 2: Code-Review+1

Thanks for fixing the bug! Change LGTM.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I35ae1a6b384b002b343689469f02ceabd84af1b6
Gerrit-Change-Number: 11396
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 07 Sep 2018 20:35:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Philip Zeyliger, David Knupp, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7508: Add Impala Python GDB module
..

IMPALA-7508: Add Impala Python GDB module

This patch adds a new Impala Python GDB module, and implements
a couple of covenience commands to make core dump analysis
easier.

The initial commands let us find queries and fragment instances
currently executing in an impalad at the time the daemon crashed:

(gdb) source impala-gdb.py
(gdb) find-query-ids
f74c863dff66a34d:1d983cc3
364525e12495932b:73f5dd02
bc4a3eec25481981:edda04b8

(gdb) find-fragment-instances
Fragment Instance IdThread IDs

364525e12495932b:73f5dd0200a2   [69]
364525e12495932b:73f5dd020171   [196, 136]
bc4a3eec25481981:edda04b801a8   [252, 237, 206]
f74c863dff66a34d:1d983cc3009b   [200, 14, 13, 12, 6, 5, 3, 2]
f74c863dff66a34d:1d983cc3013a   [4]

The commands have only been tested with Impala 2.12, and are not
expected to work with older versions since it uses ThreadDebugInfo
stuff from IMPALA-6416.

It is hoped that people contribute more commands to the module.

Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
---
M bin/rat_exclude_files.txt
A lib/python/impala_py_lib/gdb/README.md
A lib/python/impala_py_lib/gdb/impala-gdb.py
3 files changed, 143 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11352/4
--
To view, visit http://gerrit.cloudera.org:8080/11352
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11352 )

Change subject: IMPALA-7508: Add Impala Python GDB module
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11352/2/lib/python/impala_py_lib/gdb/impala-gdb.py
File lib/python/impala_py_lib/gdb/impala-gdb.py:

http://gerrit.cloudera.org:8080/#/c/11352/2/lib/python/impala_py_lib/gdb/impala-gdb.py@30
PS2, Line 30: def get_fragment_instances():
> flake8: E302 expected 2 blank lines, found 1
Fixed.


http://gerrit.cloudera.org:8080/#/c/11352/2/lib/python/impala_py_lib/gdb/impala-gdb.py@44
PS2, Line 44: s
> flake8: F841 local variable 'symbol' is assigned to but never used
Fixed



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 30 Aug 2018 22:26:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Philip Zeyliger, David Knupp, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7508: Add Impala Python GDB module
..

IMPALA-7508: Add Impala Python GDB module

This patch adds a new Impala Python GDB module, and implements
a couple of covenience commands to make core dump analysis
easier.

The initial commands let us find queries and fragment instances
currently executing in an impalad at the time the daemon crashed:

(gdb) source impala-gdb.py
(gdb) find-query-ids
f74c863dff66a34d:1d983cc3
364525e12495932b:73f5dd02
bc4a3eec25481981:edda04b8

(gdb) find-fragment-instances
Fragment Instance IdThread IDs

364525e12495932b:73f5dd0200a2   [69]
364525e12495932b:73f5dd020171   [196, 136]
bc4a3eec25481981:edda04b801a8   [252, 237, 206]
f74c863dff66a34d:1d983cc3009b   [200, 14, 13, 12, 6, 5, 3, 2]
f74c863dff66a34d:1d983cc3013a   [4]

The commands have only been tested with Impala 2.12, and are not
expected to work with older versions since it uses ThreadDebugInfo
stuff from IMPALA-6416.

It is hoped that people contribute more commands to the module.

Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
---
A lib/python/impala_py_lib/gdb/README.md
A lib/python/impala_py_lib/gdb/impala-gdb.py
2 files changed, 142 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11352/3
--
To view, visit http://gerrit.cloudera.org:8080/11352
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Philip Zeyliger, David Knupp, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7508: Add Impala Python GDB module
..

IMPALA-7508: Add Impala Python GDB module

This patch adds a new Impala Python GDB module, and implements
a couple of covenience commands to make core dump analysis
easier.

The initial commands let us find queries and fragment instances
currently executing in an impalad at the time the daemon crashed:

(gdb) source impala-gdb.py
(gdb) find-query-ids
f74c863dff66a34d:1d983cc3
364525e12495932b:73f5dd02
bc4a3eec25481981:edda04b8

(gdb) find-fragment-instances
Fragment Instance IdThread IDs

364525e12495932b:73f5dd0200a2   [69]
364525e12495932b:73f5dd020171   [196, 136]
bc4a3eec25481981:edda04b801a8   [252, 237, 206]
f74c863dff66a34d:1d983cc3009b   [200, 14, 13, 12, 6, 5, 3, 2]
f74c863dff66a34d:1d983cc3013a   [4]

The commands have only been tested with Impala 2.12, and are not
expected to work with older versions since it uses ThreadDebugInfo
stuff from IMPALA-6416.

It is hoped that people contribute more commands to the module.

Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
---
A lib/python/impala_py_lib/gdb/README.md
A lib/python/impala_py_lib/gdb/impala-gdb.py
2 files changed, 140 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11352/2
--
To view, visit http://gerrit.cloudera.org:8080/11352
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11352 )

Change subject: IMPALA-7508: Add Impala Python GDB module
..


Patch Set 1:

(10 comments)

Thanks for the comments. Uploading PS#2.

http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py
File infra/gdb/impala-gdb.py:

http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@1
PS1, Line 1: #
> I feel like this file should more properly be added to:
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@1
PS1, Line 1: #
> That would be fine by me.
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@22
PS1, Line 22: import gdb
> Please add a README.md in this directory or include usage here.
Added a README.md.


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@28
PS1, Line 28: class ImpalaGDB:
> I think we usually use new-style classes.
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@29
PS1, Line 29: # Walk the threadlist to find fragment instance ids. Assumes 
IMPALA-6416, so
> Use triple " for docstring.
I meant this as a comment, not a doc string. Leaving is as is.


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@34
PS1, Line 34: def getFragmentInstances(self):
> Python's convention is to use snake_case instead of camelCase.
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@40
PS1, Line 40: while f is not None
> nit: can be simplified to while f:
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@62
PS1, Line 62: class FindFragmentInstances(gdb.Command, ImpalaGDB):
: "Find all query fragment instance to thread Id mappings in 
this impalad."
:
: def __init__(self):
: super(FindFragmentInstances, self).__init__(
> multiple-inheritance and inheritance in general seem unnecessary here.
Thanks! I wasn't happy with the inheritance either. Made 
get_fragment_instances() a free function.


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@80
PS1, Line 80: "Find IDs of all queries this impalad is currently executing."
> Use triple " for docstring.
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@95
PS1, Line 95: qid_hi + ':' + qid_low
> nit: usually it's preferable to use string format instead of +
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 30 Aug 2018 22:11:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11352


Change subject: IMPALA-7508: Add Impala Python GDB module
..

IMPALA-7508: Add Impala Python GDB module

This patch adds a new Impala Python GDB module, and implements
a couple of covenience commands to make core dump analysis
easier.

The initial commands let us find queries and fragment instances
currently executing in an impalad at the time the daemon crashes:

(gdb) source impala-gdb.py
(gdb) find-query-ids
f74c863dff66a34d:1d983cc3
364525e12495932b:73f5dd02
bc4a3eec25481981:edda04b8

(gdb) find-fragment-instances
Fragment Instance IdThread IDs

364525e12495932b:73f5dd0200a2   [69]
364525e12495932b:73f5dd020171   [196, 136]
bc4a3eec25481981:edda04b801a8   [252, 237, 206]
f74c863dff66a34d:1d983cc3009b   [200, 14, 13, 12, 6, 5, 3, 2]
f74c863dff66a34d:1d983cc3013a   [4]

The commands have only been tested with Impala 2.12, and are not
expected to work with older versions since it uses ThreadDebugInfo
stuff from IMPALA-6416.

It is hoped that people contribute more commands to the module.

Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
---
A infra/gdb/impala-gdb.py
1 file changed, 101 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/11352/1
--
To view, visit http://gerrit.cloudera.org:8080/11352
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

Hello Michael Ho, anujphadke, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..

IMPALA-7444: Improve logging of opening/closing/expiring sessions.

Recent troubleshooting efforts have shown we can improve
logging of client session opening and expiry processing to
enhance serviceability.

This patch adds minor, but useful debug log improvements.

Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
2 files changed, 46 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/11234/4
--
To view, visit http://gerrit.cloudera.org:8080/11234
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

Hello Michael Ho, anujphadke, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..

IMPALA-7444: Improve logging of opening/closing/expiring sessions.

Recent troubleshooting efforts have shown we can improve
logging of client session opening and expiry processing to
enhance serviceability.

This patch adds minor, but useful debug log improvements.

Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
2 files changed, 47 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/11234/3
--
To view, visit http://gerrit.cloudera.org:8080/11234
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11234 )

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/11234/2/be/src/service/impala-server.cc@1962
PS2, Line 1962: expired_cnt++;
> ++expired_cnt;
Done


http://gerrit.cloudera.org:8080/#/c/11234/2/be/src/service/impala-server.cc@1975
PS2, Line 1975: LOG_IF(INFO, expired_cnt > 0) << "Expired sessions. Count: 
" << expired_cnt
  :   << ". Time: " << UnixMillis() 
- now << " milliseconds.";
> What's the purpose of this line ? Won't we be able to tell that from the "E
The idea is to gauge how long session_state_map_lock_ is held in the process of 
expiring sessions. Probably less of an issue today after stack symbolization is 
removed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 16 Aug 2018 23:00:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11234 )

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..


Patch Set 1:

(2 comments)

> Just being paranoid here but I hope this doesn't make the log too
 > verbose.

It's definitely more verbose than before, but I think it's worth it. 
Ordinarily, sessions are opened and stay around for a long time compared to 
individual queries. We would ordinarily see a lot of other messages in the log 
between open and close sessions.

http://gerrit.cloudera.org:8080/#/c/11234/1/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/11234/1/be/src/service/impala-hs2-server.cc@367
PS1, Line 367: Done opening
> Opened
Done


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

http://gerrit.cloudera.org:8080/#/c/11234/1/be/src/service/impala-server.cc@1236
PS1, Line 1236: Done closing
> Closed
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 16 Aug 2018 05:24:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

Hello Michael Ho, anujphadke, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..

IMPALA-7444: Improve logging of opening/closing/expiring sessions.

Recent troubleshooting efforts have shown we can improve
logging of client session opening and expiry processing to
enhance serviceability.

This patch adds minor, but useful debug log improvements.

Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
2 files changed, 47 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/11234/2
--
To view, visit http://gerrit.cloudera.org:8080/11234
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-7014: Disable stacktrace symbolisation by default

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10964


Change subject: IMPALA-7014: Disable stacktrace symbolisation by default
..

IMPALA-7014: Disable stacktrace symbolisation by default

Stacktrace symbolization has been shown to be 2500x slower
compared to just printing the un-symbolized one.

This has burned us a few times now, so let's disable it by
default.

Change-Id: If3af209890ccc242beb742145c63eb6836d4bfbb
---
M be/src/common/init.cc
1 file changed, 2 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/10964/1
--
To view, visit http://gerrit.cloudera.org:8080/10964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If3af209890ccc242beb742145c63eb6836d4bfbb
Gerrit-Change-Number: 10964
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 5: Code-Review+1

LGTM


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 17 Jul 2018 17:15:41 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10850 )

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 5: Code-Review+1

(1 comment)

Fixed the indentation. Thanks for the review.

http://gerrit.cloudera.org:8080/#/c/10850/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10850/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2183
PS4, Line 2183:   Type retType, String uriPath, String symbolName) {
> nit: incorrect indentation
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 13 Jul 2018 16:15:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at least SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

Testing:
New FE test cases have been added to AuthorizationStmtTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran exhaustive and covering tests.

Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 50 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/10850/5
--
To view, visit http://gerrit.cloudera.org:8080/10850
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10850 )

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/10850/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10850/3//COMMIT_MSG@10
PS3, Line 10: leas
> nit: least
Done


http://gerrit.cloudera.org:8080/#/c/10850/3//COMMIT_MSG@18
PS3, Line 18:
> add a "Testing: " section header.
Done


http://gerrit.cloudera.org:8080/#/c/10850/3//COMMIT_MSG@24
PS3, Line 24:
: Ran exhaustive and
> nit: replace with "Ran exhaustive and covering tests."
Done


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@481
PS3, Line 481: LOG.trace("rewrittenStmt: " + analysisResult_.stmt_.toSql());
> ok. looks like this change is an improvement over the current behavior. pls
Filed https://issues.apache.org/jira/browse/IMPALA-7285


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2070
PS3, Line 2070:
> nit: remove
Done


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2074
PS3, Line 2074: fo
> nit: remove
Done


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2085
PS3, Line 2085: .ok(onServer(TPrivilegeLevel
> move above L2082?
Changed the text so that the context is clearer.


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2153
PS3, Line 2153: ivate static String alterError(String object) {
  : return "User '%s' does not have privileges to execute 
'ALTER' on: " + object;
  :   }
  :
> replace with a call to the more generic function on L2160
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 12 Jul 2018 23:37:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at least SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

Testing:
New FE test cases have been added to AuthorizationStmtTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran exhaustive and covering tests.

Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 50 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/10850/4
--
To view, visit http://gerrit.cloudera.org:8080/10850
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7288: Fix Codegen Crash in FinalizeModule()

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10933 )

Change subject: IMPALA-7288: Fix Codegen Crash in FinalizeModule()
..


Patch Set 1:

> I am not sure what kind of tests I can add to this, since this
 > looks more like an abuse of API than an actual functionality that i
 > can test. any ideas welcomed

How about the test case that repro'd this? I mean the one Balasz posted?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f0b527909a9fb3090996bb7510e4d58350c21b0
Gerrit-Change-Number: 10933
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 12 Jul 2018 22:50:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 4:

Can you also add test case to look for the SIGTERM induced 'shutdown log' in 
the log file? Since that's the whole point of adding the handler, I think it 
would be good to ensure we don't lose the log message.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 11 Jul 2018 23:03:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10850 )

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@481
PS3, Line 481: LOG.trace("rewrittenStmt: " + analysisResult_.stmt_.toSql());
> supposing the folded fn reveals something interesting, e.g., getSSN("some u
I think that's the right approach in general: Parse the statement, then check 
access, then perform analysis. I think that would be well outside the scope of 
this JIRA, though. Please also note that this code was existing.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 11 Jul 2018 22:59:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@64
PS2, Line 64:LOG(INFO) << "SIGTERM encountered invoking default handler 
system going down" << endl;
I would change the wording to "Caught SIGTERM. Daemon going down."


http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@108
PS2, Line 108: old_action
> Do we currently set a handler for SIGTERM elsewhere? If not, we should remo
Please add a comment explaining why SIGTERM is specifically handled (i.e., we 
want to log a message when Impalad/statestored/catalogd is being shut down via 
the signal for whatever reason).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 06 Jul 2018 23:56:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10850 )

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 2:

(6 comments)

Thanks. Please see PS#3.

http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2084
PS2, Line 2084:
> nit: add two extra spaces for continued indentation
Done


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2087
PS2, Line 2087:   "select functional.to_lower('ABCDEF')")
> nit: move L2088 to L2087
Done


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2091
PS2, Line 2091: .ok(onDatabase("functional", TPrivilegeLevel.ALL))
> nit: move .ok(onDatabase("functional", TPrivilegeLevel.ALL)) to be at the b
Done


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2092
PS2, Line 2092: .ok(onDatabase("functional", TPrivilegeLevel.INSERT))
  : .ok(onDatabase("functional", 
TPrivilegeLevel.REFRESH))
> INSERT, REFRESH, and SELECT can be simplified to .ok(onDatabase("functional
Done. Want me to wrap ALL under viewMetadataPrivileges() as well?


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2096
PS2, Line 2096: TPrivilegeLevel.SELECT, TPrivilegeLevel.ALL,
  : TPrivilegeLevel.INSERT, 
TPrivilegeLevel.REFRESH
> can be simplified to allExcept(viewMetadataPrivileges())
Done


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2167
PS2, Line 2167: uriPath, symbolName, null, null,
> nit: move L2168 to L2167
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 03 Jul 2018 20:40:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at leat SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

New FE test cases have been added to AuthorizationStmtTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran private parameterized Jenkins job, exhaustive exploration and
covering all tests.

Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 56 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/10850/2
--
To view, visit http://gerrit.cloudera.org:8080/10850
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10850 )

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 1:

(1 comment)

Thanks for the review. Please have a look at PS#2

http://gerrit.cloudera.org:8080/#/c/10850/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10850/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2071
PS1, Line 2071: authorize("create function to_lower(string) returns string 
location " +
> As mentioned in the previous review, this code doesn't create a function in
Thanks for the education! I've made the suggested changes.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 03 Jul 2018 20:02:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10850


Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at leat SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

New FE test cases have been added to AuthorizationStmtTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran private parameterized Jenkins job, exhaustive exploration and
covering all tests.

Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 30 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/10850/1
--
To view, visit http://gerrit.cloudera.org:8080/10850
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has abandoned this change. ( http://gerrit.cloudera.org:8080/10842 
)

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Abandoned

Raising a new review.
--
To view, visit http://gerrit.cloudera.org:8080/10842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I45f5b51363afbe46653b131fad9bf68e3e3ce71f
Gerrit-Change-Number: 10842
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10842 )

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 1:

(3 comments)

Abandoning this patch, and raising a new one instead.

http://gerrit.cloudera.org:8080/#/c/10842/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java:

http://gerrit.cloudera.org:8080/#/c/10842/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2576
PS1, Line 2576:   AuthzOk(ctx, "create function default.to_lower(string) 
returns string " +
> This doesn't actually create a function in the catalog. You need to do the
Thanks for the suggestion. I did not intent for the function to be reused 
across test cases, and hence adding to the catalog is probably not required. 
I've eliminated this step altogether in the new patch.


http://gerrit.cloudera.org:8080/#/c/10842/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2578
PS1, Line 2578: ToUpper
> nit: it's probably better to call the function to_upper since the actual fu
Oops. Thanks for catching that. I meant to use ToLower and c/p'd the wrong line.


http://gerrit.cloudera.org:8080/#/c/10842/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2596
PS1, Line 2596: AuthzError(ctx1, "select default.to_lower('ABCDEF')",
> Due to the way authorization works to prefer AuthorizationException over An
Please see above response. I've botched the rebase on top of 
https://gerrit.cloudera.org/#/c/10841/, so I am going to abandon this change 
and raise a new one instead.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45f5b51363afbe46653b131fad9bf68e3e3ce71f
Gerrit-Change-Number: 10842
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 02 Jul 2018 20:50:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6802 (part 6): Clean up authorization tests

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10841 )

Change subject: IMPALA-6802 (part 6): Clean up authorization tests
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10841/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10841/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2081
PS3, Line 2081:   private static String dropFunctionError(String object) {
Don't we need a similar handler for function use error?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id594ce09a821aef4a1debfdd61569a11defd1c55
Gerrit-Change-Number: 10841
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 28 Jun 2018 21:29:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10842


Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at leat SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

New FE test cases have been added to AuthorizationTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran private parameterized Jenkins job, exhaustive exploration and
covering all tests.

Change-Id: I45f5b51363afbe46653b131fad9bf68e3e3ce71f
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
2 files changed, 44 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/10842/1
--
To view, visit http://gerrit.cloudera.org:8080/10842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I45f5b51363afbe46653b131fad9bf68e3e3ce71f
Gerrit-Change-Number: 10842
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] [DRAFT] IMPALA-6189: Add thread watchdogs for HDFS IO calls

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10696 )

Change subject: [DRAFT] IMPALA-6189: Add thread watchdogs for HDFS IO calls
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10696/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10696/3//COMMIT_MSG@20
PS3, Line 20: --thread_watchdog_threshold_ms=0 (default = 5000ms)
I wonder if the default threshold is too low, in the sense that we may get a 
lot of 'false positives' on 'hangs'. The Linux kernel hung task detector has a 
default timeout of 120 seconds by comparison.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28e918761c120043d332b034450208eaf34e3e2b
Gerrit-Change-Number: 10696
Gerrit-PatchSet: 3
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Balazs Jeszenszky 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 21 Jun 2018 07:00:18 +
Gerrit-HasComments: Yes

[native-toolchain-CR] Bump libunwind version to 1.3-rc1

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10576 )

Change subject: Bump libunwind version to 1.3-rc1
..


Patch Set 2: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10576/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10576/2//COMMIT_MSG@14
PS2, Line 14: The patch disables a test for remote unwindind. I wasn't able to 
figure
Typo: unwindind -> unwinding?



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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb277ba55fb28145367d90dcbd868bf6b3c1710a
Gerrit-Change-Number: 10576
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 01 Jun 2018 18:15:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10364 )

Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
..


Patch Set 1:

Ping?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701
Gerrit-Change-Number: 10364
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 14 May 2018 23:27:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10364 )

Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc@210
PS1, Line 210:   // (e.g. once per row) before the fragment aborts. See 
IMPALA-6997.
I wonder if we should explicitly check that the existing error is 
TErrorCode::MEM_LIMIT_EXCEEDED? I mean, could we come here after hitting some 
other error condition?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701
Gerrit-Change-Number: 10364
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 11 May 2018 16:23:29 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6954: Fix problems with CTAS into Kudu with an expr rewrite

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10251 )

Change subject: IMPALA-6954: Fix problems with CTAS into Kudu with an expr 
rewrite
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I731743bd84cc695119e99342e1b155096147f0ed
Gerrit-Change-Number: 10251
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 30 Apr 2018 23:10:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6623: [DOCS] ltrim and rtrim docs updated

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9984 )

Change subject: IMPALA-6623: [DOCS] ltrim and rtrim docs updated
..


Patch Set 1:

I don't have +2 rights, so someone else must do that.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4f7a04e3c64eade7a23cded21de5ff91c9c8c8c
Gerrit-Change-Number: 9984
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 11 Apr 2018 16:21:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6623: [DOCS] ltrim and rtrim docs updated

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9984 )

Change subject: IMPALA-6623: [DOCS] ltrim and rtrim docs updated
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4f7a04e3c64eade7a23cded21de5ff91c9c8c8c
Gerrit-Change-Number: 9984
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 11 Apr 2018 16:20:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6498: test query profile thrift timestamps causes following tests to fail.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9590 )

Change subject: IMPALA-6498: test_query_profile_thrift_timestamps causes 
following tests to fail.
..


Patch Set 4:

GVO does not seem to have kicked in for this?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
Gerrit-Change-Number: 9590
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 14 Mar 2018 22:28:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6498: test query profile thrift timestamps causes following tests to fail.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9590 )

Change subject: IMPALA-6498: test_query_profile_thrift_timestamps causes 
following tests to fail.
..


Patch Set 3:

(1 comment)

Thanks for the review. Please have a look at PS #4.

http://gerrit.cloudera.org:8080/#/c/9590/3/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/9590/3/tests/query_test/test_observability.py@198
PS3, Line 198: Moving this test to its own suite (IMPALA-6498). This test case 
forces Unregistration
 :   # of the query, so that we force computation of query end 
time, which shows up as
 :   # a non-empty 'End Time' in the profile. We use 
self.client.close() since the
 :   # Beeswax client does not have a cencellation interface. 
self.client cannot be used
 :   # after close(), so if new test cases are added to this suite, 
the test cases must be
 :   # executed before this one.
> Always better to keep comments brief, if possible. I think all of this can
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
Gerrit-Change-Number: 9590
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 13 Mar 2018 21:59:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6498: test query profile thrift timestamps causes following tests to fail.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Tauber-Marshall,

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

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

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

Change subject: IMPALA-6498: test_query_profile_thrift_timestamps causes 
following tests to fail.
..

IMPALA-6498: test_query_profile_thrift_timestamps causes following
tests to fail.

test_query_profile_thrift_timestamps uses ImapaTestSuite.client.close()
to force cancellation/unregistration of the query, so that 'EndTime'
of the query shows up in the profile. Since other test cases also need
a valid ImpalaTestSuite.client, we move the test case in question to
its own test suite.

Have also reduced the query to 'select sleep(5)', as the earlier
'select sleep(1)' is just really excessively long.

Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
---
M tests/query_test/test_observability.py
1 file changed, 56 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9590/4
--
To view, visit http://gerrit.cloudera.org:8080/9590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
Gerrit-Change-Number: 9590
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6498: test query profile thrift timestamps causes following tests to fail.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9590 )

Change subject: IMPALA-6498: test_query_profile_thrift_timestamps causes 
following tests to fail.
..


Patch Set 3:

(1 comment)

Please have a look at ps #3

http://gerrit.cloudera.org:8080/#/c/9590/2/tests/query_test/test_thrift_profile.py
File tests/query_test/test_thrift_profile.py:

http://gerrit.cloudera.org:8080/#/c/9590/2/tests/query_test/test_thrift_profile.py@24
PS2, Line 24:
> It should be sufficient just to put it into its own class still within the
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
Gerrit-Change-Number: 9590
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 12 Mar 2018 22:17:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6498: test query profile thrift timestamps causes following tests to fail.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9590 )

Change subject: IMPALA-6498: test_query_profile_thrift_timestamps causes 
following tests to fail.
..


Patch Set 3:

> (1 comment)

Thanks for the suggestion. I've moved it back to the old file, but a separate 
class.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
Gerrit-Change-Number: 9590
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 12 Mar 2018 22:17:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6498: test query profile thrift timestamps causes following tests to fail.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Tauber-Marshall,

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

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

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

Change subject: IMPALA-6498: test_query_profile_thrift_timestamps causes 
following tests to fail.
..

IMPALA-6498: test_query_profile_thrift_timestamps causes following
tests to fail.

test_query_profile_thrift_timestamps uses ImapaTestSuite.client.close()
to force cancellation/unregistration of the query, so that 'EndTime'
of the query shows up in the profile. Since other test cases also need
a valid ImpalaTestSuite.client, we move the test case in question to
its own test suite.

Have also reduced the query to 'select sleep(5)', as the earlier
'select sleep(1)' is just really excessively long.

Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
---
M tests/query_test/test_observability.py
1 file changed, 60 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9590/3
--
To view, visit http://gerrit.cloudera.org:8080/9590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
Gerrit-Change-Number: 9590
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-6498: test query profile thrift timestamps causes following tests to fail.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Thomas Tauber-Marshall,

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

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

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

Change subject: IMPALA-6498: test_query_profile_thrift_timestamps causes 
following tests to fail.
..

IMPALA-6498: test_query_profile_thrift_timestamps causes following
tests to fail.

test_query_profile_thrift_timestamps uses ImapaTestSuite.client.close()
to force cancellation/unregistration of the query, so that 'EndTime'
of the query shows up in the profile. Since other test cases also need
a valid ImpalaTestSuite.client, we move the test case in question to
its own test suite.

Have also reduced the query to 'select sleep(5)', as the earlier
'select sleep(1)' is just really excessively long.

Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
---
M tests/query_test/test_observability.py
A tests/query_test/test_thrift_profile.py
2 files changed, 82 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9590/2
--
To view, visit http://gerrit.cloudera.org:8080/9590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
Gerrit-Change-Number: 9590
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-6498: test query profile thrift timestamps causes following tests to fail.

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9590


Change subject: IMPALA-6498: test_query_profile_thrift_timestamps causes 
following tests to fail.
..

IMPALA-6498: test_query_profile_thrift_timestamps causes following
tests to fail.

test_query_profile_thrift_timestamps uses ImapaTestSuite.client.close()
to force cancellation/unregistration of the query, so that 'EndTime'
of the query shows up in the profile. Since other test cases also need
a valid ImpalaTestSuite.client, we move the test case in question to
its own test suite.

Have also reduced the query to 'select sleep(5)', as the earlier
'select sleep(1)' is just really excessively long.

Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
---
A tests/query_test/test_thrift_profile.py
1 file changed, 82 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/9590/1
--
To view, visit http://gerrit.cloudera.org:8080/9590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I287a05f3c90b1a71a5b7ee0f5c06a8840ebac4c9
Gerrit-Change-Number: 9590
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9038 )

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..


Patch Set 5:

Thanks Sailesh. I do not have the permission to carry your +2, so could you 
please fo that again?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 30 Jan 2018 18:08:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9038 )

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py
File tests/custom_cluster/test_custom_statestore.py:

http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py@34
PS3, Line 34: from thrift.transport import TSocket, TTransport
:
> one line
Done


http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py@39
PS3, Line 39:
: LOG = logging.getLogger('custom_statestore_test')
> Needed?
Removed.


http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py@42
PS3, Line 42:
> Needed?
Removed


http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py@52
PS3, Line 52:   _, port = handle.getsockname()
:
:   @classmethod
:   def get_workload(self):
> Does this mean that every "statestore subscriber" uses the same port? Looks
That's correct. All the subscribers we create below will connect from the same 
port number, but with different subscriber IDs. Is that ok with you? It seems 
simpler and puts less load on the system than creating a new port every time.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 29 Jan 2018 20:39:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho, Sailesh Mukil,

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

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

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..

IMPALA-2642: Fix a potential deadlock in statestore

The statestored can deadlock if the number of subscribers has
reached STATESTORE_MAX_SUBSCRIBERS, because the DoSubscriberUpdate()
method calls OfferUpdate(), while holding subscribers_lock_, which
also tries to take the same lock in this situation.

Fix the issue by moving out the call to acquire subscribers_lock_ from
OfferUpdate(), and depend on the callers to take it. We also make
the maximum number of statestore subscribers a start-up time tuneable,
to allow us to test the limit more easily.

Testing: The problem is easily reproduced by lowering the value of
STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
with 3 impalads. Without the fix, the statestored becomes completely
deadlocked.

A new EE test has been added to exercise this scenario. The test
verifies that statestored correctly rejects new subscription
requests when the limit it reached.

Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
A tests/custom_cluster/test_custom_statestore.py
3 files changed, 107 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/9038/4
--
To view, visit http://gerrit.cloudera.org:8080/9038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho, Sailesh Mukil,

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

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

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..

IMPALA-2642: Fix a potential deadlock in statestore

The statestored can deadlock if the number of subscribers has
reached STATESTORE_MAX_SUBSCRIBERS, because the DoSubscriberUpdate()
method calls OfferUpdate(), while holding subscribers_lock_, which
also tries to take the same lock in this situation.

Fix the issue by moving out the call to acquire subscribers_lock_ from
OfferUpdate(), and depend on the callers to take it. We also make
the maximum number of statestore subscribers a start-up time tuneable,
to allow us to test the limit more easily.

Testing: The problem is easily reproduced by lowering the value of
STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
with 3 impalads. Without the fix, the statestored becomes completely
deadlocked.

A new EE test has been added to exercise this scenario. The test
verifies that statestored correctly rejects new subscription
requests when the limit it reached.

Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
A tests/custom_cluster/test_custom_statestore.py
3 files changed, 111 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/9038/3
--
To view, visit http://gerrit.cloudera.org:8080/9038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6399: Fix timeout logic in test query profile thrift timestamps

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9079 )

Change subject: IMPALA-6399: Fix timeout logic in 
test_query_profile_thrift_timestamps
..


Patch Set 8: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f0f3db75c52b8b2081dd15ed7e7d2a1d6b22389
Gerrit-Change-Number: 9079
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 23 Jan 2018 16:27:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8349 )

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 11:

Let's wait for https://gerrit.cloudera.org/#/c/9079/ to be merged before 
rebasing, so that we're not again held up by a flaky test failing.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 11
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 22 Jan 2018 21:36:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6399: Fix timeout logic in test query profile thrift timestamps

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9079 )

Change subject: IMPALA-6399: Fix timeout logic in 
test_query_profile_thrift_timestamps
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f0f3db75c52b8b2081dd15ed7e7d2a1d6b22389
Gerrit-Change-Number: 9079
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 19 Jan 2018 20:24:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6399: Fix timeout logic in test query profile thrift timestamps

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9079 )

Change subject: IMPALA-6399: Fix timeout logic in 
test_query_profile_thrift_timestamps
..


Patch Set 1:

(1 comment)

Thanks for fixing this.

http://gerrit.cloudera.org:8080/#/c/9079/1/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/9079/1/tests/query_test/test_observability.py@164
PS1, Line 164: elapsed = time.time() - (end - MAX_WAIT)
Can you add start = time.time() at L146, and use that to calculate the elapsed 
time instead? It would be more obvious that way.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f0f3db75c52b8b2081dd15ed7e7d2a1d6b22389
Gerrit-Change-Number: 9079
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 19 Jan 2018 19:50:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Alex Behm,

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

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

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..

IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of
characters.

This patch generalizes ltrim()/rtrim() functions to accept a second
argument that specifies the set of characters to be removed from the
leading/trailing end of the target string:

ltrim(string text[, characters text])
rtrim(string text[, characters text])

A common string trimming method has been added to StringFunctions,
which is called from the general ltrim/rtrim/btrim functions. The
functions also share prepare and close operations.

New StringFunctions tests have been added to ExprTest for the new
forms of ltrim() and rtrim(). New tests to cover handling of special
characters have also been added.

Note that our string handling functions only work with the ASCII
character set. Handling of other character sets lies outside the
scope of this patch.

The existing ltrim()/rtrim()/trim() functions that take only one
argument have been updated to use the more general methods.

Testing: Queries like the following were run on a 1.5-billion row
tpch_parquet.lineitem table, with the old and new implementations
to ensure there is no performance regression:

  1. select count(trim(l_shipinstruct)), count(trim(l_returnflag)), ...
  2. select count(*) from t where trim(l_shipinstruct) = '' and ...

Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 178 insertions(+), 88 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/8349/10
--
To view, visit http://gerrit.cloudera.org:8080/8349
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 10
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8349 )

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 9:

(2 comments)

Thanks. Please see patch set 10.

http://gerrit.cloudera.org:8080/#/c/8349/9/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8349/9/be/src/exprs/string-functions-ir.cc@412
PS9, Line 412: context->GetNumArgs() < 2
> nit: it seems slightly simpler to say context->GetNumArgs() == 1 given the
Done


http://gerrit.cloudera.org:8080/#/c/8349/9/be/src/exprs/string-functions-ir.cc@434
PS9, Line 434: IS_CONSTANT
> As discussed, this may not be entirely correct to call this "IS_CONSTANT" a
Renamed it as IS_IMPLICIT_WHITESPACE. Hopefully this one sticks.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 9
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 18 Jan 2018 19:31:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9038 )

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..


Patch Set 2:

(3 comments)

Working on the test case.

http://gerrit.cloudera.org:8080/#/c/9038/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9038/2//COMMIT_MSG@17
PS2, Line 17: Testing: The problem is easily reproduced by lowering the value of
: STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
: with 3 impalads. With the fix, we can see the following entry in 
the
: statestore log:
> Is it possible to add a test for this?
Doing it. The approach I am taking requires changing STATESTORE_MAX_SUBSCRIBERS 
to FLAGS_statestore_max_subscribers, so that we can start statestored with a 
low max value. I think this is good to do anyway, as that allows us to test 
other boundary behavior.


http://gerrit.cloudera.org:8080/#/c/9038/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

http://gerrit.cloudera.org:8080/#/c/9038/2/be/src/statestore/statestore.cc@353
PS2, Line 353: // Assumes that the caller has taken subscribers_lock_
> I would add this comment in the header comment in the .h file.
Done


http://gerrit.cloudera.org:8080/#/c/9038/2/be/src/statestore/statestore.cc@353
PS2, Line 353: // Assumes that the caller has taken subscribers_lock_
> Its kind of weird IMO that OfferUpdate() expects the caller to take subscri
Thought of that, ie, moving the call to OfferUpdate from DoSubscriberUpdate to 
another scope that does not have subscribers_lock_. Then I saw that 
UnregisterSubscriber() also expects the caller to take subscribers_lock_. I 
thought it made sense to follow the same pattern here.

I think there is scope for cleaning up the way locking in the statestore code. 
For instance, RegisterSubscriber() takes subscribers_lock_, instead of its 
caller. I am just not sure if it makes sense to do that as part of this jira.

Another thing: BlockingQueue::BlockingPut() does not release the mutex it 
acquires in the shut down path. I know we're shutting down anyway, but if ever 
we implement clean shut down that does some work, a deadlock might happen here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 17 Jan 2018 19:22:30 +
Gerrit-HasComments: Yes