Skip to content
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

Make custom allocation deciders use pull based extensions #20040

Merged
merged 3 commits into from
Aug 18, 2016

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Aug 17, 2016

This change converts AllocationDecider registration from push based on
ClusterModule to implementing with a new ClusterPlugin interface.
AllocationDecider instances are allowed to use only Settings and
ClusterSettings.

This change converts AllocationDecider registration from push based on
ClusterModule to implementing with a new ClusterPlugin interface.
AllocationDecider instances are allowed to use only Settings and
ClusterSettings.
@rjernst rjernst added :Core/Infra/Plugins Plugin API and infrastructure v5.0.0-beta1 labels Aug 17, 2016
@rjernst rjernst changed the title Plugins: Make custom allocation deciders use pull based extensions Make custom allocation deciders use pull based extensions Aug 17, 2016
@@ -19,21 +19,20 @@

package org.elasticsearch.cluster.routing.allocation.decider;

import java.util.HashMap;
import java.util.Map;
Copy link
Member

Choose a reason for hiding this comment

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

Your IDE seems to be non-consistent about putting these places. In some files you put them above the ES imports, in others, below (see AllocationDeciders.java). I think we decided originally to be consistent with putting them below the ES imports?

Copy link
Member Author

Choose a reason for hiding this comment

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

My IDE is consistent, it just only moves when I optimize imports (usually to auto remove unused imports).

think we decided originally to be consistent with putting them below the ES imports?

Is this documented somewhere? I have intellij set to use the same order as eclipse defaults.

Copy link
Member

Choose a reason for hiding this comment

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

Is this documented somewhere? I have intellij set to use the same order as eclipse defaults.

I don't think it's documented anywhere, it's mostly been that way for a really long time (which isn't an excuse, we should document it). We always used to use Intellij's defaults, where were to put the java.* imports below ES imports, I think Eclipse's defaults are the other way around (the way this file is). It doesn't matter which way but it'd be nice to standardize only so we don't have them flipped around for every PR.

Personally I like the original Intellij style, but that's because I already have import-helpers configured to add them in that manner. It also means not switching them around for every PR since 90% of the codebase already uses this format.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #20062 for discussion

@dakrone
Copy link
Member

dakrone commented Aug 18, 2016

LGTM

@rjernst rjernst merged commit 165565a into elastic:master Aug 18, 2016
@rjernst rjernst deleted the pull_allocation_deciders branch August 18, 2016 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants