On 30/08/2021 20:27, Mark Michelson wrote: > On 8/30/21 6:14 AM, Mark Gray wrote: >> On 27/08/2021 18:56, Mark Michelson wrote: >>> Hi Mark, >>> >>> I had a look at this series, but I'm not 100% sure what the intent is. >>> In patch 2, you mentioned modularity and the ability to include northd >>> as a library. But I'm not sure where that allows us to go. Can you >>> elaborate a bit? >> >> Thanks for the review Mark! >> >> I think that describing my future intentions may, perhaps, be confusing >> matters. Regardless of those intentions, I think that splitting out this >> code makes sense in order simplify the code. ovn-northd.c is >15k lines >> of code and any steps we can take to slowly break this down into >> separate files should help make the code more manageable. Also, I am not >> functionally changing anything? Therefore, I think that this series can >> stand on its own. >> >> However, I also think my commit message could be misinterpreted as I am >> not trying to make a northd "library" in the lib.a sense. I am exploring >> the possibility of integrating I-P into northd and, rather than adding >> the I-P code directly into ovn-northd.c, I am trying to add it in a way >> that should be slightly less invasive and hopefully a bit easier to >> manage. My basic idea is to organize the code so that each I-P node >> consists of a corresponding .c and .h file (with all the associated >> benefits - testing, etc). I would then run the IP framework from >> ovn-northd.c. This would create dependencies something like this. >> >> ovn-northd.c ────► inc-proc.c/.h ───┬───► node1.c/.h >> │ >> ├───► node2.c/.h >> │ >> └───► node3.c/.h >> >> My first goal is to implement all of northd.c into one node and then >> try to split out some elements of it into other nodes. I am trying to do >> all this in order to incrementally(!) add I-P to northd rather than just >> try to rewrite the whole thing in one fell swoop. Regardless of these >> plans, which may amount to nothing, I think this series is independent. >> However, if you don't like the idea, maybe I could try to add all the >> I-P code directly into ovn-northd.c - i just think it will be messier. > > Thanks for the explanation, Mark. For the record, I usually am in favor > of breaking large source files into smaller, more manageable, more > testable units, and this is no exception. > > I'm fine with the change, but I wanted to double-check about the intent > here, because
Thanks for that. If the approach is generally good, then I can, at least, proceed with my changes locally and then do a rebase before merge. > 1) It wasn't 100% clear to me just from the code changes themselves > 2) I was trying to figure out if this is something that we should try to > get into OVN 21.09.> > For 1) your explanation here has made things more clear. > > For 2) I'll need to know what else you have up your sleeve. For > instance, if you have follow-up changes ready to go, then this may be a > candidate for 21.09. If, however, follow-up changes are not ready and > they will take weeks to write, then I think even with an ACK this change > should wait until 21.12. That way, it and additional followups can all > be part of 21.12. I'll see how I progress this week before soft-freeze. If I can make soft-freeze, then we can consider it. Thanks again. > >> >>> >>> Thanks, >>> Mark >>> >>> On 8/26/21 3:04 PM, Mark Gray wrote: >>>> Please note that this commit mainly involves moving code around with >>>> minimal >>>> code changes. However, due to tight coupling between ovn-northd.c and >>>> northd.c, >>>> some minor changes were needed. For reference, and to help reviews, please >>>> examine the following at a minimum: >>>> >>>> * Configuration of the probe interval in northd.c >>>> (ovsdb_idl_set_probe_interval()) >>>> * Passing of "use_parallel_build" and "lflow_locks" from ovn-northd.c and >>>> northd.c. >>>> * Update of "struct northd_context": additopn of fields and move to h >>>> file.>> >>>> The commits were (hopefully) structured in a way to make the review >>>> easier. As >>>> this change touches all of ovn-northd, any change to "master" will make a >>>> rebase >>>> necessary and probably difficult. Therefore, if the general ideas is OK, >>>> then >>>> it would be great if this series could be expedited to prevent many >>>> rebases! >>>> >>>> Thanks >>>> >>>> Mark Gray (2): >>>> ovn-northd: Rename ovn-northd.c to northd.c >>>> northd: Split northd.c >>>> >>>> Documentation/tutorials/ovn-openstack.rst | 154 +- >>>> northd/automake.mk | 2 + >>>> northd/lrouter.dl | 2 +- >>>> northd/northd.c | 14418 +++++++++++++++++++ >>>> northd/northd.h | 42 + >>>> northd/ovn-northd.c | 14717 +------------------- >>>> northd/ovn.rs | 2 +- >>>> northd/ovn_northd.dl | 2 +- >>>> tests/ovn-northd.at | 2 +- >>>> 9 files changed, 14709 insertions(+), 14632 deletions(-) >>>> create mode 100644 northd/northd.c >>>> create mode 100644 northd/northd.h >>>> >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
