Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14824 )
Change subject: IMPALA-9199: Add support for single query retries on cluster membership changes ...................................................................... Patch Set 1: (6 comments) Pretty major update. Think the feature is coming along and all the core functionality is present. @Thomas addressed your comments and rebased on top of master. As discussed, removed all the changes to status.h and status.cc. Retries are now driven by cluster membership changes or blacklist events. Still a lot TODO, but appreciate any early feedback. Some additional re-factoring to make the code cleaner is probably necessary, along with a lot more documentation. A lot more testing is necessary as well. http://gerrit.cloudera.org:8080/#/c/14824/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14824/1//COMMIT_MSG@44 PS1, Line 44: Retries are transparent to the user > | So, if a query fails and is retried the failed version will just Ended up implementing your suggestion for adding a mapping between query ids. See ImpalaServer::client_query_id_mapping_ http://gerrit.cloudera.org:8080/#/c/14824/1/be/src/common/status.h File be/src/common/status.h: http://gerrit.cloudera.org:8080/#/c/14824/1/be/src/common/status.h@308 PS1, Line 308: overriding any existing values > Yeah, legitimate concern. Although FWIW I think we have the same issue with Removed all of this. http://gerrit.cloudera.org:8080/#/c/14824/1/be/src/common/status.cc File be/src/common/status.cc: http://gerrit.cloudera.org:8080/#/c/14824/1/be/src/common/status.cc@255 PS1, Line 255: msg_->AddDetail(status.msg().msg()); > I think you're missing a 'msg_->SetStatusProperties(properties)' here Removed all this. http://gerrit.cloudera.org:8080/#/c/14824/2/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/14824/2/be/src/service/impala-server.h@1007 PS2, Line 1007: > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/14824/1/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14824/1/common/thrift/ImpalaService.thrift@507 PS1, Line 507: due to cluster membership changes > I assume ultimately we'll want to use this same flag to control all situati Done http://gerrit.cloudera.org:8080/#/c/14824/1/common/thrift/Status.thrift File common/thrift/Status.thrift: http://gerrit.cloudera.org:8080/#/c/14824/1/common/thrift/Status.thrift@45 PS1, Line 45: 3: optional TStatusProperties status_properties > Yeah, we could include it in StatusAuxInfo (although now that StatusAuxInfo Removed all this. -- To view, visit http://gerrit.cloudera.org:8080/14824 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e4a0e72a9bf8ec10b91639aefd81bef17886ddd Gerrit-Change-Number: 14824 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Fri, 24 Jan 2020 16:58:21 +0000 Gerrit-HasComments: Yes
