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 12: (16 comments) http://gerrit.cloudera.org:8080/#/c/13559/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13559/11//COMMIT_MSG@25 PS11, Line 25: Testing: > I think this needs to be updated to explain what is done on the catalog. Done http://gerrit.cloudera.org:8080/#/c/13559/11//COMMIT_MSG@34 PS11, Line 34: * add tests for error paths via debug actions > I'm thinking about how we can test more of the error paths, that seems like It's a good idea. I'm planning to add such tests in the following PS. Until that I added a TODO to the commit msg. http://gerrit.cloudera.org:8080/#/c/13559/11//COMMIT_MSG@37 PS11, Line 37: * add locks and heartbeats (without heartbeats long-running transactions > What are the implications of not having heartbeats? Will long-running queri Yes, they will be aborted by HMS. Extended the commit message with this information. http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/exec/hdfs-table-sink.h File be/src/exec/hdfs-table-sink.h: http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/exec/hdfs-table-sink.h@134 PS11, Line 134: > "base or delta". It was confusing whether it was a path separate or or not Done http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.h@493 PS11, Line 493: Status FetchRowsInternal(const int32_t max_rows, QueryResultSet* fetched_rows) > Can you document the postcondition around transactions being committed/abor Done http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.cc@1106 PS11, Line 1106: Status status = client.DoRpc(&CatalogServiceClientWrapper::UpdateCatalog, > missing space Done http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.cc@1107 PS11, Line 1107: catalog_update, &resp); > It would be more explicit to Yeah, I think your suggestion makes the code cleaner. Thanks, done. http://gerrit.cloudera.org:8080/#/c/13559/11/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/13559/11/common/thrift/Frontend.thrift@361 PS11, Line 361: target > target Done http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@941 PS11, Line 941: loadValidWriteIdList(client); > I guess you didn't actually add this comment, is there a JIRA for this? No, I didn't write it and there is no Jira for that. I consulted with the author and this comment was just a reminder to do some investigation if whether we need to load the write ids at other code paths. So the text of it meant to be 'may also need to be loaded' I think. I removed this line because it's not an exact TODO. http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/common/TransactionException.java File fe/src/main/java/org/apache/impala/common/TransactionException.java: http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/common/TransactionException.java@21 PS11, Line 21: * Thrown for errors that occur when interacting with ACID transactions, > I feel like this could be a little more specific, e.g. "Thrown for errors t Done http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/common/TransactionException.java@22 PS11, Line 22: * e.g. failures to open, commit, or abort a transaction. Or, failing to > unnecesssary blank line? Done http://gerrit.cloudera.org:8080/#/c/13559/11/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/13559/11/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3801 PS11, Line 3801: e of fa > failures Done http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3803 PS11, Line 3803: if (update.isSetTransaction_id()) { > I think handling the abort on the client is probably the right thing to do Yeah it's reasonable, and it's also a better comment than 'error handling is confusing in this method' :) http://gerrit.cloudera.org:8080/#/c/13559/11/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/11/fe/src/main/java/org/apache/impala/service/Frontend.java@1266 PS11, Line 1266: try { > is there a jira? Opened IMPALA-8788. Done. http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/service/Frontend.java@1379 PS11, Line 1379: } catch (TransactionException te) { > nit: extra newline before catch Done http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/service/Frontend.java@1629 PS11, Line 1629: * Opens a new transaction. > The name/comment is a bit misleading, since it depends on whether the query Yeah I was struggling with the name but couldn't come up with a better one. Anyway, I identified a bug when we inserted data into a non-ACID table from an ACID table (we wrote the non-ACID table as it was an ACID one). So I changed the code a bit and don't use this function. Added tests for 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: 12 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: Wed, 24 Jul 2019 16:04:57 +0000 Gerrit-HasComments: Yes
