dannycranmer commented on pull request #18880:
URL: https://github.com/apache/flink/pull/18880#issuecomment-1048800635


   > I do not want to be the devil's advocate but this PR looks like a new 
feature for the AsyncSink that would require a FLIP and we are passed the 
feature freeze so we can very likely only merge it to 1.16.
   
   @fapaul we were treating this change as a bug, however I agree the new 
interface definitions are edging into new feature territory. During our testing 
we have found that when the output streams are underprovisioned, saturating the 
destination stream results in a high number of records throttled. This results 
in a high retry count and consumes un-necessary CPU/network. See graph below, 
`aimd8`/`aimd50` contain this fix and `async8`/`async50` do not. You can see 
that the `aimd` implementation has slightly better throughput and significantly 
less throttling.
   
   <img width="1440" alt="Screenshot 2022-02-23 at 09 59 45" 
src="https://user-images.githubusercontent.com/4950503/155303236-7c0be0b8-816f-424b-b2ea-7d510eca2766.png";>
   
   In response to your feedback, in order to remediate the issues observed in 
KDF/KDS sinks I propose we remove the rate limiting strategy change from the 
Async Sink base, and move to the concrete implementation. This would leave the 
Async Sink Base public interface untouched. We can then follow up with a FLIP 
to define how the rate limiting interfaces will work and pull into the Async 
Sink base. In order to facilitate this change we would need to extract 2 
methods in AsyncSinkWriter that can be overridden to introduce the limiting.


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

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to