Vuk Ercegovac 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: (13 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 reworded. 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 Done 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 d Done 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. Done 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 L33 Done 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? nope. perhaps there is an api I've overlooked. fwict, you'd have to provide a close request for a given session handle (see hs2_test_suite). at this point, a port has been opened, but I don't see a close for that one... perhaps the idea is that you need to keep track of both the port and the client. 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 t Done 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 keeping track of total time- so that's what's printed here. 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 be went with 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." Done 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 Done 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 pat Agreed, the precondition should be enough to show what is expected. 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 Done -- 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 22:19:47 +0000 Gerrit-HasComments: Yes
