Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21048 )
Change subject: IMPALA-12426: Workload Management Supporting Changes ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/network-util.cc File be/src/util/network-util.cc: http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/network-util.cc@266 PS3, Line 266: const int uds_addr_compare = a.uds_address.compare(b.uds_address); uds_address is the newest field and most likely empty (flag rpc_use_unix_domain_socket still False by default). I think uds_address should be the last tie breaker instead of port? http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/string-util-test.cc File be/src/util/string-util-test.cc: http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/string-util-test.cc@272 PS3, Line 272: NotEmptyPopOnce Can you add another test that compares the result is equal with regular stringstream? Start both with multiple string append that sufficiently long. http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/string-util-test.cc@284 PS3, Line 284: t Change to char other than t. http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/string-util-test.cc@297 PS3, Line 297: EmptyPopOnce Add another test like this, but append something before asserting. http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/ticker-test.cc File be/src/util/ticker-test.cc: http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/ticker-test.cc@84 PS3, Line 84: [&wakeup_guard, &wakeup_val]{ return *wakeup_guard == wakeup_val; } Can this be part of Ticker class itself? Maybe Ticker class can have method to return this lambda expression? http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/ticker.h File be/src/util/ticker.h: http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/ticker.h@86 PS3, Line 86: iteraction iteration http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/ticker.h@122 PS3, Line 122: TickerSB nit: I'd rather spell this out to TickerSecondsBool http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/uid-util.h File be/src/util/uid-util.h: http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/uid-util.h@135 PS3, Line 135: || why this is '||' instead of '&&'? Please cross-check with this comment in line 57 /// Query id: uuid with bottom 4 bytes set to 0 /// Fragment instance id: query id with instance index stored in the bottom 4 bytes -- To view, visit http://gerrit.cloudera.org:8080/21048 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iee23334ec56a18b192a75d052468bf59159b6424 Gerrit-Change-Number: 21048 Gerrit-PatchSet: 3 Gerrit-Owner: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Fri, 23 Feb 2024 00:46:31 +0000 Gerrit-HasComments: Yes
