Persist currently started allocation IDs to index metadata by ywelsch · Pull Request #14964 · elastic/elasticsearch

ywelsch · 2015-11-24T12:40:59Z

These allocation IDs serve as candidates to decide future primary shards

Subtask of #14739

s1monw · 2015-11-24T13:39:11Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java

I think we should make this unmodifiable here at least?

s1monw · 2015-11-24T13:42:30Z

left minor comments LGTM otherwise

bleskes · 2015-11-24T21:09:29Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java

can we call these "activeAllocationIds" ?

also we need those PER shard.. so the allocation ids for shard 0, alloc ids for shard 1 etc.

bleskes · 2015-11-24T21:50:16Z

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 · 2015-11-25T15:03:48Z

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 · 2015-11-26T12:50:50Z

core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java

I wonder if we should follow the OpenIntMap and call this StringMapDiff , just to key things shorter.

bleskes · 2015-11-30T10:15:20Z

core/src/test/java/org/elasticsearch/cluster/routing/allocation/ActiveAllocationIdTests.java

cool. Thanks!

bleskes · 2015-11-30T10:19:44Z

Nice one @ywelsch . Left some minor comments.

ywelsch · 2015-11-30T15:33:41Z

@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

bleskes · 2015-12-01T09:24:32Z

core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java

DiffablePrototypeValueReader -> DiffablePrototypeValue_Serializer_ for consistency

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

yeah, got it- just found it confusing.

bleskes · 2015-12-01T09:36:21Z

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 · 2015-12-01T12:13:46Z

pushed minor changes to address @bleskes's suggestions

check correctness of diffmap
randomize serialization

s1monw · 2015-12-02T09:15:56Z

core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java

can we have some javadocs what that is?

s1monw · 2015-12-02T10:55:06Z

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

s1monw · 2015-12-02T20:07:44Z

core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java

I think write and read would be enough as method names? the Diff & To part is implicit from the args?

Makes sense to throw the To and From part away. I want to keep writeDiff and readDiff though. For readDiff, it's necessary as it has the same method signature as read (except return type). To keep things symmetric for write and make it more verbose in the code of the MapDiff implementation, I'll use writeDiff.

s1monw · 2015-12-02T20:13:37Z

i left some cosmetic comments LGTM in general

s1monw · 2015-12-04T09:13:10Z

LGTM

- 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.

Closes elastic#14964

…etadata Persist currently started allocation IDs to index metadata

ywelsch added >enhancement review v5.0.0-alpha1 labels Nov 24, 2015

ywelsch assigned bleskes Nov 24, 2015

ywelsch force-pushed the feature/persist-allocid-indexmetadata branch from 94441d3 to 7d2b2ed Compare November 24, 2015 12:44

s1monw reviewed Nov 24, 2015
View reviewed changes

ywelsch mentioned this pull request Nov 24, 2015

Allocate primary shard based on allocation IDs #14739

Closed

7 tasks

bleskes reviewed Nov 24, 2015
View reviewed changes

bleskes reviewed Nov 26, 2015
View reviewed changes

bleskes reviewed Nov 30, 2015
View reviewed changes

core/src/test/java/org/elasticsearch/cluster/routing/allocation/ActiveAllocationIdTests.java

Copy link
Copy Markdown

Contributor

bleskes Nov 30, 2015

Choose a reason for hiding this comment

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

cool. Thanks!

bleskes reviewed Dec 1, 2015
View reviewed changes

s1monw reviewed Dec 2, 2015
View reviewed changes

core/src/main/java/org/elasticsearch/cluster/DiffableUtils.java Outdated

Copy link
Copy Markdown

Contributor

s1monw Dec 2, 2015

Choose a reason for hiding this comment

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

can we have some javadocs what that is?

s1monw reviewed Dec 2, 2015
View reviewed changes

ywelsch force-pushed the feature/persist-allocid-indexmetadata branch from 7aab1a5 to 601a119 Compare December 3, 2015 09:30

Yannick Welsch added 2 commits December 4, 2015 11:24

Persist currently started allocation IDs to index metadata

fef043a

Closes elastic#14964

ywelsch force-pushed the feature/persist-allocid-indexmetadata branch from 601a119 to fef043a Compare December 4, 2015 10:32

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

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

0c2c7e7

…etadata Persist currently started allocation IDs to index metadata

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

lcawl added :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. and removed :Allocation labels Feb 13, 2018

clintongormley added :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) and removed :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. labels Feb 14, 2018

Conversation

ywelsch commented Nov 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw commented Nov 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bleskes commented Nov 24, 2015

Uh oh!

ywelsch commented Nov 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bleskes commented Nov 30, 2015

Uh oh!

ywelsch commented Nov 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bleskes commented Dec 1, 2015

Uh oh!

ywelsch commented Dec 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw commented Dec 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw commented Dec 2, 2015

Uh oh!

s1monw commented Dec 4, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants