Lars Volker has posted comments on this change. Change subject: IMPALA-4494: Fix crash in SimpleScheduler ......................................................................
Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/5127/3/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 204: if (delta.is_delta && delta.topic_entries.empty() && delta.topic_deletions.empty()) { > Can you comment on what this achieves (it's an optimisation to avoid a copy Done in the next PS http://gerrit.cloudera.org:8080/#/c/5127/4/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 261: if (new_backend_config->NumBackends() > 0 > I'm not sure about the NumBackends() > 0 check. The reasoning behind the check was that it prevents empty messages from the statestore (heartbeats) from setting up the local backend as the only known backend. If first an empty heartbeat arrives, then a query, and then the initial statestore message, then the whole query will be assigned to the scheduler. Assuming the first message by the statestore contains the cluster membership, I removed the >0 check. Line 264: new_backend_config->AddBackend(local_backend_descriptor_); > Do we also need to add this to current_membership_? I think we want curren So far current_membership_ has mirrored the statestore's view. Adding the local backend here will prevent the code below from registering it with the statestore. current_membership_ is also not used outside this method. Do you see a way to keep them consistent while still tracking both the local and the statestore views? -- To view, visit http://gerrit.cloudera.org:8080/5127 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6e1196a2fa47e5954c4a190aa326c135d039a77f Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
