Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Repeat specification for repeatWhen #3593

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Repeat specification for repeatWhen #3593

wants to merge 1 commit into from

Conversation

Desislav-Petrov
Copy link

@Desislav-Petrov Desislav-Petrov commented Oct 1, 2023 •
edited
Loading

hi @chemicL and @OlegDokuka - kicked off some work for #3545 but wanted to verify the approach before i go any further.

So my plan is as follows.

  1. Work out a base approach to get the simple repeat test pass
  2. Add necessary functionality for the rest of the tests to pass
  3. Refactor and pull the common bits between repeat/retry as previously suggested
  4. See if anything else is missing in the repeat functionality and add it
@Desislav-Petrov Desislav-Petrov requested a review from a team as a code owner October 1, 2023 18:46
@pivotal-cla
Copy link

@Desislav-Petrov Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.
1 similar comment
@pivotal-cla
Copy link

@Desislav-Petrov Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.
@pivotal-cla
Copy link

@Desislav-Petrov Thank you for signing the Contributor License Agreement!
Desislav-Petrov
Desislav-Petrov commented Oct 1, 2023
View reviewed changes
reactor-core/src/main/java/reactor/util/repeat/RepeatSpec.java
.concatMap(retryWhenState -> {
RepeatSignal copy = retryWhenState.copy();
long iteration = copy.getRepeatsSoFar();
if (iteration > repeats) {
Copy link
Author

Choose a reason for hiding this comment

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

I don't think this is the most graceful way to do this, was trying to get some inspiration from the Retry one but the case is slightly different there
@chemicL chemicL changed the title Establishing approach Jul 17, 2024
@chemicL
Copy link
Member

chemicL commented Jul 17, 2024

Hey, @Desislav-Petrov. I'm trying to go through some PRs that were not given enough attention. Please accept the team's apologies. I had a brief look and would like to know whether after not gaining much attention from us you are still willing to make progress on this.

My first concern is a public API change - we need to make sure that anything that we add does not remove existing functionality.

Also, the next(-1) signal looks suspicious - in case of differences between the implementation in reactor-extras, can you list them with a high-level explanation?

Let me know, thanks.
@chemicL chemicL added the status/need-user-input This needs user input to proceed label Jul 17, 2024
@chemicL chemicL added this to the 3.6.x Backlog milestone Jul 17, 2024
@chemicL chemicL self-assigned this Jul 17, 2024
@Desislav-Petrov
Copy link
Author

Hey @chemicL - I'm still keen on working on that as long as I get some support/gudaince - let me take a look since it's been a while to summarise the idea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/need-user-input This needs user input to proceed
3 participants
@Desislav-Petrov @pivotal-cla @chemicL