Tim Armstrong 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 10: (13 comments) I had to refresh my memory on how the query lifecycle worked for inserts, but I think I paged enough back in to have some valid thoughts on it. The biggest problem I think is the one that you identified - that the transaction commit, Impala catalog and HMS updates are not atomic from the point of view of Impala, so we're opening ourselves up to various anomalies. I haven't really thought through exactly what anomalies are possible, but it would be good to avoid them entirely, if possible. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/exec/hdfs-table-sink.h File be/src/exec/hdfs-table-sink.h: http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/exec/hdfs-table-sink.h@264 PS10, Line 264: long int64_t? http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/exec/hdfs-table-sink.cc File be/src/exec/hdfs-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/exec/hdfs-table-sink.cc@500 PS10, Line 500: // However, for transactional tables we should create a new empty base directory. Why? I assume there is some good reason but it's not immediately obvious to me. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/exec/hdfs-table-sink.cc@700 PS10, Line 700: if (IsTransactional()) return true; This one seems more obvious to me, but makes me think that the class comment should have a brief expectation of the directory layout and behaviour of ACID inserts. Or a pointer to something that explains it. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/runtime/coordinator.cc@571 PS10, Line 571: FinalizeHdfsInsert We should maybe rename this to FinalizeHdfsDml(), now or later. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/runtime/coordinator.cc@571 PS10, Line 571: Status Coordinator::FinalizeHdfsInsert() { I think we should probably do the transaction abort in this function, since it will happen asynchronously and not depend on the client unregistering the query. I think it fits conceptually with removing the staging directory and that cleanup. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/client-request-state.h@307 PS10, Line 307: /// True if there is an open transaction. Hive ACID transaction, just to be clear. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/client-request-state.h@308 PS10, Line 308: bool in_transaction_ = false; I think this would best fit in DmlExecState. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/client-request-state.cc@720 PS10, Line 720: DCHECK(exec_request().__isset.transaction_id); I think we would prefer to abort the transaction earlier in the query lifecycle. Query unregistration does not necessarily happen in a timely fashion because it depends on client RPCs. Coordinator::FinalizeHdfsInsert() is maybe the right place. This could also be a helper, maybe - Done() is getting to the point where it doesn't fit on a screen. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/client-request-state.cc@820 PS10, Line 820: //TODO: HMS is not updated at this point, only in UpdateCatalog(). But Zoli explained this to me out-of-band. I think I agree that having the catalog commit the transaction is the right approach, since we'd want transaction commit and the Impala catalog update to be an atomic operation from the point-of-view of impalads. It looks like there's an add_dynamic_partitions() method that takes a transaction ID, so I think in theory we could solve that partition creation problem by having the impalad create the partitions in the transaction, but that would still leave a window of inconsistency. It's a little asymmetrical to have the Impala start the transaction and catalogd commit it, but it seems less weird than the consistency issues. http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/frontend.h@170 PS10, Line 170: /// Commits transaction with the given transaction id. Are there any invariants we should document for these methods? I guess we're just assuming that this coordinator had opened a transaction previous. Are there any interesting failure modes to document? http://gerrit.cloudera.org:8080/#/c/13559/10/be/src/service/frontend.h@171 PS10, Line 171: long I think we want to use int64_t, to match the thrift definition and our style guide. http://gerrit.cloudera.org:8080/#/c/13559/10/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/13559/10/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@179 PS10, Line 179: private Long writeId_; This is just a nit, but it would be slightly more intuitive to me to use -1 here to represent not set, since that's what's used in the backend for write IDs and generally elsewhere in the frontend for similar cases (cardinality estimates, etc). http://gerrit.cloudera.org:8080/#/c/13559/10/testdata/workloads/functional-query/queries/QueryTest/acid-insert.test File testdata/workloads/functional-query/queries/QueryTest/acid-insert.test: http://gerrit.cloudera.org:8080/#/c/13559/10/testdata/workloads/functional-query/queries/QueryTest/acid-insert.test@3 PS10, Line 3: RESET insertonly_nopart_insert We've generally preferred using unique_database for tables that are mutated as part of tests so that the tests can run in parallel. The execute_serially thing in test_insert is a bit of a legacy thing. Not a blocker, or maybe I'm missing a reason not to use unique_database. -- 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: 10 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 27 Jun 2019 23:46:56 +0000 Gerrit-HasComments: Yes
