Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10855 )
Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC ...................................................................... Patch Set 11: (15 comments) http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/exec/hdfs-parquet-table-writer.h File be/src/exec/hdfs-parquet-table-writer.h: http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/exec/hdfs-parquet-table-writer.h@199 PS10, Line 199: ParquetInsertStatsPB parquet_insert_stats_; nit: should #include the appropriate .pb.h here ("include-what-you-use") http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/coordinator-backend-state.h@60 PS10, Line 60: Coordinator* coord I think this can probably change back to being const if you take the suggestion below. Also please add a doc comment indicating that the Coordinator must remain valid until the BackendState object is destructed. http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/coordinator-backend-state.h@176 PS10, Line 176: last_report_ti nit: I think the term "sequence number" is more usual here -- "version" to me sounds like each update is always a "replace the previous update", whereas "sequence" indicates that there should not be gaps http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/coordinator-backend-state.h@220 PS10, Line 220: /// Reference to the owning coordinator's 'dml_exec_state_'. This "back pointer" still seems error-prone to me. I think the object lifetimes and relationships will be clearer if you pass this in as an out-argument to every call to ApplyExecStatusReport(), so that the struct becomes single-owner without "escaping" pointers. http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/coordinator-backend-state.cc@265 PS10, Line 265: } : isn't it possible (though unlikely) that you have some old report sitting in the service queue long enough that you could get some late delivery with a sequence number that is < last_report_version, not just equal to? http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/coordinator-backend-state.cc@267 PS10, Line 267: inline bool Coordinator::BackendState::IsDone() const { I think a VLOG_QUERY about the skipped RPC is probably useful http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/coordinator-backend-state.cc@294 PS10, Line 294: instance_stats->Update( nit: why not: const Status& instance_status = instance_exec_status.status(); it's a bit more conventional for making a copy (if necessary) and will also just take a reference if safe to do so. http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/query-state.cc@287 PS10, Line 287: if I understand the threading correctly, there are two entry points for sending a report: one is periodic during the query, and the other is on query completion ("done"). Once the "done" report is sent, any other reports will be ignored. Does this mean that it's possible we can race as follows? - periodic report constructs a request including calling GetUnreportedErrors - perhaps gets a SerivceUnavailable and goes to sleep - the query finishes and triggers a 'done' report, with no errors (because they got consumed by the above already) - the first report retries and its report gets ignored because the fragment is already "done" thus we'd be losing the reported errors. Perhaps this is a known/existing bug, but maybe its' worth documenting the behavior in a TODO here if not a JIRA? On a related note, I'm afraid the following race might cause a problem: - exec thread reporting "done" allocates seqno 10 - makes a request, gets "unavailable" or whatever (or just goes to sleep for a few millis) - profile reporting thread allocates seqno 11 and sends a request, which is accepted by the server -- the server side increases the sequence number to 11 - the exec thread sends the "I'm done" message with seqno 10 - the server now ignores the "done" request and the query hangs Does that seem like a possibility to you? Maybe we could trigger such a race in a test by setting the service pool size for this endpoint to e smaller than the number of fragments and increasing the report frequency to once every millisecond or something? Given that the effect of a bug in this area is serious (hung queries) we should be pretty careful about testing I think. http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/query-state.cc@375 PS10, Line 375: MonoDelta::FromMilliseconds(FLAGS_backend_client_rpc_timeout_ms)); should we have a failure injection point on the RPC itself? I only saw failure injection on the serialization of the profile http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/query-state.cc@379 PS10, Line 379: (resp.status()); should we backoff? http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/runtime/runtime-state.cc@202 PS10, Line 202: (*new_errors)[v.first] = v.second; the method doc says that new_errors is cleared, but it's actually written into without a prior clear http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/service/control-service.cc File be/src/service/control-service.cc: PS10: This is a general krpc-in-Impala question: I can't find where you set up authorization for the KRPC services so that only the impala service user can connect to them. Is there a risk of an unauthorized (but authenticated) client principal connecting to this service? http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/util/uid-util.h File be/src/util/uid-util.h: http://gerrit.cloudera.org:8080/#/c/10855/10/be/src/util/uid-util.h@79 PS10, Line 79: TUniqueId result; worth DCHECKs here that the fields are set by calling uid_pb.IsInitialized()? http://gerrit.cloudera.org:8080/#/c/10855/10/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/10855/10/bin/impala-config.sh@562 PS10, Line 562: export HBASE_CONF_DIR="$IMPALA_FE_DIR/src/test/resources" why's this necessary? Can we change cmake to invoke it from the full path instead? http://gerrit.cloudera.org:8080/#/c/10855/10/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: http://gerrit.cloudera.org:8080/#/c/10855/10/tests/custom_cluster/test_rpc_exception.py@97 PS10, Line 97: @CustomClusterTestSuite.with_args("--status_report_interval=1 \ can we change this flag to be in millis instead of seconds? Or do we advertise its existence to users so it would be a breaking change to rename it? once a second isn't much of a stress -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 22 Aug 2018 00:15:24 +0000 Gerrit-HasComments: Yes
