Hi Piotr,
Thanks for your feedback!
> As I understand it, this effort is about replacing hardcoded
`AIMDRateLimitingStrategy` with something more flexible?
Yes __
> I have one main question about the design. Why are you
trying to split it into three different interfaces?
Can not we have a single public interface `RateLimitingStrategy`
You're right about the intention being to separate out the (what), (when) and
(how).
The intention here was to make the separation of concerns clearer, but I like
your idea to reduce the surface area in the interface.
We can change it such that AsyncSinkWriter passes a constructed batch of
messages to the RateLimitingStrategy.shouldBlock(), and that returns a boolean,
so the RateLimitingStrategy can decide the evaluation logic internally. (That
was my original idea too __ )
1. RateLimitingStrategy can update it's internal logic on `completeRequest `
`startRequest` methods
2. RateLimitingStrategy can provide a go/no-go decision on `shouldBlock`, given
a List of requests.
The above works, but we also have to also expose
"currentInFlightMessageCapacity" from the RateLimitingStrategy as well (since
it is important the batch of messages in the proposed request be constructed <
currentInFlightMessageCapacity), otherwise we will end up in a situation where
requests will never be sent.
An alternative is to give RateLimitingStrategy the power to construct the size
of the batches, I think that would be bloating the responsibility of the
RateLimitingStrategy a little too much. What do you think?
Regards,
Hong
On 23/06/2022, 10:05, "Piotr Nowojski" <pnowoj...@apache.org> wrote:
CAUTION: This email originated from outside of the organization. Do not
click links or open attachments unless you can confirm the sender and know the
content is safe.
Hi Hong,
As I understand it, this effort is about replacing hardcoded
`AIMDRateLimitingStrategy` with something more flexible? +1 for the general
idea.
If I read it correctly, there are basically three issues:
1. (what) `AIMDRateLimitingStrategy` is only able to limit the size of all
in-flight records across all batches, not the amount of in-flight batches.
2. (when) Currently `AsyncSinkWriter` decides whether and when to scale up
or down. You would like it to be customisable behaviour.
3. (how) The actual `scaleUp()` and `scaleDown()` behaviours are hardcoded,
and this could be customised as well.
Right? Assuming so, I have one main question about the design. Why are you
trying to split it into three different interfaces?
Can not we have a single public interface `RateLimitingStrategy` instead of
three that you proposed, that would have methods like:
`bool shouldRateLimit()` / `bool shouldBlock()`
`void startedRequest(RequestInfo requestInfo)`
`void completedRequest(RequestInfo requestInfo)`
where `RequestInfo` is a simple POJO similar to `CongestionControlInfo`
that you suggested
public class RequestInfo {
int failedMessages;
int batchSize;
long requestStartTime;
}
I think it would be more flexible and at the same time a simpler public
interface. Also we could provide the same builder that you proposed in
"Example configuring the Congestion Control Strategy using the new
interfaces",
Or am I missing something?
Best Piotrek
pon., 20 cze 2022 o 09:52 Teoh, Hong <lian...@amazon.co.uk.invalid>
napisał(a):
> Hi all,
>
> I would like to open a discussion on FLIP-242: Introduce configurable
> CongestionControlStrategy for Async Sink.
>
> The Async Sink was introduced as part of FLIP-171 [1], and implements a
> non-configurable congestion control strategy to reduce network congestion
> when the destination rejects or throttles requests. We want to make this
> configurable, to allow the sink implementer to decide the desired
> congestion control behaviour given a specific destination.
>
> This is a step towards reducing the barrier to entry to writing new async
> sinks in Flink!
>
> You can find more details in the FLIP-242 [2]. Looking forward to your
> feedback!
>
> [1]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-171%3A+Async+Sink
> [2]
>
https://cwiki.apache.org/confluence/display/FLINK/FLIP-242%3A+Introduce+configurable+CongestionControlStrategy+for+Async+Sink
>
>
> Regards,
> Hong
>
>
>
