HDDS-5847. Make OM Retry Policy configurable for different protocols#2964
HDDS-5847. Make OM Retry Policy configurable for different protocols#2964hanishakoneru wants to merge 1 commit intoapache:masterfrom
Conversation
hanishakoneru
force-pushed
the
HDDS-5847
branch
from
a6fd262 to
8a7e21b
Compare
hanishakoneru
force-pushed
the
HDDS-5847
branch
from
8a7e21b to
f6e7641
Compare
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @hanishakoneru for working on this, and sorry for forgetting to review.
Some trivial code improvements can be made in OMRetryPolicy, but otherwise looks good. Especially OMFailoverProxyProvider looks much cleaner now.
One question: what is the reason for adding ExceptionType-based mapping in addition to the exception class-based one?
| private final HashMap<ExceptionType, RetryAction> defaultRetryRulesMap; | ||
| private HashMap<ExceptionType, RetryAction> retryRulesMapByType; | ||
| private HashMap<Class, RetryAction> retryRulesMapByClass; |
There was a problem hiding this comment.
I think we could improve it in the following ways:
- defaults do not need to be instance-specific
- variables can be declared using
Mapinterface - specify type parameter for
Class - variables can be final
| private final HashMap<ExceptionType, RetryAction> defaultRetryRulesMap; | |
| private HashMap<ExceptionType, RetryAction> retryRulesMapByType; | |
| private HashMap<Class, RetryAction> retryRulesMapByClass; | |
| private static final Map<ExceptionType, RetryAction> DEFAULT_RETRY_RULES; | |
| private final Map<ExceptionType, RetryAction> retryRulesByType; | |
| private final Map<Class<? extends Exception>, RetryAction> retryRulesByClass; |
(Needs corresponding changes where these are used.)
| this.defaultRetryRulesMap = new HashMap<>(); | ||
| this.retryRulesMapByType = new HashMap<>(); |
There was a problem hiding this comment.
These two can be created as EnumMap.
| this.defaultRetryRulesMap.put(ExceptionType.NOT_LEADER, | ||
| RetryAction.FAILOVER_AND_RETRY); | ||
| this.defaultRetryRulesMap.put(ExceptionType.LEADER_NOT_READY, | ||
| RetryAction.RETRY_ON_SAME_OM); |
There was a problem hiding this comment.
If variable changed to static, these can be in static {} block.
| if (retryRulesMapByClass == null) { | ||
| retryRulesMapByClass = new HashMap<>(); | ||
| } |
There was a problem hiding this comment.
Unnecessary, already created in constructor.
| /** | ||
| * Add a Retry rule based on Exception Class. | ||
| */ | ||
| public void addRetryRule(Class exceptionClass, |
There was a problem hiding this comment.
| public void addRetryRule(Class exceptionClass, | |
| public void addRetryRule(Class<? extends Exception> exceptionClass, |
| } | ||
|
|
||
| /** | ||
| * Get the ExceptioType for the given Exception. |
There was a problem hiding this comment.
| * Get the ExceptioType for the given Exception. | |
| * Get the ExceptionType for the given Exception. |
|
|
||
| public enum ExceptionType { |
There was a problem hiding this comment.
Checkstyle insists on javadoc for these enums.
|
@hanishakoneru I think this is a useful change, so I would like to wrap this up. If you agree, I would like to apply the improvements I had suggested (and resolve merge conflict). Then we could either merge it, or ask someone else for another round of review. |
kerneltime
requested review from
fapifta and
kerneltime
and removed request for
bharatviswa504 and
fapifta
|
@kerneltime , Thanks for bringing up this older PR. General comment: it needs to be rebased and updated for the refactored |
|
/pending |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@kerneltime , this PR needs some attention to resolve conflicts with the master. |
|
@neils-dev I think one will have to adopt this Jira and work on the fix. Let me know if you want to take that up. I am closing this issue for now as even if works is continued on this Jira it would make sense to start a new PR. |
4 participants
What changes were proposed in this pull request?
Currently we reuse the retry policy in OMFailoverProxyProvider formulated for OM client communication for the Inter Service Protocol and the OM Admin protocol (HDDS-5490). This does not take into account the retry policy required when a ReconfigurationInProgressException is encountered while bootstrapping or decommissioning an OM (please refer to Bharat Viswanadham's comment here). It would be good to separate out the retry policy based on the protocol.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5847
How was this patch tested?
Existing unit tests