Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21048 )
Change subject: IMPALA-12426: Workload Management Supporting Changes ...................................................................... Patch Set 7: (8 comments) > 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: if (a.port < b.port) { > uds_address is the newest field and most likely empty (flag rpc_use_unix_do I debated back and forth on this question. Looking at it again, I am going with your suggestion for the reason you gave plus checking port second makes more sense since hostname and port go together while uds_address is standalone. 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 str Excellent idea! Done. http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/string-util-test.cc@284 PS3, Line 284: v > Change to char other than t. Done 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. Done 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: rd = ""; > Can this be part of Ticker class itself? I like that idea. The vast majority of wakeup guard predicates will follow this pattern. 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: > iteration Done http://gerrit.cloudera.org:8080/#/c/21048/3/be/src/util/ticker.h@122 PS3, Line 122: > nit: I'd rather spell this out to TickerSecondsBool Done 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 '&&'? Good catch! I'm not sure why it's ||. It should be &&. I have seen instances where only the lo bytes are zeroed out. -- 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: 7 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 23 Feb 2024 19:20:04 +0000 Gerrit-HasComments: Yes
