[GitHub] [beam] echauchot edited a comment on pull request #13061: [BEAM-11050] Duplicate accumulator if it appears in multiple windows.

[GitBox <gi...@apache.org>]

echauchot edited a comment on pull request #13061:
URL: https://github.com/apache/beam/pull/13061#issuecomment-722451553


   @lukecwik I don't see why this change is necessary because of 2 reasons:
   1. all the validates runner tests including multiple window (eg. sliding windows) already passed.
   2. when I wrote this code, I already took some safety mesures about the modification of the (first only) accumulator during the `combineFn.mergeAccumulator` by creating a new first accumulator for each merged window see initial code below:
   
      // merge the accumulators for each mergedWindow
       ...
       for (Map.Entry<W, List<Tuple2<AccumT, Instant>>> entry :
           mergedWindowToAccumulators.entrySet()) {
          ...
         // we need to create the first accumulator because combineFn.mergerAccumulators can modify the
         // first accumulator
         AccumT first = combineFn.createAccumulator();
         Iterable<AccumT> accumulatorsToMerge =
             Iterables.concat(
                 Collections.singleton(first),
                 accumsAndInstantsForMergedWindow.stream()
                     .map(x -> x._1())
                     .collect(Collectors.toList()));
                  ...
                 combineFn.mergeAccumulators(accumulatorsToMerge),
                ...
     }
    


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org