Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/15497 )
Change subject: IMPALA-8005: Randomize partitioning exchanges. ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/15497/3/be/src/runtime/data-stream-test.cc File be/src/runtime/data-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/15497/3/be/src/runtime/data-stream-test.cc@514 PS3, Line 514: } nit: add a newline between this and next method http://gerrit.cloudera.org:8080/#/c/15497/3/be/src/runtime/data-stream-test.cc@725 PS3, Line 725: Reset(); Seems like this code block can be consolidated with the identical block on lines 711-720 and moved a common function. http://gerrit.cloudera.org:8080/#/c/15497/3/be/src/runtime/data-stream-test.cc@742 PS3, Line 742: if (map_query_1[i] != map_query_2[i]) { Is the non-match guaranteed though ? Given that there are only 4 receivers, depending on the random seed value, I am not sure if this test will always succeed, although the fix for the random seed itself is fine .. it just means that with high probability senders in 2 queries will not send to the same destination fragment. http://gerrit.cloudera.org:8080/#/c/15497/3/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/15497/3/be/src/runtime/krpc-data-stream-sender.cc@88 PS3, Line 88: exchange_hash_seed_ = 0x66bd68df22c3ef37 ^ state->query_id().hi; Suggest keeping the constant factor as a static constexpr in the header file such multiple places (e.g unit tests and regular code) can access it and also changes to it are centralized. Maybe you can rename it to EXCHANGE_HASH_SEED_CONST or something like that. -- To view, visit http://gerrit.cloudera.org:8080/15497 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1936e6cc3e8d66420a5a9301f49221ca38f3e468 Gerrit-Change-Number: 15497 Gerrit-PatchSet: 3 Gerrit-Owner: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Fri, 20 Mar 2020 23:44:09 +0000 Gerrit-HasComments: Yes
