Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/9818 )
Change subject: KUDU-2351 Add IP/port for Recv() failure ...................................................................... Patch Set 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@1 PS9, Line 1: // Licensed to the Apache Software Foundation (ASF) under one > nit: blank line at top of file Done http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@45 PS9, Line 45: virtual void TearDown() override { > style: indentation seems off in this file. we only indent private/public by Done http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@46 PS9, Line 46: for (auto& thread : threads) { : thread.join(); : } > is this necessary? you're right, I forgot deleting it after refactoring http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@56 PS9, Line 56: > again strange indent Done http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@63 PS9, Line 63: CHECK_OK(lis > hrm, we don't really use std::promise and std::future elsewhere in the code ok, changing it. would it make sense to rewrite Promise in util/promise.h to be a wrapper around std::future for now? I mean in a separate refactor commit of course http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@73 PS9, Line 73: istener_.Accept( > should CHECK_OK this Done http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@76 PS9, Line 76: SleepFor(MonoDelta::FromMilliseconds(200)); : CHECK_OK(lis > these aren't needed in this scope - they can be inside 'if (accept)' Done, also stayed there also refactoring... http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@80 PS9, Line 80: > also CHECK_OK Done http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@93 PS9, Line 93: UE(s.IsNetwork > here too Done -- To view, visit http://gerrit.cloudera.org:8080/9818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22436b13bb351b132e1c0b7159294dd0c980c2b3 Gerrit-Change-Number: 9818 Gerrit-PatchSet: 10 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 04 Apr 2018 11:37:26 +0000 Gerrit-HasComments: Yes
