Hi list, I'm the PR author — calling out for reviewers!
The diff is under 1 kLoC. I have it well-groomed already: • CI bots are green; • new code is clang-formatted; • entry in CHANGES.md added; • both buildsystems adjusted for new file; • non-pertinent changes split off; • Jens Geyer's "partial" code review remarks addressed; • Mario Emmenlau's remarks addressed. Mario has done, or is doing, his own testing of this patch. No issues have returned to me so far. I did my own testing — I describe which exactly in the commit message / patch header. If there is any C++ developer out there who feels like spending a share of his/her valuable > time to review this PR, that would be a great thing. But be warned, there is > a reason for > this request: The patch is a bit more complicated and has plenty of #ifdef-ed > code portions, > so better come prepared ;-) > https://github.com/apache/thrift/pull/2151 > > Right, about those #ifdef's. I add only one new #ifdef _WIN32, in TSocketUtils.h. It's used to correctly handle the slightly different meanings of getaddrinfo() error codes on win32 vs posix. All other #ifdef's in the diff are moves. As I explain in the commit message; several screenfuls of repetitive #ifdef-rich lines in the middle of important loop — are big nuisance and get in the way. So I grouped all those setsockopt()s and separated them into dedicated private methods. Makes it possible to navigate the listen() method again; I refrained from any further refactorings. git config diff.colormoved zebra — may help to see these code moves. GitHub diff viewer does not visualize those. Lastly: about third of the diff consists of just replicating the same changes as in TServerSocket onto TNonblockingServerSocket. These two classes are obviously "inherited" one from another (via copy-paste). I again refrained from refactoring any of that. Instead, I carefully did the same changes again to the other class. This is another reason why the diff is bigger than it needs to be (and it's not my fault). Please NOTE: an earlier patch from me ("Thrift libraries crash with localhost-only network"), merged post-0.13, does remedy the localhost-only situation — but also has a potential to increase the "Address family for hostname not supported" errors in _networked_ scenarios. Before that former patch, we've seen the error ~once a month or two on our systems; with it (vendored in-house build of Thrift), we continued to see the errors, somewhat even more frequently — but this time, on _networked_ machines (non-localhost-only), as it turned out. Which is why I went big, and developed this second patch. I hope you can see why I'd really really really like to get this patch ("Rewrite address resolution") landed in 0.14 — otherwise, the earlier patch may make some havoc. Hope to get some good looks on the patch (with git diff zebra enabled!..), helpful comments, and ultimately LGTM. Best regards Max On Fri, May 29, 2020 at 1:29 PM max ulidtko <ulid...@gmail.com> wrote: > Hi all, >> >> the PR creator is a bit hesitant about it, so I ask for him: >> >> If there is any C++ developer out there who feels like spending a share of >> his/her valuable >> time to review this PR, that would be a great thing. But be warned, there is >> a reason for >> this request: The patch is a bit more complicated and has plenty of >> #ifdef-ed code portions, >> so better come prepared ;-) >> https://github.com/apache/thrift/pull/2151 >> >> Right, about those #ifdef's — I add only one new #ifdef _WIN32, in > TSocketUtils.h. It's used to correctly handle the slightly different > meanings of getaddrinfo() error codes on win32 vs posix. > > All other #ifdef's in the diff are moves. As I explain in the commit > message; several screenfuls of repetitive #ifdef-rich lines in the middle > of important loop are big nuisance and get in the way. So I grouped all > those setsockopt()s and separated them into dedicated private methods. > Makes it possible to navigate the listen() method; I refrained from any > further refactorings. > > git config diff.colormoved zebra — may help to see these code moves. > GitHub diff viewer doesn't visualize those. > > Lastly: around third of the diff consists of just replicating the same > changes as in TServerSocket onto TNonblockingServerSocket. These two > classes are obviously "inherited" from each other (by copy-paste). I again > refrained from refactoring any of that. Instead, I carefully did the same > changes again to the other class. This is another reason why the diff is > bigger than it needs to be (and it's not my fault). > > Max > >
