[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 4:

(1 comment)

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

PS4, Line 1963: exec_env->metrics(), FLAGS_fe_service_threads, 
ThriftServer::ThreadPool);
> Sorry, should that be "let's remove it from network-perf-benchmark"
I mean let's move network-perf-benchmark to a threaded server as well, with 0 
as the number of max tasks. Yes, let's remove the server type from ThriftServer.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 4:

(1 comment)

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

PS4, Line 1963: exec_env->metrics(), FLAGS_fe_service_threads, 
ThriftServer::ThreadPool);
> Yes, let's remove it for network-perf-benchmark.cc.
Sorry, should that be "let's remove it from network-perf-benchmark"
or should it be "let's leave it for network-perf-benchmark"?

If I remove it from network-perf-benchmark, should I remove support of the 
ThreadPool type from ThriftServer.cc?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 4:

(2 comments)

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

PS4, Line 1963: exec_env->metrics(), FLAGS_fe_service_threads, 
ThriftServer::ThreadPool);
> Running this change through run-all-tests and changing the comment at 1954 
Yes, let's remove it for network-perf-benchmark.cc.


PS4, Line 1985: Threaded
> Does it matter that FLAG_enable_accept_queue_server can be turned to false 
I think the accept queue is stable enough at this point - it's been running in 
production for us for quite a long time. We can probably deprecate the flag 
(and hide it using DEFINE_bool_hidden).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 4:

(2 comments)

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

PS4, Line 1963: exec_env->metrics(), FLAGS_fe_service_threads, 
ThriftServer::ThreadPool);
> Let's make this change for beeswax as well.
Running this change through run-all-tests and changing the comment at 1954 to 
be:
"ODBC and Hue drivers do not support non-blocking servers." similar to the 
comment at line 1976. I'm guessing at the intent of the old comment.

After this change the only thing using ThreadPool server implementation is 
network-perf-benchmark.cc

Not sure what the network-perf-benchmark.cc is used for, but if nothing else is 
using ThreadedPool implementation should I just remove support for it from 
ThriftServer() to prevent bit rot in the unused code paths? Or just leave it be 
for future use (or just for the network-perf-benchmark to use?)


PS4, Line 1985: Threaded
> Maybe we can just remove Threaded and ThreadPool and just pass in the numbe
Does it matter that FLAG_enable_accept_queue_server can be turned to false and 
that these thread limits will not be enforced? Or at this point is there enough 
confidence that the accept_queue_server is good/stable enough and won't be 
disabled? If that is the case, should the flag be deprecated and the special 
case code be removed for disabling it?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has uploaded a new patch set (#4).

Change subject: IMPALA-5394: Handle blocked HS2 connections
..

IMPALA-5394: Handle blocked HS2 connections

- TThreadPoolServer calls getTransport() on a client from the Server
  thread (the thread that does the accepts).
  - TSaslServerTransport->getTransport() calls TSaslTransport->open()
  - TSaslServerTransport->open() tries to negotiate SASL which calls
read/write
- If read/write blocks, the TThreadPoolServer cannot accept
  connections
- Set the underlying TSocket's recvTimeout and sendTimeout before the
  TSaslServerTransport->open() and reset them to 0 after open()
  completes.
- Added sasl_connect_tcp_timeout_ms flag that defaults to 30
  milliseconds (5 minutes)
- Changed the Thrift server type for hs2 connections from ThreadPool
  to Threaded to take advantage of the AcceptQueueServer
  implementation.
- Add the ability for the TAcceptQueueServer to limit the maximum
  number of tasks at a time
- Removed the previously unenforced thread limit requests from
  StatestoreService, StatestoreSubscriber, CatalogService.

Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
---
M be/src/catalog/catalogd-main.cc
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 be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/transport/TSaslServerTransport.cpp
M common/thrift/metrics.json
10 files changed, 77 insertions(+), 36 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

Thanks, yeah I overthought it. I'll run this through run-all-tests and post the 
patch.

 > My instinct would be just to use the size of tasks_ to control the
 > coordination.
 > 
 > In SetupConnection(), line 168 or so:
 > 
 > {
 > Synchronized s(tasksMonitor_);
 > if (tasks_.size() > max) tasksMonitor_.wait();
 > tasks_.insert(task);
 > }

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

My instinct would be just to use the size of tasks_ to control the coordination.

In SetupConnection(), line 168 or so:

  { 
Synchronized s(tasksMonitor_);
if (tasks_.size() > max) tasksMonitor_.wait();
tasks_.insert(task);
  }

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

I'd like some advice/feedback on the approach for enforcing fe_service_threads. 
In my local diff, I've added a maxTasks, numTasks and a taskLimitMonitor to the 
TAcceptQueueServer class.
>From there it seems like I have two choices:
1)
a) inside the Task constructor, synchronize with the taskLimitMonitor, check 
the passed in server's numTasks against maxTasks and wait if we've reached the 
limit or increment the server's numTasks.
b) inside the Task destructor synchronize with the taskLimitMonitor, decrement 
the server's numTasks and notifyAll if I'm transistioning from maxTasks to < 
maxTasks.
-- I like this because the code is somewhat symmetrical, but I do not know if 
it is a good idea to do these blocking operations inside a 
constructor/destructor.

2)
a) inside the SetupConnection function before I create a new task, synchronize 
with the tasklimitMonitor and check/block/increment numTasks.
b) inside the Tasks run method, decrement/notify where the Task removes itself 
from the TAcceptQueueServer's tasks set.

My local diff uses approach 1 currently but I'm a bit unsure about the best 
practices around that approach.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

It looks like it'll be next week (or this weekend) before I'm able to make some 
time to write/test the code to honor the fe_service_threads flag.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

Yeah, I'll give it a go.

> Do you think that's something you could add to this patch?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

(1 comment)

Patch looks pretty good. It would be great to still respect fe_service_threads, 
if possible, as I know of some users who set that flag to control the 
concurrency on a particular Impala instance. It shouldn't be too hard to have 
TAcceptQueueServer::Task limit itself to have only fe_service_threads active at 
one time with a condition variable and a shared atomic integer. Do you think 
that's something you could add to this patch?

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

Line 210
> The prior comment mentioned potential thread safety issues, but I didn't se
IIRC, the issue was that we didn't know for sure whether the various transport 
and protocol factories (particularly with SSL enabled) were thread-safe. Might 
be worth keeping this at 1 until we know for sure one way or the other.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

Hi John - Thanks for taking the time to make this change! I'll probably do the 
first review round here, but just got back from vacation, so bear with me while 
I catch up!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

Giving this a bump, since my updated patch might have slipped under the radar 
during the holiday.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

(1 comment)

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

Line 210
The prior comment mentioned potential thread safety issues, but I didn't see 
any thread safety issues in the current usage. If someone has experience in 
this area, it's probably worth it to make sure I didn't miss something.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections

[John Sherman (Code Review)]

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
..


Patch Set 3:

Ran the latest patch through be/fe/e2e testing.
be/fe ran clean.
end to end mostly ran clean. I had one failure I couldn't explain (could be 
related to me using Java 8/Ubuntu 16 or the limited amount of memory in my 
development environment):
custom_cluster/test_hdfs_fd_caching.py::TestHdfsFdCaching::test_caching_enabled
custom_cluster/test_hdfs_fd_caching.py:125: in test_caching_enabled
self.run_fd_caching_test(vector, caching_expected, cache_capacity)
custom_cluster/test_hdfs_fd_caching.py:85: in run_fd_caching_test
assert num_handles_after == (num_handles_start + 1)
E   assert 0 == (0 + 1)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No