[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/10754 )
Change subject: IMPALA-6918: Implement COMMENT ON COLUMN
..
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10754/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10754/6//COMMIT_MSG@12
PS6, Line 12: 'comment'
State somewhere that this can be null and define the expected behavior for this
case.
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3611
PS6, Line 3611: if (!catalog_.tryLockTable(tbl)) {
: throw new InternalException(String.format("Error altering
table/view %s due to " +
: "lock contention.", tbl.getFullName()));
: }
this is the fourth time this error message and check is done... its bound to
become inconsistent, pls factor it into a separate method.
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3636
PS6, Line 3636: updates
nit: update
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4027
PS6, Line 4027: new Pair<>("fun
what is this case adding to the one on L4025?
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl.py@337
PS6, Line 337: is null".fo
since the null case follows '' (empty string), how do you know that null had an
effect vs. had no effect? I think it would be more obvious if the null case was
applied to a column with a non-empty comment.
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl_base.py
File tests/metadata/test_ddl_base.py:
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl_base.py@107
PS6, Line 107: print(result)
for debugging?
--
To view, visit http://gerrit.cloudera.org:8080/10754
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437
Gerrit-Change-Number: 10754
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 05 Jul 2018 06:20:51 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Hello Michael Ho, Lars Volker, Fredy Wijaya, Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10744
to look at the new patch set (#9).
Change subject: IMPALA-1760: Implement shutdown command
..
IMPALA-1760: Implement shutdown command
This allows graceful shutdown of executors and partially graceful
shutdown of coordinators (new operations fail, old operations can
continue).
Details:
* In order to allow future admin commands, this is implemented with
function-like syntax and does not add any reserved words.
* ALL privilege is required on the server
* The coordinator impalad that the client is connected to can be shut
down directly with ":shutdown()".
* Remote shutdown of another impalad is supported, e.g. with
":shutdown('hostname')", so that non-coordinators can be shut down
and for the convenience of the client, which does not have to
connect to the specific impalad. There is no assumption that the
other impalad is registered in the statestore; just that the
coordinator can connect to the other daemon's thrift endpoint.
This simplifies things and allows shutdown in various important
cases, e.g. statestore down.
* The shutdown time limit can be overridden to force a quicker or
slower shutdown.
* If shutting down, a banner is shown on the root debug page.
Workflow:
1. (if a coordinator) clients are prevented from submitting
queries to this coordinator via some out-of-band mechanism,
e.g. load balancer
2. the shutdown process is started via ":shutdown()"
3. a bit is set in the statestore and propagated to coordinators,
which stop scheduling fragment instances on this daemon
(if an executor).
4. the quiesce period (which is ideally set to the AC queueing
delay plus some additional leeway) expires
5. once the daemon is drained (i.e. no fragments, no registered
queries), it shuts itself down.
6. If the daemon does not drain (e.g. rogue clients, long-running
queries), after a longer timeout it will shut down anyway.
What this does:
* Executors can be shut down without causing a service-wide outage
* Shutting down an executor will not disrupt any short-running queries
and will wait for long-running queries up to a threshold.
* Coordinators can be shut down without query failures only if
there is an out-of-band mechanism to prevent submission of more
queries to the shut down coordinator.
* Long running queries or other issues (e.g. stuck fragments) will
slow down but not prevent eventual shutdown.
Limitations:
* The quiesce period needs to be configured to be greater than the
latency of statestore updates + scheduling + admission +
coordinator startup. Otherwise a coordinator may send a
fragment instance to the shutting down impalad. (We could
automate this configuration as a follow-on)
* The quiesce period means a minimum latency for shutdown,
even if the cluster is idle.
* We depend on the statestore detecting the process going down
if queries are still running on that backend when the timeout
expires. This may still be subject to existing problems,
e.g. IMPALA-2990.
Tests:
* Added parser, analysis and authorization tests.
* End-to-end test of shutting down impalads.
* End-to-end test of shutting down then restarting an executor while
queries are running.
* End-to-end test of shutting down a coordinator
- New queries cannot be started on coord, existing queries continue to run
- Exercises various Beeswax and HS2 operations.
Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0
---
M be/src/runtime/backend-client.h
M be/src/runtime/data-stream-test.cc
M be/src/scheduling/scheduler.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/fault-injection-util.h
M be/src/util/default-path-handlers.cc
M common/thrift/Frontend.thrift
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/LimitElement.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M tests/common/impala_cluster.py
M tests/common/impala_service.py
M tests/custom_clu
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 ) Change subject: IMPALA-1760: Implement shutdown command .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG@46 PS7, Line 46: does > extra word? Done http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc@626 PS7, Line 626: RETURN_IF_ERROR(client_status); > Should we retry if we get a stale client (similar to here: https://github.c Done -- To view, visit http://gerrit.cloudera.org:8080/10744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0 Gerrit-Change-Number: 10744 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 06:10:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10744 )
Change subject: IMPALA-1760: Implement shutdown command
..
Patch Set 8:
(30 comments)
http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG@56
PS7, Line 56: Limitations:
> I think it would be good to mention that the shutdown cannot be aborted. Ev
Agree these are useful to note but this commit message is already out of
control. Might be easier to track in JIRA.
I wonder if we should rename is_shutting_down to is_quiesced or something like
that in the statestore topic if we're going to generalise it later.
http://gerrit.cloudera.org:8080/#/c/10744/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10744/8//COMMIT_MSG@39
PS8, Line 39: which is
> how about: which ideally is set to the AC
Done
http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.h@a451
PS7, Line 451:
> What happened to LOAD DATA?
There wasn't an implementation of this function, just cleaned up the unused
declaration.
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/client-request-state.cc@616
PS6, Line 616:
> nit:avoids
Done
http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc@612
PS7, Line 612: avoid
> nit: avoids
Done
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-hs2-server.cc@381
PS6, Line 381: HS2_RETURN_IF_ERROR(return_val, CheckNotShuttingDown(),
SQLSTATE_GENERAL_ERROR);
> we can probably remove the check here since its not starting any new operat
That brings up a good point - I was a bit uncertain about what the best thing
to do with requests like this. On one hand we could just continue to serve them
and then if the client continues to send them at some point the connection will
drop and the client will get a socket error or something similar. I thought
that it might be better to fail with a clear error earlier so it's more obvious
why the operation failed.
It may be that there's no perfect solution without better integration with load
balancer and clients.
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@354
PS6, Line 354: otherwi
> nit: shutdown_status
Done
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@358
PS6, Line 358:
> dont mention private vars in public method comments. also at L367
Done
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@361
PS6, Line 361: ft
> nit: a
Done
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@362
PS6, Line 362: ly.
> nit:before
Done
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.cc@212
PS6, Line 212:
> nit: long line
Done
http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/impala-server.cc@216
PS7, Line 216: drain
> I'd rephrase it as "wait for queries" since they might not even have arrive
Done
http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/impala-server.cc@2328
PS7, Line 2328: while (deadline_val == 0 || deadline_val >
absolute_deadline_ms) {
> maybe name these "curr_deadline_val" and "new_deadline_val"? or something s
Done
http://gerrit.cloudera.org:8080/#/c/10744/8/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/10744/8/be/src/service/impala-server.cc@1772
PS8, Line 1772: && is_shutting_down == it->second.is_shutting_down) {
> maybe mention briefly that is_shutting_down is the only part that can chang
Done
http://gerrit.cloudera.org:8080/#/c/10744/6/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/10744/6/common/thrift/ImpalaInternalService.thrift@865
PS6, Line 865: 5: optional TBloomFilter bloom_filter
> not sure if mentioned elsewhere in the code, but maybe we can explain the d
This probably isn't the best place to document since it's just a status
message. I think I probably need to
[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down
Hello Lars Volker, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10847 to look at the new patch set (#2). Change subject: IMPALA-6271: Impala daemon should log a message when it's being shut down .. IMPALA-6271: Impala daemon should log a message when it's being shut down Currently Impalad does not log any message when SIGTERM is sent to impalad to terminate or to do a graceful shutdown. This change logs a message when SIGTERM is recieved by impalad. This logging will assist in debugging the issues seen in the field where impalad was not gracefully shutdown. Testing: --- a) Used kill to send signals to impalad `kill -s SIGTERM ` and see the message is being logged in impalad.INFO. b) Ran test_breakpad.py to check that existing breakpad functionalities are not affected. c) Ran exhaustive tests without failure. Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29 --- M be/src/service/impalad-main.cc M be/src/util/minidump.cc 2 files changed, 14 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/10847/2 -- To view, visit http://gerrit.cloudera.org:8080/10847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29 Gerrit-Change-Number: 10847 Gerrit-PatchSet: 2 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Pranay Singh
