[Impala-ASF-CR] IMPALA-9155: Add recovery mechanism to admission service
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17332 ) Change subject: IMPALA-9155: Add recovery mechanism to admission service .. Patch Set 1: (10 comments) Some more comments http://gerrit.cloudera.org:8080/#/c/17332/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17332/1//COMMIT_MSG@9 PS1, Line 9: Major changes: I wrote my own version of the commit message to see that I understand the change. Is this correct? Feel free to use this All heartbeats from the coordinator already contain a unique id for the coordinator. The admissiond uses this unique id to track the coordinators that have sent their full admission state. When the admissiond starts up (in particular when it restarts) it will not not service any requests from coordinators until it has received the full admission state of that coordinator. The trigger for coordinators to resend their full admission state is a new flag set in replies to the AdmissionHeartbeat messages that every coordinator periodically sends to the admissiond. If the new flag is set in the reply that the coordinator receives then the full admission state is sent to the admissiond. Note that when the admissiond restarts it will service requests from any coordinator that has sent its full admission state. This means that over-admission is possible during the period when not every coordinator has sent its full admission state. http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-client.cc File be/src/scheduling/admission-control-client.cc: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-client.cc@32 PS1, Line 32: "Re-submit for admission due to a possible admission service restart"; Is there a reason it is only a "possible" restart? http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-service.cc File be/src/scheduling/admission-control-service.cc: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-service.cc@304 PS1, Line 304: LOG(WARNING) << "Received heartbeat from unrecognized coord_id=" << req->coord_id(); Maybe WARNING is too high, we do expect to see this after a restart, maybe just make the message say that? http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-service.cc@332 PS1, Line 332: LOG(INFO) << "Query id already exists. Can be lingering from old coord_id=" Is it possible that the admissionstate we have just received is more up-to-date than that in admission_state_map_ ? http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-control-service.cc@449 PS1, Line 449: LOG(INFO) << "Adding to list of known coordinators, coord_id=" << coord_id; The size of known_coord_ids_ is interesting, maybe add it to the message. http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.h File be/src/scheduling/remote-admission-control-client.h: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.h@54 PS1, Line 54: /// TODO: add info on what this does? here or in the class comments Yes it seems like these methods don't have descriptions http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.cc File be/src/scheduling/remote-admission-control-client.cc: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.cc@131 PS1, Line 131: retry_admission = true; I think this was already set to true? http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/remote-admission-control-client.cc@158 PS1, Line 158: //is network error or admisiond did not recognize this coord. Nit: "admissiond" http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py@1604 PS1, Line 1604: def __compare_admission_json(self, json1, json2): This is a clever way to test this change. http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py@1606 PS1, Line 1606: # that are updated immediately and admission changes are compared. "and admission changes" is slightly unclear, maybe "is changed by admissiond" -- To view, visit http://gerrit.cloudera.org:8080/17332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd Gerrit-Change-Number: 17332 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Fri, 28 May 2021 19:46:41 +
[Impala-ASF-CR] IMPALA-9155: Add recovery mechanism to admission service
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17332 ) Change subject: IMPALA-9155: Add recovery mechanism to admission service .. Patch Set 1: (3 comments) I read through the code once, saving my nits here http://gerrit.cloudera.org:8080/#/c/17332/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17332/1//COMMIT_MSG@12 PS1, Line 12: - No RPCs are serviced by a coordinator unless it sends its complete Nit" should this be "No RPCs are serviced from a coordinator until it has sent"? http://gerrit.cloudera.org:8080/#/c/17332/1/common/protobuf/admission_control_service.proto File common/protobuf/admission_control_service.proto: http://gerrit.cloudera.org:8080/#/c/17332/1/common/protobuf/admission_control_service.proto@280 PS1, Line 280: /// those situations. An error response is sent if the coordinator has not yet sent a Move new comment to before the TODO to clarify that it is not part of the TODO http://gerrit.cloudera.org:8080/#/c/17332/1/common/protobuf/admission_control_service.proto@301 PS1, Line 301: /// ReleaseQuery. An error response is sent if the coordinator has not yet sent a Move new comment to before the TODO to clarify that it is not part of the TODO -- To view, visit http://gerrit.cloudera.org:8080/17332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd Gerrit-Change-Number: 17332 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Wed, 19 May 2021 21:28:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9155: Add recovery mechanism to admission service
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17332 ) Change subject: IMPALA-9155: Add recovery mechanism to admission service .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8620/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd Gerrit-Change-Number: 17332 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Thu, 22 Apr 2021 10:59:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9155: Add recovery mechanism to admission service
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17332 ) Change subject: IMPALA-9155: Add recovery mechanism to admission service .. Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-controller.cc@2103 PS1, Line 2103: DCHECK(num_backends_to_release_.find(state->query_id()) == num_backends_to_release_.end()); line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/17332/1/be/src/scheduling/admission-controller.cc@2104 PS1, Line 2104: num_backends_to_release_[state->query_id()] = state->per_backend_schedule_states().size(); line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py@1578 PS1, Line 1578: : flake8: E231 missing whitespace after ':' http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py@1585 PS1, Line 1585: - flake8: E226 missing whitespace around arithmetic operator http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py@1589 PS1, Line 1589: / flake8: E226 missing whitespace around arithmetic operator http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py@1614 PS1, Line 1614: 2 flake8: E501 line too long (99 > 90 characters) http://gerrit.cloudera.org:8080/#/c/17332/1/tests/custom_cluster/test_admission_controller.py@1626 PS1, Line 1626: k flake8: E501 line too long (97 > 90 characters) -- To view, visit http://gerrit.cloudera.org:8080/17332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd Gerrit-Change-Number: 17332 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Thu, 22 Apr 2021 10:40:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9155: Add recovery mechanism to admission service
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17332 Change subject: IMPALA-9155: Add recovery mechanism to admission service .. IMPALA-9155: Add recovery mechanism to admission service Major changes: - Leverages the admission heartbeat mechanism to signal the coordinator to send its complete admission state - No RPCs are serviced by a coordinator unless it sends its complete admission state. This is to prevent making admission decisions till admission service has built its view of the cluster - The complete admission state consists of the states of all queries that have successfully been admitted, that is, received a valid schedule from the admission controller and have marked its admission as complete (for remote admission it means its pending admit status has transitioned from true to false) - This helps prevent sending incomplete/inconsistent state to the admission controller - Queries that have not started admission get a chance to send their request to the new service - Queries that are queued restart the admission process by sending the request again. This re-try is now also marked in the query profile - Other RPCs like ReleaseBackend, ReleaseQuery, CancelQuery that don't get serviced (till initial admission state is sent) can result in inconsistent state. This state will be rectified in the admission heartbeats - AdmitQuery and GetQueryStatus just retry again if they notice a network failure(assuming admissiond might be down/restarting) or received the error message that they cannot be serviced yet admissiond is waiting on initial state from this coordinator) Limitations: - Rebuilding the state can not ensure that queued queries will maintain their spot in the queue. - Queries can be admitted before all coordinators get a chance to send their state. This can result in a brief period of over-admission We cannot rely completely on the statestore membership update and wait for all coordinators there to send admission state because that membership is also dynamic which makes it difficult to decide when to assume that the admission state is complete. Testing: - Added end to end tests Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd --- M be/src/runtime/coordinator-backend-resource-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/scheduling/admission-control-client.cc M be/src/scheduling/admission-control-client.h M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-control-service.h M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/local-admission-control-client.cc M be/src/scheduling/local-admission-control-client.h M be/src/scheduling/remote-admission-control-client.cc M be/src/scheduling/remote-admission-control-client.h M be/src/scheduling/schedule-state.cc M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/protobuf/admission_control_service.proto M common/thrift/generate_error_codes.py M tests/custom_cluster/test_admission_controller.py 23 files changed, 674 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/17332/1 -- To view, visit http://gerrit.cloudera.org:8080/17332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8ad3ef9b9e2496c484833d6326ce914c851e02fd Gerrit-Change-Number: 17332 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
