Closed jnievelt closed 7 years ago
Good catch. IIUC this can happen if one message id can result in multiple keys, is that the case you're referring to?
I think it just needs to be a partial flush on the AsyncSummer
. I forget offhand if any of the ones in algebird currently do this, but I should be able to start the PR with a test that fails because of this.
One thing to keep in mind are diamonds: due to caching and map-side combining, one output may correspond to more than one input. So, when that output is successful or fails, we need to fail/succeed all the tuple IDs to that point.
Then intent is to track that using standard algebird approaches and just merge the sequence of ids.
CC @ttim
The current logic is something like:
AsyncSummer
addAll
call is converted to the (keyIndex, (key, value)) formatOf course, if we have two colliding keys being summed, and an
AsyncSummer
implementation that doesn't do all-or-nothing flushes onaddAll
/etc., an emit of one key would include the message IDs for the aggregations on the other key as well.I think I missed this in review, writing it off as okay because we are doing synchronous handling on the
addAll
result. But actually we still need to track the message IDs along with the aggregated values, a laFinalFlatMap
andSummer
.I have the fix ready, originally thinking the change was only needed to do Future queuing, but I can split it out since they're really independent.