Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13559 )
Change subject: IMPALA-8636: Implement INSERT for insert-only ACID tables ...................................................................... Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/13559/6/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/13559/6/be/src/service/client-request-state.cc@722 PS6, Line 722: in_transaction_ = false; > Can you add query_events_->MarkEvent("Transaction aborted");? See answer at Frontend.java. http://gerrit.cloudera.org:8080/#/c/13559/6/be/src/service/client-request-state.cc@820 PS6, Line 820: // UpdateCatalog() needs the transaction to be committed to load the new valid > Can you add query_events_->MarkEvent("Transaction committed");? See answer at Frontend.java. http://gerrit.cloudera.org:8080/#/c/13559/6/common/thrift/DataSinks.thrift File common/thrift/DataSinks.thrift: http://gerrit.cloudera.org:8080/#/c/13559/6/common/thrift/DataSinks.thrift@81 PS6, Line 81: write_id > Here and other similar variables: I would consider adding "acid_" or "trans Actually at first I called it "acid_write_id", but then I noticed that we already use the term "write id" at a lot of places so I just switched to "write_id" for consistency and brevity. Added the word "ACID" to the comment. http://gerrit.cloudera.org:8080/#/c/13559/6/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/13559/6/fe/src/main/java/org/apache/impala/service/Frontend.java@677 PS6, Line 677: * Parses and plans a query in order to generate its explain string. This method does > I wonder how to handle EXPLAIN - the backend won't be there to commit/abort Thanks, I didn't think about EXPLAIN. Done. http://gerrit.cloudera.org:8080/#/c/13559/6/fe/src/main/java/org/apache/impala/service/Frontend.java@1257 PS6, Line 1257: > Can you add timeline.markEvent("Transaction opened");? Yeah, I think it is a valid security concern. And maybe we shouldn't open a transaction until we are authorized to execute the query. And we should only load the write ids and list of files if we have an open transaction. Will leave it as it is right now, updated the TODO in the comment. http://gerrit.cloudera.org:8080/#/c/13559/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/13559/7/fe/src/main/java/org/apache/impala/service/Frontend.java@1378 PS7, Line 1378: } catch (Exception e) { > This may be out of the scope of this patch, but I would consider making abo Added TODO about it. -- To view, visit http://gerrit.cloudera.org:8080/13559 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id6c36fa6902676f06b4e38730f737becfc7c06ad Gerrit-Change-Number: 13559 Gerrit-PatchSet: 8 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 25 Jun 2019 14:07:13 +0000 Gerrit-HasComments: Yes
