Hi Neels,
On Wed, Jun 21, 2023 at 05:37:55AM +0200, Neels Hofmeyr wrote:
Let me compare two larger feature branches: There are weeks of review on 19 patches in osmo-hnbgw/cnpool, and <24h from submit to merge on 14 patches in osmo-msc.
I'm not saying this is good, but I think the difference is likely in terms of complexity of the related patches. Small patches doing simple to understand things are easy to review and hence likely to be merged more quickly.
The cnpool patch series for osmo-hnbgw spawned huge discussions and long delays on account of minor issues: log lines, event names and comments. I said so, yet it droned on and even spawned offspring.
It would be good to have pointers to specific gerrit patches here. I'm trying to look at it now, but topic:cnpool has so many patches that it's hard to find which ones you're referring-to exactly.
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33133 has a discussion about parsing of routing area identities and using shared code rather than open-coded implementations in various places. I consider that relevant and not cosmetic/bikeshedding
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33173 contains a discussion about naming. Naming *is* important, particularly if it uses terms which already have well-established meaning, such as (here) what is a RANAP RESET and what is a "lost link". Code in maintained software development projects is written not merely to execute, but for other developers to read and continue to develop/maintain. If terminology is used in a wrong way, it is likely misunderstood by other developers, causing problems down the road when investigating bugs or adding new functionality. I am seeing quite a lot of questionable naming in general when it comes to SS7/SIGTRAN. Also, event/state names of FSMs may end up in log or VTY output, where it has the potential to confuse not just other developers but users.
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33178 contains a "maybe add a log line" spawning a discussion with relatively verbose messages in it. That might be one of the cases you are referring to? To be fair it was a "maybe" comment, so a simple one line response might have been sufficient.
None of the important features was part of code review: larger design choices of the API, efficiency in handling the actual signalling, correctness of the signalling.
tbh, if there's a series of (it feels like) 30+ patches doing lots of complex stuff, it's hard to assume anyone is able to grasp those from reading the diffs. For correctness of signalling the test cases and associated pcaps are probably a better resource than gerrit patch review.
And at the same time I was "just now" asked to review some patches for osmo-msc, which changes an API that I have recently added and had ideas and reasons for.
again, please provide specific pointers. Without that you are expecting every reader of this e-mail to re-read all osmo-msc patches of the last few weeks just to find out which specific one[s] you are referring to.
It might have been https://gerrit.osmocom.org/c/osmo-msc/+/33358/1 where I overlooked a *resolved* comment "Not merging this one yet to give Neels a chance to review as well since he wrote the codec filter stuff that this is now modeled after." which I didn't see as the patch and many following it had two positive reviews and built correctly and hence I merged the entire series.
My workflow is usually to perform code review, and then if I'm done and there are series or parts of series with V+1/R+2 merge those entire series/chunks, particularly if there are no unresolved comments. I'm [obviously] not going through each and every resolved comment of every patch in a series. Those are *resolved*.
So while it was me who merged that series, and I'm of course sorry if that created problems, I acted in good faith (see above).
I think if anyone wants to hold back a merge for whatever reason, make sure you put a -1 vote in so it becomes impossible to be merged by accident. Or at the very least make sure there is an unresolved comment which will produce a warning when attempting to merge.
Before I get a chance to even glance, I see that all the patches are already merged! It is now basically too late to bring in my feedback, because the patches are in, and why spend more time on it. Suck it up, bygones.
It's not all lost: One can always send follow-up patches? Or revert the original patch and replace it? No release has been made in between, so no stable API has been set in stone that needs to continue to be supported.
Also I have (in my own free time) tried to get an improved logging timestamp into libosmocore. This is tagging along for years and years now, because every time over it is being blocked by minor details that are matters of taste. For years I lose precious moments every time i have to read an 'extended-timestamp' in osmo logging and identify the boundary of hours, minutes and seconds. This I consider important. AFAIR the reasons for blocking the patches were, objectively, all not important.
I always found it mildly fascinating how many options, choices and formats (I think primarily you) introduce to the libosmocore logging. Like file name in front or file at the end? I think osmo* is the only software with which I've worked where the actual *format* of the log has many user-configurable options. I'm not talking about sub-systems, levels and targets! Now we even have multiple different formats for timestamps, and we are growing more of them. In my opinion, most of this is bloat. It just adds complexity to things happening at the runtime of the osmo-* code for something that could just as well be obtained with log post-processing (like converting from a unix epoch timestamp to whatever human readable format the reader desires). We've already merged a lot of this over the years.
To me, the problem with all those many options is that it becomes very difficult to have any software parsing/analyzing osmo* logs, as every deployment might be chosing a different combination of "this is my personal favorite of ordering the portions of a log line" or "this is my personal favorite of formatting a timestamp".
But now introducing *timezones* into the program (https://gerrit.osmocom.org/c/libosmocore/+/32043) really goes over the top for me. Timezones are handled at the operating system level / libc.
Let's still give all the code review we can, but let's keep in mind the actual impact the particular code change has. If it goes in the "fringe detail" bucket, let's mention it, but just let it pass if the author disagrees.
To be honest, I don't really see much of this here. I see a lot of optional / non-critical stuff like "typo here" or "maybe ..." but I rarely see "I absolutely demand that you must ...".
Regards, Harald