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.

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

Bugfix/fix 0 retry times #981

Merged
merged 10 commits into from May 6, 2024

Conversation

VaderKai
Copy link
Contributor

@VaderKai VaderKai commented Apr 19, 2024

Context

#928

Change

Fix wireless retry when retry count is 0
Fix the total number of executions to 1+ retry times
if retry times is less than or equal to 0 is 0

Checklist

Before submitting this PR, please check the following points:

  • I have added unit and integration tests for my change
  • All unit and integration tests in the module I have added/changed are green
  • All unit and integration tests in the core and main modules are green
  • I have added/updated the documentation
  • I have added an example in the examples repo (only for "big" features)
  • I have added my new module in the BOM (only when a new module is added)

Checklist for adding new embedding store integration

  • I have added a {NameOfIntegration}EmbeddingStoreIT that extends from either EmbeddingStoreIT or EmbeddingStoreWithFilteringIT

@jmartisk
Copy link
Contributor

You're turning "attempts" into "retries" here I'd say. If it's called 'attempts', shouldn't 0 mean an error (IllegalArgumentException), and 1 to attempt exactly once?
That is, unless we rename the whole thing to "retries" in the code, but it's currently named "attempts".

@VaderKai
Copy link
Contributor Author

so we need do a choice,the utils name is RetryUtils,we can rename the method name or forbidan 0 attempts params,
i think rename is more reasonably,but changes is big,do you thinking about it?

@jmartisk
Copy link
Contributor

I would personally keep it as attempts, so 0 means error, and 1 means try exactly once. But it depends what @langchain4j thinks

@langchain4j langchain4j added the P2 High priority label Apr 23, 2024
@VaderKai
Copy link
Contributor Author

I would personally keep it as attempts, so 0 means error, and 1 means try exactly once. But it depends what @langchain4j thinks

I've changed the number of attempts to a minimum of 1, and 1 for illegal parameters (<=0).

Copy link
Owner

@langchain4j langchain4j left a comment

Choose a reason for hiding this comment

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

@VaderKai thank you!

@langchain4j langchain4j merged commit 31a86b5 into langchain4j:main May 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants