Skip to content

Conversation

@OlegDokuka
Copy link
Member

In this PR I want to introduce rate-limited request coordinator which basically prevents direct Long.MAX_VALUE propagation.

I'm considering moving this part to "addons" and instrument RSocket via plugin API.

For now, leave it in this for pre-reviewing

Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
@OlegDokuka OlegDokuka requested a review from robertroeser July 29, 2019 10:00
@OlegDokuka OlegDokuka force-pushed the feature/rate-limiter-publisher branch from 0c3bfe2 to 7b822a9 Compare July 29, 2019 10:02
@OlegDokuka OlegDokuka requested a review from yschimke July 29, 2019 10:04
replacement for coordinated request publisher

Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
@OlegDokuka OlegDokuka force-pushed the feature/rate-limiter-publisher branch from 7b822a9 to 3b25e68 Compare July 29, 2019 10:14
@yschimke
Copy link
Member

I thought this was already directly possible based on optional Spring Reactor operations? I hate the infinite then cancel default mode of Reactor, but why the need to implement yourself?

@OlegDokuka
Copy link
Member Author

OlegDokuka commented Jul 29, 2019

@yschimke I consider that as a replacement for LimitableRequestPublisher which does internaRequesN(Long.MAX_VALUE) and then do coordination based on the external request size. In case external consumer requests Long.MAX_VALUE it can lead to -> #514. This is still an issue. Previously, I was not able to end up with efficient enough and properly tunned request coordination algorithm, and do not think it makes any sense (see Flux#flatMap request coordination between producers. You may find that it is based on the prefetch and subsequent additional requests over the time). Thus, my idea is to get rid of LimitableRequestPublisher, so it will not bring any additional synchronization overhead during runtime, and in case of need, provide Plugin with that functionality. Note, default Reactor#limitRate is too overhead. It creates SPSCArrayQueue (in fact there are no reasons to do that) and then introduces tasks rescheduling over Schedulers.immediate, so it leads to function invocation overhead

@yschimke
Copy link
Member

Thanks for the explanation. Is this worth opening an issue with reactor as well?

@OlegDokuka
Copy link
Member Author

I think, I'm going to port it eventually, but it worth, indeed

public void request(long n) {
synchronized (this) {
long requested = externalRequested;
if (requested == Long.MAX_VALUE) {
Copy link
Member

@yschimke yschimke Jul 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to match the logic on line 56. Integer vs Long MAX_VALUE. It's hard to intuit whether that's intentional because there are multiple fields. Maybe some comments to clarify what max values are used.

Copy link
Member

@robertroeser robertroeser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTGM - I'd like to merge this in to start testing if its ready to go.

@robertroeser robertroeser marked this pull request as ready for review August 5, 2019 19:13
@robertroeser robertroeser merged commit 88ee909 into develop Aug 5, 2019
@robertroeser robertroeser deleted the feature/rate-limiter-publisher branch August 5, 2019 19:14
mostroverkhov added a commit that referenced this pull request Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants