[Impala-ASF-CR] IMPALA-5394: Handle blocked HS2 connections
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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-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 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 ShermanGerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: John Sherman Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No