Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8202 )
Change subject: IMPALA-4704: Turns on client connections when local catalog initialized. ...................................................................... Patch Set 20: (2 comments) http://gerrit.cloudera.org:8080/#/c/8202/19/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8202/19/be/src/service/impala-server.h@89 PS19, Line 89: must call the following : /// methods: > good question-- I've kept it the same as before so not sure what the reason That DCHECK: DCHECK(exec_env.process_mem_tracker() != nullptr) << "ExecEnv::StartServices() must be called before starting RPC services"; looks stale since the process_mem_tracker() is set in exec_env_.Init() which is called earlier in main (not to mention StartServices() doesn't exist anymore). I also don't see what the specific dependency is that this is trying to point out (this DCHECK should have been closer to the code that requires this to be true). If you want to keep the dcheck, I see no reason it couldn't be moved before both Init and Start. Then we could combine them, since there doesn't appear to be any logic behind the separation of those two. But you can defer cleaning this up if you prefer (though the dcheck wording should be updated at least). http://gerrit.cloudera.org:8080/#/c/8202/20/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8202/20/be/src/service/impala-server.cc@1936 PS20, Line 1936: I think a quick comment is worth it to highlight the dependency between these two lines: // Subscribe to the catalog topic and then wait for the initial catalog update. (or whatever is accurate) -- To view, visit http://gerrit.cloudera.org:8080/8202 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 Gerrit-Change-Number: 8202 Gerrit-PatchSet: 20 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Balazs Jeszenszky <jes...@gmail.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Juan Yu <j...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Nov 2017 18:04:28 +0000 Gerrit-HasComments: Yes
