On Mon, Apr 04, 2016 at 05:28:36AM -0700, 'Viktor Bachraty' via ganeti-devel
wrote:
> On Monday, April 4, 2016 at 11:50:43 AM UTC+1, Brian Foley wrote:
>
> On Mon, Apr 04, 2016 at 11:13:21AM +0100, 'Viktor Bachraty' via
> ganeti-devel wrote:
> [snip]
> > diff --git a/lib/jqueue/__init__.py b/lib/jqueue/__init__.py
> > index 2b90d00..7714d0d 100644
> > --- a/lib/jqueue/__init__.py
> > +++ b/lib/jqueue/__init__.py
> > @@ -621,20 +621,44 @@ class _OpExecCallbacks(mcpu.OpExecCbBase):
> > logging.debug("Opcode will be retried. Back to waiting.")
> >
> > @locking.ssynchronized(_QUEUE, shared=1)
> > - def _AppendFeedback(self, timestamp, log_type, log_msg):
> > + def _AppendFeedback(self, timestamp, log_type, log_msgs):
> > """Internal feedback append function, with locks
> >
> > - """
> > - self._job.log_serial += 1
> > - self._op.log.append((self._job.log_serial, timestamp,
> log_type, log_msg))
> > + @type timestamp: tuple (int, int)
> > + @param timestamp: timestamp of the log message
> Nit: I assume this is seconds from epoch. Also are you sure it's
> integer, not
> float?
>
> Yes, the float value returned by time.time() is converted by
> utils.SplitTime() into a pair of integers.
>
> > + for msg in log_msgs:
> > + self._job.log_serial += 1
> > + self._op.log.append((self._job.log_serial, timestamp,
> log_type, msg))
> > self._queue.UpdateJobUnlocked(self._job, replicate=False)
> Nit: Do we really need to increment the serial number per message,
> or could
> we just do it once overall? I know what you've got preserves the
> current
> behaviour, but from luxid's point of view, isn't it only going to
> care that
> the new serial > old serial?
>
> If you notice in the loop, each log message in the list has it's own
> log serial. In some other place the list is iterated over to find the
> maximum serial value (as if the code assumes it doesn't have to be
> always monotonically growing?) So this made me think it would be safer
> and less likely to break other things if I keep it unchanged.
>
> [snip]
> > + self._feedback_fn = self.Feedback
> > self.bad = False
> >
> > + # TODO: Cleanup calling conventions, make them explicit
> > + def Feedback(self, *args):
> > +
> > + # TODO: Remove this once calling conventions are explicit.
> > + # Feedback can be called with anything, we interpret
> ELogMessageList as
> > + # messages that have to be individually added to the log
> list, but pushed
> > + # in a single update. Other types are only transparently
> passed forward.
> > + if len(args) == 1:
> > + log_type = constants.ELOG_MESSAGE
> > + log_msg = args[0]
> > + else:
> > + log_type, log_msg = args
> > +
> > + if log_type != constants.ELOG_MESSAGE_LIST:
> > + log_msg = [log_msg]
> Any reason not to make this if log_type == constants.ELOG_MESSAGE: ?
>
>
> As discussed IRL, the log_type could theoretically have other values as
> well (see ELogType in src/Ganeti/Types.hs)
>
> > +
> > + self.msglist.extend(log_msg)
>
> [snip]
> > + # Feedback can be called with anything, we interpret
> ELogMessageList as
> > + # messages that have to be individually added to the log
> list, but pushed
> > + # in a single update. Other types are only transparently
> passed forward.
> > + if log_type == constants.ELOG_MESSAGE_LIST:
> > + log_msg_list = [(constants.ELOG_MESSAGE, msg) for msg in
> log_msg]
> > + else:
> > + log_msg_list = [(log_type, log_msg)]
> As above.
LGTM, and thanks.
