Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12037 )
Change subject: IMPALA-7928: Consistent remote read scheduling ...................................................................... Patch Set 5: (12 comments) Some more comments. i think this is pretty close. I think we can make the hashing simpler. http://gerrit.cloudera.org:8080/#/c/12037/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12037/5//COMMIT_MSG@12 PS5, Line 12: cache that is at the file level). There is no other cache at the file level, so i think this parenthetical confuses more than helps? http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/backend-config.cc File be/src/scheduling/backend-config.cc: http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/backend-config.cc@22 PS5, Line 22: // TODO: determine the appropriate number of replicas for the hash-ring Are we going to do this, or leave for the future? Fine to leave; just checking that you didn't forget about this accidentally. http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring-test.cc File be/src/scheduling/hash-ring-test.cc: http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring-test.cc@31 PS5, Line 31: class HashRingTest : public ::testing::Test { I don't see a test for GetNode(n) here. Something pretty basic should work. If you're willing to peek at internals, checking that the "overflow" works correctly and that picking n around the biggest address works is working correctly. If we want, we can write crash tests for Adding and Removing nodes that already exist or don't exist, though I'm ok without those. http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring-test.cc@39 PS5, Line 39: EXPECT_EQ(hash_ring.GetTotalReplicas(), num_nodes * hash_ring.GetNumReplicas()); This assumes no collisions? http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.h File be/src/scheduling/hash-ring.h: http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.h@42 PS5, Line 42: and expects the caller to do its own hashing for the item. Does this conflict with the TODO about "function pointer that generates a hash value"? http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.h@56 PS5, Line 56: : num_replicas_(num_replicas) {} assert that num_replicas >= 1? http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.h@58 PS5, Line 58: /// Copy constructor Why do we need a copy constructor? We need it to copy the backend, eh? http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.h@64 PS5, Line 64: for (auto it = hash_ring.node_hashmap_.begin(); : it != hash_ring.node_hashmap_.end(); : it++) { : uint32_t hash_value = it->first; : NodeIterator old_node_it = it->second; : // Look up the equivalent node iterator in the new nodes_ set. : NodeIterator new_node_it = nodes_.find(*old_node_it); : DCHECK(new_node_it != nodes_.end()); : node_hashmap_.insert(std::pair<int32_t, NodeIterator>(hash_value, new_node_it)); : } Should this be moved out of the header? http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.h@90 PS5, Line 90: /// value. Add: The hash ring must not be empty. (I do wonder if we can get in trouble if the executors haven't joined the coordinators yet...) http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.cc File be/src/scheduling/hash-ring.cc: http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.cc@78 PS5, Line 78: DCHECK_GT(num_removed, 0); If we have just the right hash collisions, it's possible that num_removed == 0. I don't think this affects our correctness (the node just never gets any work scheduled to it, and it's unlikely). But am I understanding this right? http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/scheduling/hash-ring.cc@91 PS5, Line 91: return &(*node_it); If node_it == node_hashmap_.end(), should you return nullptr here? In debug mode, DCHECK_GT will prevent this from reaching, so it probably doesn't matter. http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/util/hash-util.h File be/src/util/hash-util.h: http://gerrit.cloudera.org:8080/#/c/12037/5/be/src/util/hash-util.h@290 PS5, Line 290: class MultipleHashGenerator { I suspect we can just use Rehash32to32 above. -- To view, visit http://gerrit.cloudera.org:8080/12037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbf74088a8bd8c285ab7285ea3a01acd1bb53a45 Gerrit-Change-Number: 12037 Gerrit-PatchSet: 5 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Thu, 31 Jan 2019 19:30:00 +0000 Gerrit-HasComments: Yes
