> On Apr 11, 2015, at 4:58 PM, Russell Bryant <rbry...@redhat.com> wrote:
>
> On 04/11/2015 02:59 AM, Justin Pettit wrote:
>>
>>
>> There's a trade-off here between creating too many transactions and
>> too few. I was leaning towards more transactions, since they can be
>> annotated through comments which can aid debugging. The argument
>> against it is that it creates too many transactions that must be
>> waded through and a lot of blocking. My thinking on this is changing
>> towards creating fewer transactions, but my preference is to get it
>> in as it is now, and then reassess later.
>
> I want to make sure I understand this. Are you saying that by not
> integrating committing transactions back into the main loop, you end up
> with more transactions? Further, you suggest we may want to rework
> this, but you'd prefer to just get it in as-of now?
>
> That's all fine with me. I just want to make sure I'm following.
>
> I'll try to rephrase my original concern another way. In this code we
> have 2 open connections. I was thinking that it would be ideal that any
> time we ever end up in poll(), we're actually polling on both of them.
> By committing the transaction inside of bindings_run() or chassis_run()
> like this, we end up doing a poll() on one connection
> (txn_commit_block() -> poll_block()). Input could have arrived on the
> other while we're sleeping and we won't wake up to handle it.
>
> I guess I don't expect it to cause any real problems in practice,
> though. I was thinking of it more in terms of an ideal pattern to be
> following.
My thinking was that to do the commits in the main loop, all modifications to a
particular database would occur in a single transaction. The way it is now we
can comment on Chassis and Bindings changes separately. In v1 of this patch, I
actually broke each Bindings update for a logical port into a separate
transaction, but Ben thought it might be too heavyweight at scale. At this
point, we're probably not providing enough extra information by separating
Chassis and Bindings into separate transactions to justify the overhead, but I
just wanted to think about it before restructuring the code, since the current
design does make it a bit easier to provide additional information if that's
the right direction. I want to talk with some members of our controller team
to understand what they've found useful when debugging OVSDB at scale in
customer environments.
>>>> + struct unixctl_server *unixctl;
>>>> + struct controller_ctx ctx;
>>>
>>> Instead of the memset() further below, I tend to prefer something like:
>>>
>>> struct controller_ctx ctx = {
>>> .chassis_name = NULL,
>>> };
>>
>> I don't have a strong preference, so I went with your suggestion.
>
> OK, it's really just a habit for me, and I'm not even sure it's
> justified. I have some vague memory of thinking it might provide a
> better opportunity to optimize out needing to set memory to 0. From a
> style perspective, it keeps the initialization with the declaration,
> which is kind of nice.
>
> Anyway, I hope the minor style things like this aren't too annoying. I
> can do better to keep them to myself if so. :-)
I don't find it annoying at all. I'm actually interested in hearing what Ben
has to say about initialization, since I saw you provided similar feedback on
one of his patches.
>> I'll send out v3 in a minute. There's an incremental at the end of this
>> message.
>>
>> Thanks for the thorough review!
>
> Thanks again for being welcoming of my participation!
Good feedback is always appreciated and encouraged.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
