-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Update exception messages #135017
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
Update exception messages #135017
Conversation
Modifies the Javadoc descriptions of FailedToCommitClusterStateException and NotMasterException to better distinguish between the two
Modifies the Javadoc descriptions of FailedToCommitClusterStateException and NotMasterException to better distinguish between the two
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
server/src/main/java/org/elasticsearch/cluster/NotMasterException.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/NotMasterException.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/NotMasterException.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/elasticsearch/cluster/coordination/FailedToCommitClusterStateException.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/elasticsearch/cluster/coordination/FailedToCommitClusterStateException.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/NotMasterException.java
Outdated
Show resolved
Hide resolved
Applying suggested changed from David Co-authored-by: David Turner <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you've addressed all of David's review comments, I just have the one suggestion about a possible overwrite.
* | ||
* This is different from {@link NotMasterException} where we know for certain that a state update never took effect. | ||
* | ||
* This exception is retryable within {@link FailedToCommitClusterStateException}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this was overwritten in baaf76a, I think you wanted TransportNodeMasterAction
?
* This exception is retryable within {@link FailedToCommitClusterStateException}. | |
* This exception is retryable within {@link TransportNodeMasterAction}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, thanks! I'll have to push a new revision since it needs to be imported. Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Modifies the Javadoc descriptions of
FailedToCommitClusterStateException
and
NotMasterException
to better distinguish between the twoPrerequisite to #134213