Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17553 )
Change subject: WIP IMPALA-10557: Support Kudu's multi-row transaction ...................................................................... Patch Set 1: (5 comments) Just a few comments. Looks good. http://gerrit.cloudera.org:8080/#/c/17553/1/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/17553/1/be/src/service/client-request-state.cc@825 PS1, Line 825: if (InTransaction()) { : AbortTransaction(); : } else if (InKuduTransaction()) { : AbortKuduTransaction(); : } I wonder the rational to decouple kudu tractions from impala ones. For example, for the following query, if it is protected via a transaction, you would need to protect both tables in the same transaction. insert into kudu_t as select * from impala_t http://gerrit.cloudera.org:8080/#/c/17553/1/fe/src/main/java/org/apache/impala/common/KuduTransactionManager.java File fe/src/main/java/org/apache/impala/common/KuduTransactionManager.java: http://gerrit.cloudera.org:8080/#/c/17553/1/fe/src/main/java/org/apache/impala/common/KuduTransactionManager.java@54 PS1, Line 54: put(queryId, txn); Normally, a transaction can cover multiple queries as follows. begin work; <q1> <q2> ... commit; I wonder if kudu/impala supports such semantics. http://gerrit.cloudera.org:8080/#/c/17553/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/17553/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1717 PS1, Line 1717: else if ((targetTable instanceof FeKuduTable) : && queryOptions.isKudu_transaction()) { : FeKuduTable kuduTable = (FeKuduTable) targetTable; : KuduClient client = KuduUtil.getKuduClient(kuduTable.getKuduMasterHosts()); : KuduTransaction txn = null; : try { : // Open Kudu transaction. : txn = client.newTransaction(); : timeline.markEvent("Kudu transaction opened with query id: " : + queryCtx.getQuery_id().toString()); : insertStmt.setTransactionToken(txn.serialize()); : kuduTxnManager_.addTransaction(queryCtx.getQuery_id(), txn); : queryCtx.setIs_kudu_transactional(true); : } catch (IOException e) { : if (txn != null) txn.close(); : throw new TransactionException(e.getMessage()); : } : } This covers insert/createTableAsSelect stmts which is very nice. Just wonder if we need to worry about other IUD cases, such as delete, or update. http://gerrit.cloudera.org:8080/#/c/17553/1/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/17553/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@772 PS1, Line 772: TUniqueId queryId Would it be possible to pass in a kuduTransaction object direct here? This helps remove the restriction of one to one mapping of query id to transaction. http://gerrit.cloudera.org:8080/#/c/17553/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@782 PS1, Line 782: TUniqueId queryId Same comment as above. -- To view, visit http://gerrit.cloudera.org:8080/17553 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I876ada48991afdff5d61b5d6a0417571aba7cb34 Gerrit-Change-Number: 17553 Gerrit-PatchSet: 1 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Comment-Date: Mon, 07 Jun 2021 21:14:59 +0000 Gerrit-HasComments: Yes
