Skip to content

Conversation

@SaketaChalamchala
Copy link
Contributor

What changes were proposed in this pull request?

Retry policy when reading keys is hardcoded. Made the read retry max attempts and retry interval configurable in client configuration ozone.client.max.read.retries and ozone.client.max.read.retries

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-5865

How was this patch tested?

Existing UT and integration tests should run as is.

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

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

@SaketaChalamchala, Thanks for working on this.

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

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

@SaketaChalamchala Thanks for updating patch, LGTM +1.

@adoroszlai
Copy link
Contributor

@SaketaChalamchala I recommend resetting master branch in your fork from apache/ozone to avoid more and more Merge branch 'apache:master' into master commits in your PRs. This can be done in Github UI (Sync fork drop-down button in https://github.com/SaketaChalamchala/ozone/tree/master, choose Discard ... commits), or via CLI.

Comment on lines +111 to +113
retryPolicy =
HddsClientUtils.createRetryPolicy(config.getMaxReadRetryCount(),
TimeUnit.SECONDS.toMillis(config.getReadRetryInterval()));
Copy link
Contributor

@adoroszlai adoroszlai Mar 7, 2024

Choose a reason for hiding this comment

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

Storing this as static seems unsafe.

  1. Code is spread in 3 places:

    • this class
    • class that calls setRetryPolicy
    • class that uses the policy

    It is hard to see if any usage may get outdated or null value.

  2. Concurrent clients may get unexpected results.

@SaketaChalamchala
Copy link
Contributor Author

Thanks for the feedback @adoroszlai . Closing this PR and opened another one #6408 with updated changes. Please review those as well.

@SaketaChalamchala I recommend resetting master branch in your fork from apache/ozone to avoid more and more Merge branch 'apache:master' into master commits in your PRs. This can be done in Github UI (Sync fork drop-down button in https://github.com/SaketaChalamchala/ozone/tree/master, choose Discard ... commits), or via CLI.

Also, will do this from next time. Thanks

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.

3 participants