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

Persist currently started allocation IDs to index metadata #14964

Merged
merged 2 commits into from Dec 4, 2015

Conversation

Projects
None yet
5 participants
@ywelsch
Contributor

ywelsch commented Nov 24, 2015

These allocation IDs serve as candidates to decide future primary shards

Subtask of #14739

@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Nov 24, 2015

Contributor

left minor comments LGTM otherwise

Contributor

s1monw commented Nov 24, 2015

left minor comments LGTM otherwise

@ywelsch ywelsch referenced this pull request Nov 24, 2015

Closed

Allocate primary shard based on allocation IDs #14739

7 of 7 tasks complete
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated ...java/org/elasticsearch/cluster/routing/allocation/AllocationService.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Nov 24, 2015

Member

This looks good. I left some minor suggestion. Note the comment about storing allocation ids per shard in the meta data. I know the allocation Ids are unique so in theory we can throw them in one big pile, but I think it will be clearer in terms of code and API later on to have them separated.

Member

bleskes commented Nov 24, 2015

This looks good. I left some minor suggestion. Note the comment about storing allocation ids per shard in the meta data. I know the allocation Ids are unique so in theory we can throw them in one big pile, but I think it will be clearer in terms of code and API later on to have them separated.

@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Nov 25, 2015

Contributor

Pushed a larger set of changes, please have another look @bleskes and @s1monw. I store the allocation ids now on a per shard id basis and introduced a new class that helps with the serialization / cluster diffs.

Contributor

ywelsch commented Nov 25, 2015

Pushed a larger set of changes, please have another look @bleskes and @s1monw. I store the allocation ids now on a per shard id basis and introduced a new class that helps with the serialization / cluster diffs.

@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/AllocationIds.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@@ -450,7 +473,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
private final Settings settings;
private final Diff<ImmutableOpenMap<String, MappingMetaData>> mappings;
private final Diff<ImmutableOpenMap<String, AliasMetaData>> aliases;
private Diff<ImmutableOpenMap<String, Custom>> customs;

This comment has been minimized.

@bleskes

bleskes Nov 26, 2015

Member

good catch

@bleskes

bleskes Nov 26, 2015

Member

good catch

@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated ...rg/elasticsearch/cluster/routing/allocation/ActiveAllocationIdTests.java
@bleskes

View changes

Show outdated Hide outdated ...rg/elasticsearch/cluster/routing/allocation/ActiveAllocationIdTests.java
@bleskes

View changes

Show outdated Hide outdated ...rg/elasticsearch/cluster/routing/allocation/ActiveAllocationIdTests.java
@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Nov 26, 2015

Member

thx @ywelsch . This looks great. Left some comments here and there. One thing I think is forgotten (because it's not evident at all) is adding the allocation ids to the toXContent serialization of the cluster state.

Member

bleskes commented Nov 26, 2015

thx @ywelsch . This looks great. Left some comments here and there. One thing I think is forgotten (because it's not evident at all) is adding the allocation ids to the toXContent serialization of the cluster state.

@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Nov 27, 2015

Contributor

Pushed another set of changes. In IndexMetaData, the activeAllocationIds are now stored as ImmutableOpenIntMap<Set<String>>. To enable serialization for maps that have non-diffable values, I heavily refactored DiffableUtils. I also introduced the possibility to specify how key values of map should be serialized. For activeAllocationIds (map from shard id to set of allocation id), we can use VInt-based serialization of keys (as we know that shard ids are small positive numbers).

Contributor

ywelsch commented Nov 27, 2015

Pushed another set of changes. In IndexMetaData, the activeAllocationIds are now stored as ImmutableOpenIntMap<Set<String>>. To enable serialization for maps that have non-diffable values, I heavily refactored DiffableUtils. I also introduced the possibility to specify how key values of map should be serialized. For activeAllocationIds (map from shard id to set of allocation id), we can use VInt-based serialization of keys (as we know that shard ids are small positive numbers).

@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
protected final Map<String, T> adds;
protected final List<K> deletes;
protected final Map<K, Diff<T>> diffs; // incremental updates
protected final Map<K, T> upserts; // additions or full updates

This comment has been minimized.

@bleskes

bleskes Nov 30, 2015

Member

this is a great way of handling the non-diffable case. nice one.

@bleskes

bleskes Nov 30, 2015

Member

this is a great way of handling the non-diffable case. nice one.

@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
@bleskes

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java
clusterState = ClusterState.builder(clusterState).routingResult(rerouteResult).build();
// active allocation ids should not be updated
assertThat(clusterState.getRoutingTable().shardsWithState(UNASSIGNED).size(), equalTo(3));

This comment has been minimized.

@bleskes

bleskes Nov 30, 2015

Member

cool. Thanks!

@bleskes

bleskes Nov 30, 2015

Member

cool. Thanks!

@bleskes

View changes

Show outdated Hide outdated ...src/test/java/org/elasticsearch/cluster/serialization/DiffableTests.java
@bleskes

View changes

Show outdated Hide outdated ...src/test/java/org/elasticsearch/cluster/serialization/DiffableTests.java
@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Nov 30, 2015

Member

Nice one @ywelsch . Left some minor comments.

Member

bleskes commented Nov 30, 2015

Nice one @ywelsch . Left some minor comments.

@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Nov 30, 2015

Contributor

@bleskes Another set of changes:

  • Reworked DiffableTests so that it now parameterizes over the following:
    • map implementation type (JDK Map, ImmutableOpenMap, ImmutableOpenIntMap)
    • map value type (Diffable or non-diffable)
    • specific values that are added / removed. We now compare randomly-generated sets.
  • Moved value extraction from IndexMetaData constructor to its builder
  • supportsDiffableValues() is now applied after equals check
  • removed unnecessary builder methods
Contributor

ywelsch commented Nov 30, 2015

@bleskes Another set of changes:

  • Reworked DiffableTests so that it now parameterizes over the following:
    • map implementation type (JDK Map, ImmutableOpenMap, ImmutableOpenIntMap)
    • map value type (Diffable or non-diffable)
    • specific values that are added / removed. We now compare randomly-generated sets.
  • Moved value extraction from IndexMetaData constructor to its builder
  • supportsDiffableValues() is now applied after equals check
  • removed unnecessary builder methods
*
* Note: this implementation is ignoring the key.
*/
public static class DiffablePrototypeValueReader<K, V extends Diffable<V>> extends DiffableValueSerializer<K, V> {

This comment has been minimized.

@bleskes

bleskes Dec 1, 2015

Member

DiffablePrototypeValueReader -> DiffablePrototypeValue_Serializer_ for consistency

@bleskes

bleskes Dec 1, 2015

Member

DiffablePrototypeValueReader -> DiffablePrototypeValue_Serializer_ for consistency

This comment has been minimized.

@ywelsch

ywelsch Dec 1, 2015

Contributor

The prototype is only used for the "reading" part, hence the name.

@ywelsch

ywelsch Dec 1, 2015

Contributor

The prototype is only used for the "reading" part, hence the name.

This comment has been minimized.

@bleskes

bleskes Dec 1, 2015

Member

yeah, got it- just found it confusing.

@bleskes

bleskes Dec 1, 2015

Member

yeah, got it- just found it confusing.

@bleskes

View changes

Show outdated Hide outdated ...src/test/java/org/elasticsearch/cluster/serialization/DiffableTests.java
@bleskes

View changes

Show outdated Hide outdated ...src/test/java/org/elasticsearch/cluster/serialization/DiffableTests.java
@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Dec 1, 2015

Member

LGTM. Left some minor suggestions (first on the commit - sorry for the noise). I would love it if @s1monw or @imotov will also look at the changes to the diff infra as the were involved in it as well.

Member

bleskes commented Dec 1, 2015

LGTM. Left some minor suggestions (first on the commit - sorry for the noise). I would love it if @s1monw or @imotov will also look at the changes to the diff infra as the were involved in it as well.

@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Dec 1, 2015

Contributor

pushed minor changes to address @bleskes's suggestions

  • check correctness of diffmap
  • randomize serialization
Contributor

ywelsch commented Dec 1, 2015

pushed minor changes to address @bleskes's suggestions

  • check correctness of diffmap
  • randomize serialization
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
public static final IntKeySerializer INSTANCE = new IntKeySerializer();
@Override
public void writeKey(Integer key, StreamOutput out) throws IOException {

This comment has been minimized.

@s1monw

s1monw Dec 2, 2015

Contributor

can we throw an exception if the key is negative?

@s1monw

s1monw Dec 2, 2015

Contributor

can we throw an exception if the key is negative?

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Dec 2, 2015

Contributor

I will look again this afternoon... sorry for the delay

Contributor

s1monw commented Dec 2, 2015

I will look again this afternoon... sorry for the delay

@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java
@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Dec 2, 2015

Contributor

i left some cosmetic comments LGTM in general

Contributor

s1monw commented Dec 2, 2015

i left some cosmetic comments LGTM in general

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Dec 4, 2015

Contributor

LGTM

Contributor

s1monw commented Dec 4, 2015

LGTM

ywelsch added some commits Dec 4, 2015

Add capabilities for serializing map-based cluster state diffs
- Supports ImmutableOpenIntMap besides java.util.Map and ImmutableOpenMap
- Map keys can be any value (not only String)
- Map values do not have to implement Diffable interface. In that case custom value serializer needs to be provided.

ywelsch added a commit that referenced this pull request Dec 4, 2015

Merge pull request #14964 from ywelsch/feature/persist-allocid-indexm…
…etadata

Persist currently started allocation IDs to index metadata

@ywelsch ywelsch merged commit 0c2c7e7 into elastic:master Dec 4, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment