On Mon, 18 Jan 2021 19:27:53 +0100 Jonas Bonn wrote: > On 18/01/2021 18:27, Jakub Kicinski wrote: > > v5 itself was laying around on patchwork for almost a week, marked as > > "Needs Review/Ack". > > When new series show up just hours after review, it's hard to take them > seriously. It takes a fair amount of time to go through an elephant > like this and to make sense of it; the time spent in response to review > commentary shouldn't be less.
Agreed. > > Normally we try to merge patches within two days. If anything my > > lesson from this whole ordeal is in fact waiting longer makes > > absolutely no sense. The review didn't come in anyway, and we're > > just delaying whatever project Pravin needs this for :/ > > I think the expectation that everything gets review within two days is > unrealistic. Right, it's perfectly fine to send an email saying "please wait, I'll review it on $date". > Worse though, is the insinuation that anything unreviewed > gets blindly merged... No, the two day target should be for the merging > of ACK:ed patches. Well, certainly, the code has to be acceptable to the person merging it. Let's also remember that Pravin is quite a seasoned contributor. > > Do I disagree with you that the patch is "far from pretty"? Not at all, > > but I couldn't find any actual bug, and the experience of contributors > > matters to us, so we can't wait forever. > > > >> The following issues remain unaddressed after review: > >> > >> i) the patch contains several logically separate changes that would be > >> better served as smaller patches > >> ii) functionality like the handling of end markers has been introduced > >> without further explanation > >> iii) symmetry between the handling of GTPv0 and GTPv1 has been > >> unnecessarily broken > >> iv) there are no available userspace tools to allow for testing this > >> functionality > > > > I don't understand these points couldn't be stated on any of the last > > 3 versions / in the last month. > > I believe all of the above was stated in review of series v1 and v2. v3 > was posted during the merge window so wasn't really relevant for review. > v4 didn't address the comments from v1 and v2. v5 was posted 3 hours > after receiving reverse christmas tree comments and addressed only > those. v5 received commentary within a week... hardly excessive for a > lightly maintained module like this one. Sorry, a week is far too long for netdev. If we were to wait that long we'd have a queue of 300+ patches always hanging around. > >> I have requested that this patch be reworked into a series of smaller > >> changes. That would allow: > >> > >> i) reasonable review > >> ii) the possibility to explain _why_ things are being done in the patch > >> comment where this isn't obvious (like the handling of end markers) > >> iii) the chance to do a reasonable rebase of other ongoing work onto > >> this patch (series): this one patch is invasive and difficult to rebase > >> onto > >> > >> I'm not sure what the hurry is to get this patch into mainline. Large > >> and complicated patches like this take time to review; please revert > >> this and allow that process to happen. > > > > You'd need to post a revert with the justification to the ML, so it can > > be reviewed on its merits. That said I think incremental changes may be > > a better direction. > > I guess I'll have to do so, but that seems like setting the bar higher > than for even getting the patch in in the first place. > > I don't think it's tenable for patches to sneak in because they are so > convoluted that the maintainers just can't find the energy to review > them. I'd say that the maintainers silence on this particular patch > speaks volumes in itself. Sadly most maintainers are not particularly dependable, so we can't afford to make that the criteria. I have also pinged for reviews on v4 and nobody replied. > Sincerely frustrated because rebasing my IPv6 series on top of this mess > will take days, I sympathize, perhaps we should document the expectations we have so less involved maintainers know the expectations :(
