[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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:
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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: A Frontend test was asserting an error message removed by this change. Latest patch fixes that test. All tests pass with this latest patch. -- 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 Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 11 Oct 2017 16:33:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Hello Philip Zeyliger, Balazs Jeszenszky, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8202 to look at the new patch set (#7). Change subject: IMPALA-4704: Disallow client connections to imapalad until catalog is received. .. IMPALA-4704: Disallow client connections to imapalad until catalog is received. Currently, impalad starts beeswax and hs2 servers even if the catalog has not yet been received. As a result, client connections see an error message stating that the impalad is not yet ready. This patch changes the impalad startup sequence to wait until the catalog is received before opening beeswax and hs2 ports and starting their servers. Testing: - python e2e tests that start a cluster without a catalog and check that client connections are rejected as expected. Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 --- M be/src/service/frontend.cc M be/src/service/frontend.h M be/src/service/impala-server.cc M be/src/testutil/in-process-servers.cc M bin/start-impala-cluster.py M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/test/java/org/apache/impala/service/FrontendTest.java M tests/common/custom_cluster_test_suite.py M tests/common/impala_cluster.py M tests/common/impala_service.py A tests/custom_cluster/test_catalog_wait.py M tests/custom_cluster/test_coordinators.py 13 files changed, 157 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/8202/7 -- 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: newpatchset Gerrit-Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 Gerrit-Change-Number: 8202 Gerrit-PatchSet: 7 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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 5: latest patch fixes two issues with tests: 1) delaying ports to be open after the catalog is ready means that testing infrastructure needs to check that the ports are open, not just that the catalog is ready. 2) avoid killing cluster processes in this test since it interacts negatively with subsequent tests that stop/start the cluster. -- 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: 5 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 10 Oct 2017 01:20:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Hello Philip Zeyliger, Balazs Jeszenszky, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8202 to look at the new patch set (#5). Change subject: IMPALA-4704: Disallow client connections to imapalad until catalog is received. .. IMPALA-4704: Disallow client connections to imapalad until catalog is received. Currently, impalad starts beeswax and hs2 servers even if the catalog has not yet been received. As a result, client connections see an error message stating that the impalad is not yet ready. This patch changes the impalad startup sequence to wait until the catalog is received before opening beeswax and hs2 ports and starting their servers. Testing: - python e2e tests that start a cluster without a catalog and check that client connections are rejected as expected. Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 --- M be/src/service/frontend.cc M be/src/service/frontend.h M be/src/service/impala-server.cc M be/src/testutil/in-process-servers.cc M bin/start-impala-cluster.py M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M tests/common/custom_cluster_test_suite.py M tests/common/impala_cluster.py M tests/common/impala_service.py A tests/custom_cluster/test_catalog_wait.py M tests/custom_cluster/test_coordinators.py 12 files changed, 153 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/8202/5 -- 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: newpatchset Gerrit-Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 Gerrit-Change-Number: 8202 Gerrit-PatchSet: 5 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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 4: Update on this one. Several e2e tests failed so I'm fixing them. Main issue that I've found so far is how various tests check that a cluster is ready. -- 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: 4 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 06 Oct 2017 19:14:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Balazs Jeszenszky 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 4: Code-Review+1 This approach looks good from user POV, thanks! -- 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: 4 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 06 Oct 2017 00:18:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/8202/3/be/src/testutil/in-process-servers.cc File be/src/testutil/in-process-servers.cc: http://gerrit.cloudera.org:8080/#/c/8202/3/be/src/testutil/in-process-servers.cc@75 PS3, Line 75: if (started.ok()) return impala; > You're no longer logging status.getDetail() any more. I suspect that's inte prev. code only logged for the set-catalog case, which was odd. I've added logging now for all branches that return a not-ok status. http://gerrit.cloudera.org:8080/#/c/8202/3/tests/custom_cluster/test_catalog_wait.py File tests/custom_cluster/test_catalog_wait.py: http://gerrit.cloudera.org:8080/#/c/8202/3/tests/custom_cluster/test_catalog_wait.py@26 PS3, Line 26: > nit: whitespace? 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: 4 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Oct 2017 17:52:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Hello Philip Zeyliger, Balazs Jeszenszky, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8202 to look at the new patch set (#4). Change subject: IMPALA-4704: Disallow client connections to imapalad until catalog is received. .. IMPALA-4704: Disallow client connections to imapalad until catalog is received. Currently, impalad starts beeswax and hs2 servers even if the catalog has not yet been received. As a result, client connections see an error message stating that the impalad is not yet ready. This patch changes the impalad startup sequence to wait until the catalog is received before opening beeswax and hs2 ports and starting their servers. Testing: - python e2e tests that start a cluster without a catalog and check that client connections are rejected as expected. Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 --- M be/src/service/frontend.cc M be/src/service/frontend.h M be/src/service/impala-server.cc M be/src/testutil/in-process-servers.cc M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M tests/common/impala_cluster.py A tests/custom_cluster/test_catalog_wait.py 8 files changed, 115 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/8202/4 -- 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: newpatchset Gerrit-Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 Gerrit-Change-Number: 8202 Gerrit-PatchSet: 4 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Philip Zeyliger 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 3: (2 comments) Thanks for the updates! http://gerrit.cloudera.org:8080/#/c/8202/3/be/src/testutil/in-process-servers.cc File be/src/testutil/in-process-servers.cc: http://gerrit.cloudera.org:8080/#/c/8202/3/be/src/testutil/in-process-servers.cc@75 PS3, Line 75: if (started.ok()) return impala; You're no longer logging status.getDetail() any more. I suspect that's intentional because it was logged when it was create? http://gerrit.cloudera.org:8080/#/c/8202/3/tests/custom_cluster/test_catalog_wait.py File tests/custom_cluster/test_catalog_wait.py: http://gerrit.cloudera.org:8080/#/c/8202/3/tests/custom_cluster/test_catalog_wait.py@26 PS3, Line 26: nit: whitespace? -- 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: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Oct 2017 17:24:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Hello Philip Zeyliger, Balazs Jeszenszky, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8202 to look at the new patch set (#3). Change subject: IMPALA-4704: Disallow client connections to imapalad until catalog is received. .. IMPALA-4704: Disallow client connections to imapalad until catalog is received. Currently, impalad starts beeswax and hs2 servers even if the catalog has not yet been received. As a result, client connections see an error message stating that the impalad is not yet ready. This patch changes the impalad startup sequence to wait until the catalog is received before opening beeswax and hs2 ports and starting their servers. Testing: - python e2e tests that start a cluster without a catalog and check that client connections are rejected as expected. Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 --- M be/src/service/frontend.cc M be/src/service/frontend.h M be/src/service/impala-server.cc M be/src/testutil/in-process-servers.cc M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M tests/common/impala_cluster.py A tests/custom_cluster/test_catalog_wait.py 8 files changed, 114 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/8202/3 -- 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: newpatchset Gerrit-Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 Gerrit-Change-Number: 8202 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/8202/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/8202/2/be/src/runtime/exec-env.cc@356 PS2, Line 356: if (is_started_) return Status::OK(); > Is this really an assertion that we should only call StartServices() once? removed this... see the comment in impala-server. http://gerrit.cloudera.org:8080/#/c/8202/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8202/2/be/src/service/impala-server.cc@2044 PS2, Line 2044: RETURN_IF_ERROR(exec_env_->StartServices()); > It strikes me as weird that there are now two possible places where we can Reworked this. I want to keep the simplicity of recent init/start cleanup. Let me know if its clearer. http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py File tests/custom_cluster/test_catalog_wait.py: http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@22 PS2, Line 22: NUM_SUBSCRIBERS, > unused Done http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@23 PS2, Line 23: CLUSTER_SIZE, > unused Done http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@27 PS2, Line 27: # Impalad must wait for the catalog prior to openning up client ports. > python nit: Done http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@32 PS2, Line 32: def test_client_connection(self, vector): > Do we end up running this in multiple "vectors"? I think having this test i Done. not accustomed to var args everything. http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@48 PS2, Line 48: # The ports may not be ready if accessed too quickly, so wait. > nit: They'll never be ready, which is sort of the point. re-worded to better reflect what I'm trying to catch with this sleep. -- 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: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Oct 2017 06:03:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Philip Zeyliger 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 2: (7 comments) Thanks for the updates. Some comments/questions about the initialization code. I'm very much not an authority on it, I'm but interested in learning. http://gerrit.cloudera.org:8080/#/c/8202/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/8202/2/be/src/runtime/exec-env.cc@356 PS2, Line 356: if (is_started_) return Status::OK(); Is this really an assertion that we should only call StartServices() once? http://gerrit.cloudera.org:8080/#/c/8202/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8202/2/be/src/service/impala-server.cc@2044 PS2, Line 2044: RETURN_IF_ERROR(exec_env_->StartServices()); It strikes me as weird that there are now two possible places where we can call StartServices(): in both Init() and in Start(). Is the one here now really a DCHECK that exec_env_->is_started_ is true? http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py File tests/custom_cluster/test_catalog_wait.py: http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@22 PS2, Line 22: NUM_SUBSCRIBERS, unused http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@23 PS2, Line 23: CLUSTER_SIZE, unused http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@27 PS2, Line 27: # Impalad must wait for the catalog prior to openning up client ports. python nit: More common is: class Foo(...): """documentation""" This makes Foo.__doc__ populated. We do this at least in the first case I looked at: @SkipIfBuildType.not_dev_build class TestExchangeDelays(CustomClusterTestSuite): """Tests for handling delays in finding data stream receivers""" http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@32 PS2, Line 32: def test_client_connection(self, vector): Do we end up running this in multiple "vectors"? I think having this test is good, but I think it's reasonable to believe that this test isn't affected by cluster size, or security, or compression type... http://gerrit.cloudera.org:8080/#/c/8202/2/tests/custom_cluster/test_catalog_wait.py@48 PS2, Line 48: # The ports may not be ready if accessed too quickly, so wait. nit: They'll never be ready, which is sort of the point. -- 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: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Oct 2017 02:37:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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 2: (2 comments) Updated the description as well; waits until catalog is ready now. http://gerrit.cloudera.org:8080/#/c/8202/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8202/1/be/src/service/impala-server.cc@2043 PS1, Line 2043: ImpalaServer::Start > the relationship between this code and the HS2/Beeswax servers would be cle moved this into init since my understanding now is that the ports should be opened only when the catalog is ready. http://gerrit.cloudera.org:8080/#/c/8202/1/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/1/fe/src/main/java/org/apache/impala/service/Frontend.java@905 PS1, Line 905: // 1) Analysis completes successfully. > perhaps this should be a Preconditions check, if our intention is to preven Done. Agreed that when we get here, catalog should always be in ready state. -- 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: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Oct 2017 01:23:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Hello Philip Zeyliger, Balazs Jeszenszky, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8202 to look at the new patch set (#2). Change subject: IMPALA-4704: Disallow client connections to imapalad until catalog is received. .. IMPALA-4704: Disallow client connections to imapalad until catalog is received. Currently, impalad starts beeswax and hs2 servers even if the catalog has not yet been received. As a result, client connections see an error message stating that the impalad is not yet ready. This patch changes the impalad startup sequence to wait until the catalog is received before opening beeswax and hs2 ports and starting their servers. Testing: - python e2e tests that start a cluster without a catalog and check that client connections are rejected as expected. Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 --- M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/service/frontend.cc M be/src/service/frontend.h M be/src/service/impala-server.cc M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M tests/common/impala_cluster.py A tests/custom_cluster/test_catalog_wait.py 9 files changed, 120 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/8202/2 -- 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: newpatchset Gerrit-Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 Gerrit-Change-Number: 8202 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Dan Hecht 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 1:
(2 comments)
It does seem like picking a reasonable timeout here is not possible.
Additionally, given that the current behavior is that all queries will fail
before isReady(), waiting indefinitely for isReady() seems okay.
http://gerrit.cloudera.org:8080/#/c/8202/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8202/1/be/src/service/impala-server.cc@2043
PS1, Line 2043: FLAGS_is_coordinator
the relationship between this code and the HS2/Beeswax servers would be clearer
if this were:
if (hs2_server_.get() != NULL || beeswax_server_.get() != NULL)
or if we just call WaitForCatalog() on both branches.
http://gerrit.cloudera.org:8080/#/c/8202/1/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/1/fe/src/main/java/org/apache/impala/service/Frontend.java@905
PS1, Line 905: if (!impaladCatalog_.get().isReady()) {
perhaps this should be a Preconditions check, if our intention is to prevent
this from being callable before isReady() returns true.
--
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: 1
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Balazs Jeszenszky
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 04 Oct 2017 17:39:33 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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 1: What process is currently monitoring for impalad health and under what conditions is it restarted? Depending on that watchdog process, a quicker death may be easier to deal with since it would be a clear signal to restart the process. -- 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: 1 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 04 Oct 2017 17:22:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Balazs Jeszenszky 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 1: I think the bug report's title is a better than it's description. In particular, I'm not sure shutting down the daemon is the best way out of situations like this since in most managed environments they will be restarted automatically (causing another attempt to get initial catalog update, same as if they were left alive). If for some reason opening the ports later is not possible, I think timing out should result in closing the connection with a more descriptive error message so that clients can retry. However, the best solution would be to only open the frontend ports after the catalog update is processed. With a load balancer in front of several coordinators, this setup would allow for a seamless (i.e. not even a delay due to connections waiting for the catalog update to be processed) experience for clients. -- 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: 1 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 04 Oct 2017 17:04:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
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 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8202/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8202/1//COMMIT_MSG@17 PS1, Line 17: this wait time bound, the impalad exits. > Because it's hard to time the start-up of processes on multiple machines, s the bug description recommends that the impalad die after a "reasonable" amount of time/retries. I can adjust the setting that's currently in this patch-- I figured that would be a point of discussion. -- 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: 1 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 04 Oct 2017 16:39:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Philip Zeyliger 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 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8202/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8202/1//COMMIT_MSG@17 PS1, Line 17: this wait time bound, the impalad exits. Because it's hard to time the start-up of processes on multiple machines, short timeouts like this one (it's 10 seconds, if I'm reading correctly) can cause issues. Most Hadoop-related distributed systems (but not all) wait forever or ~hours before they decide to give up. I'm also under the impression that really large catalogs can take a long time to start-up. Do you know if that would trigger? All that said, I'm simply arguing that it's reasonable to make the number of retries infinite here. I look forward to what others will add! -- 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: 1 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 04 Oct 2017 16:29:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8202 Change subject: IMPALA-4704: Disallow client connections to imapalad until catalog is received. .. IMPALA-4704: Disallow client connections to imapalad until catalog is received. Currently, impalad starts beeswax and hs2 servers even if the catalog has not yet been received. As a result, client connections see an error message stating that the impalad is not yet ready. This patch changes the impalad startup sequence to wait until the catalog is received before starting the beeswax and hs2 servers. The wait time is bounded by a constant number of attempts (5). Each attempt waits for the maximum allowable wait time for catalog notifications (2s). If the catalog is not ready within this wait time bound, the impalad exits. Testing: - python e2e tests that start a cluster without a catalog and check that impalad's die as expected and that client connections are rejected as expected. Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 --- M be/src/service/frontend.cc M be/src/service/frontend.h M be/src/service/impala-server.cc M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M tests/common/impala_cluster.py A tests/custom_cluster/test_catalog_wait.py 7 files changed, 134 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/8202/1 -- 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: newchange Gerrit-Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 Gerrit-Change-Number: 8202 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac
