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]
