Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8202 )
Change subject: IMPALA-4704: Disallow client connections to imapalad until catalog is received. ...................................................................... Patch Set 7: (15 comments) http://gerrit.cloudera.org:8080/#/c/8202/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8202/7//COMMIT_MSG@7 PS7, Line 7: imapalad nit: typo http://gerrit.cloudera.org:8080/#/c/8202/7//COMMIT_MSG@7 PS7, Line 7: catalog catalog update http://gerrit.cloudera.org:8080/#/c/8202/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8202/7/be/src/service/impala-server.cc@a2045 PS7, Line 2045: :) nice http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@81 PS7, Line 81: action="store_true", help="Starts all cluster processes except catalogd.") nit: long line http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@254 PS7, Line 254: catalog_needs_wait I think that function tests if the catalog is ready given that we haven't disabled it. So maybe "is_catalog_ready" is a better name. Thoughts? http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@323 PS7, Line 323: def wait_for_client(impalad, timeout_in_seconds=CLUSTER_WAIT_TIMEOUT_IN_SECONDS): Add a function comment. http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@332 PS7, Line 332: ready = True You can just break and use client_hs2 or client_beeswax in the check in L339. http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@336 PS7, Line 336: client_beeswax hs2_client doesn't have a close() function? http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@859 PS7, Line 859: ready Maybe also mention what "ready" means, i.e. receive the first update from the catalog. http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@867 PS7, Line 867: if (getCatalog().isReady()) return; It may make sense to add a log message here to indicate that the catalog is now ready after waiting for MAX_CATALOG_UPDATE_WAIT_TIME_MS * num_triews msec. http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@868 PS7, Line 868: catalog to be ready That phrase kind of suggests that the catalog server is not ready. Maybe best to say that we're waiting for the initial catalog update from the statestore or for the local catalog cache to be initialized. http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@899 PS7, Line 899: support when the catalog is not ready. Maybe "is not supported if the local catalog cache is not initialized." http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java@587 PS7, Line 587: public void waitForCatalog() { : frontend_.waitForCatalog(); : } single line http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/test/java/org/apache/impala/service/FrontendTest.java File fe/src/test/java/org/apache/impala/service/FrontendTest.java: http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/test/java/org/apache/impala/service/FrontendTest.java@117 PS7, Line 117: private void testCatalogIsNotReady(String stmt, Frontend fe) { : TQueryCtx queryCtx = TestUtils.createQueryContext( : Catalog.DEFAULT_DB, AuthorizationTest.USER.getName()); : queryCtx.client_request.setStmt(stmt); : try { : fe.createExecRequest(queryCtx, new StringBuilder()); : fail("Expected failure to due uninitialized catalog."); : } catch (IllegalStateException e) { : assertEquals("Analyzing a query is not support when the catalog is not ready.", : e.getMessage()); : } catch (Exception e) { : fail("Failed to create exec request due to: " + ExceptionUtils.getStackTrace(e)); : } : } Does this even make sense to keep given the introduced behavior in this patch? http://gerrit.cloudera.org:8080/#/c/8202/7/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/8202/7/tests/common/custom_cluster_test_suite.py@138 PS7, Line 138: # The number of statestore subscribers is cluster_size (# of impalad) + 1 (for catalogd). nit: long line -- 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: 7 Gerrit-Owner: Vuk Ercegovac <vercego...@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: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Fri, 13 Oct 2017 19:31:35 +0000 Gerrit-HasComments: Yes
