-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add listener mechanism for failures to send shard failed #14295
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
Conversation
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.
I'm wondering if the ReplicaPhase shouldn't implement the ShardStateActionListener interface it self, which will make things simpler and save on the param.
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.
For now I exposed it as a parameter to hook into for the testing that I added in TransportReplicationActionTests. However this might not be necessary and we can go with the approach that you suggest depending on how we respond to your comment regarding moving the initial testing out of TransportReplicationActionTests. I'll return to this after addressing the other item.
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.
I've changed the tests in 00310d112427b9f8fae67fc7fba05473f4aa966b and removed the field in a02571dbb8b6421c59e547857dbbb3d169697e66.
|
Great start. Left some comments. |
|
I've pushed commits responding to your comments. Thanks for the initial review @bleskes. |
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.
would it make sense to make these methods default impl empty body?
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.
@s1monw Added in 0c20e8f6eafc63c53e4981de82eae3bd129c4538.
|
LGTM. Thx @jasontedor |
This commit adds a listener mechanism for executing callbacks when exceptional situations occur sending a shard failure message to the master. The two types of exceptional situations that can occur are if the master is not known and if the transport request exception handler is invoked for any reason after sending the shard failed request to the master. This commit only adds the infrastructure for executing callbacks when one of these exceptional situations occur; no effort is made to properly handle the exceptional situations. Some unit tests are added for ShardStateAction to test that the listener infrastructure is correct. Relates #14252
Add listener mechanism for failures to send shard failed
|
Rebased on master, squashed, and merged into master. Thanks for reviewing @bleskes. |
This commit adds a listener mechanism for executing callbacks when
exceptional situations occur sending a shard failure message to the
master. The two types of exceptional situations that can occur are if
the master is not known and if the transport request exception handler
is invoked for any reason after sending the shard failed request to the
master. This commit only adds the infrastructure for executing
callbacks when one of these exceptional situations occur; no effort is
made to properly handle the exceptional situations. Some unit tests are
added for ShardStateAction to test that the listener infrastructure is
correct.
Relates #14252