Skip to content

Refactor ShardFailure listener infrastructure #14206

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

Merged
merged 2 commits into from
Oct 21, 2015

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Oct 20, 2015

Today we leak the notion of an engine outside of the shard abstraction
which is not desirable. This commit refactors the infrastrucutre to use
use already existing interfaces to communicate if a shard has failed and
prevents engine private classes to be implemented on a higher level.
This change is purely cosmentical...

Today we leak the notion of an engine outside of the shard abstraction
which is not desirable. This commit refactors the infrastrucutre to use
use already existing interfaces to communicate if a shard has failed and
prevents engine private classes to be implemented on a higher level.
This change is purely cosmentical...
@@ -167,7 +166,7 @@
private final MeanMetric refreshMetric = new MeanMetric();
private final MeanMetric flushMetric = new MeanMetric();

private final ShardEngineFailListener failedEngineListener = new ShardEngineFailListener();
private final ShardEngineFailListener engineEventListener = new ShardEngineFailListener();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since we're moving away from the word fail in the name, can we rename ShardEngineFailListener to ShardEngineEventListener

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the shard end was not intended to be generic... I left that in here on purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah wrong I misunderstood... fixing

@bleskes
Copy link
Contributor

bleskes commented Oct 20, 2015

LGTM. Left one minor comment (next to trivial stuff) about null protection in IndicesClusterStateService

@s1monw
Copy link
Contributor Author

s1monw commented Oct 21, 2015

pushed a new commit adressing all comments and I also improved the situation if indexShard is null

public static final class ShardFailure {
public final ShardRouting routing;
public final String reason;
@Nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks.

@bleskes
Copy link
Contributor

bleskes commented Oct 21, 2015

LGTM. Thx @s1monw

s1monw added a commit that referenced this pull request Oct 21, 2015
Refactor ShardFailure listener infrastructure
@s1monw s1monw merged commit 3ba4dfa into elastic:master Oct 21, 2015
@s1monw s1monw deleted the refactor_shard_failure_listener branch October 21, 2015 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants