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.
>
