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
--
- Harald Welte <laforge(a)osmocom.org>
https://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)