Log action can distinguish data from flush

[antiguru]

Previously, the log action would receive an empty container when we'd like
to report progress but no new data. We'd only send the empty container when
we didn't have any other data to send. This makes it hard for users to
distinguish between flushing and just data updates. It might be important
to distinguish the two because only on flush we might not be called again
for a while, but otherwise it's very likely that the logger might receive
more data, or sees a flush.

This change alters the signature of the action to accept a &mut Option<C>
(where C is CB::Container), and we pass Some(container) on data, and
None on flush.

Clients using the logging API need to change their implementation, as both
vectors and Option offer a iter function, but with different results.

Signed-off-by: Moritz Hoffmann [email protected]

[frankmcsherry]

This all looks good to me. I left a few comments, but they are roughly nits, and can be followed on later if we conclude they are worth it!

[frankmcsherry]

No strong opinion, but is this clearing new behavior? Does it e.g. prevent passing back owned data, into which the logger can write? Again, really no strong opinion, but just checking whether the force-clear is new, and whether it is intentional.

[antiguru]

It was purely defensive. Thinking about it, it should be the container builder's responsibility to enforce what needs to be true about a container after extracting or finishing it, so it doesn't make sense to have the clear call here.

[frankmcsherry]

A nit, but one of the reasons to take a &mut Option<_> rather than a Option<&mut _> is to allow the None call to pass back resources, and at least with Push the intent is that you keep calling this as long as you get a non-None back. We don't have to do that here, and probably massively over-thinking this, but wanted to call out the gap.

[antiguru]

I see your point, but I'm wondering if this is actually true! Or put differently, is it something we'd like to be true, or is it something that we had at some point and forgot about?

The reason I'm asking is because most (all?) places where we call Push::done, we don't have a loop to drain resources. If I recall correctly, the only place where we loop is in Differential merge batchers to drain the stash of allocations once we're done merging chains.

We could change Push's done function to look like this instead:

    fn done(&mut self) { 
        let mut container = None;
        loop {
            self.push(&mut container);
            if container.is_none() { break; }
        }
    }