Just took a look. +1 On Tue, Jun 9, 2020 at 12:59 AM max ulidtko <ulid...@gmail.com> wrote:
> 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 > > > > > -- -- Randy Abernethy Managing Partner RX-M, llcrandy.aberne...@rx-m.com o 415-800-2922 c 415-624-6447
