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 <abu...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <abu...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Comment-Date: Wed, 04 Apr 2018 11:37:26 +0000
Gerrit-HasComments: Yes

Reply via email to