On Mon, Jun 9, 2014 at 2:36 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Sun, Jun 01, 2014 at 10:04:29PM -0700, Alex Wang wrote:
> > This commit adds logic that checks the return value of status_txn
> > transaction and runs the update again if the transaction fails.
> >
> > Signed-off-by: Alex Wang <al...@nicira.com>
>
> I am not sure that you understand what TXN_INCOMPLETE means. It means
> that a transaction has been queued up to send to the database server,
> but the database server has not yet replied. Some of the comments in
> this commit imply that we should retry the transaction in the case of
> TXN_INCOMPLETE, but no, that's not the case; we should just wait for
> the database server to respond.
>
> It seems to me that status_txn_try_again is only used after the
> transaction finishes, since if status == TXN_INCOMPLETE then we're
> still waiting for it to complete and status_txn is still nonnull. So
> I would update this comment to remove the mention of "incomplete":
>
Thanks for correcting my understanding. I forgot to follow the
instant_stats_wait() logic when removing it. I'll adjust the code
accordingly.
> > + * If the previous database transaction was incomplete or failed (is not
> > + * 'TXN_SUCCESS' or 'TXN_UNCHANGED'), 'status_txn_try_again' is set to
> true,
> > + * which will cause the main thread wake up soon and retry the status
> update.
> > */
> > static struct ovsdb_idl_txn *status_txn;
> > +static bool status_txn_try_again;
>
> Also, here we really only care to set status_txn_try_again if the
> transaction is somehow complete, right? Then I would move the new
> code inside the previous "if" block that only runs if status !=
> TXN_INCOMPLETE. Also, TXN_UNCOMMITTED should be impossible here,
> since we called ovsdb_idl_txn_commit() a few lines earlier.
>
I'll do that.
> > @@ -2453,6 +2458,14 @@ bridge_run(void)
> > ovsdb_idl_txn_destroy(status_txn);
> > status_txn = NULL;
> > }
> > +
> > + /* Sets the 'status_txn_try_again' if the transaction fails or
> > + * is still incomplete. */
> > + if (status == TXN_SUCCESS || status == TXN_UNCOMMITTED) {
> > + status_txn_try_again = false;
> > + } else {
> > + status_txn_try_again = true;
> > + }
> > }
> >
> > run_system_stats();
>
> There seems to be some similar confusion here. If the transaction is
> incomplete, then we shouldn't do any kind of timer-based waiting, we
> should instead wait for the transaction to complete:
>
I'll fix it.
> > @@ -2486,11 +2499,11 @@ bridge_wait(void)
> > poll_timer_wait_until(stats_timer);
> > }
> >
> > - /* If the status database transaction is 'TXN_INCOMPLETE' in this
> run,
> > - * register a timeout in 'STATUS_CHECK_AGAIN_MSEC'. Else, wait on
> the
> > - * global connectivity sequence number. Note, this also helps batch
> > - * multiple status changes into one transaction. */
> > - if (status_txn) {
> > + /* If the status update to database needs to be run again
> (transaction
> > + * fails or incomplete), registers a timeout in
> 'STATUS_CHECK_AGAIN_MSEC'.
> > + * Else, waits on the global connectivity sequence number. Note,
> this also
> > + * helps batch multiple status changes into one transaction. */
> > + if (status_txn_try_again) {
> > poll_timer_wait_until(time_msec() + STATUS_CHECK_AGAIN_MSEC);
> > } else {
> > seq_wait(connectivity_seq_get(), connectivity_seqno);
> > --
> > 1.7.9.5
> >
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
