Hello Fujii-san,
On Wed, 28 Jul 2021 00:20:21 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
> On 2021/07/27 11:02, Yugo NAGATA wrote:
> > Hello Fujii-san,
> >
> > Thank you for looking at it.
> >
> > On Tue, 27 Jul 2021 03:04:35 +0900
> > Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> + * Per-thread last disconnection time is not measured because it
> + * is already done when the transaction successfully finished.
> + * Also, we don't need it when the thread is aborted because we
> + * can't report complete results anyway in such cases.
>
> What about commenting a bit more explicitly like the following?
>
> --------------------------------------------
> In CSTATE_FINISHED state, this disconnect_all() is no-op under -C/--connect because all the connections that this
threadestablished should have already been closed at the end of transactions. So we don't need to measure the
disconnectiondelays here.
>
> In CSTATE_ABORTED state, the measurement is no longer necessary because we cannot report complete results anyways in
thiscase.
> --------------------------------------------
Thank you for the suggestion. I updated the comment.
> >
> >> - /* no connection delay to record */
> >> - thread->conn_duration = 0;
> >> + /* connection delay is measured globally between the barriers */
> >>
> >> This comment is really correct? I was thinking that the measurement is not necessary here because this is the case
where-C option is not specified.
> >
> > This comment means that, when -C is not specified, the connection delay is
> > measured between the barrier point where the benchmark starts
> >
> > /* READY */
> > THREAD_BARRIER_WAIT(&barrier);
> >
> > and the barrier point where all the thread finish making initial connections.
> >
> > /* GO */
> > THREAD_BARRIER_WAIT(&barrier);
>
> Ok, so you're commenting about the initial connection delay that's
> measured when -C is not specified. But I'm not sure if this comment
> here is really helpful. Seem rather confusing??
Ok. I removed this comment.
> I found another disconnect_all().
>
> /* XXX should this be connection time? */
> disconnect_all(state, nclients);
>
> The measurement is also not necessary here.
> So the above comment should be removed or updated?
I think this disconnect_all will be a no-op because all connections should
be already closed in threadRun(), but I left it just to be sure that
connections are all cleaned-up. I updated the comment for explaining above.
I attached the updated patch. Could you please look at this?
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
