Persist allocation ID with shard state metadata on nodes #14831
Conversation
| @@ -148,12 +156,52 @@ public String toString() { | |||
|
|
|||
| @Override | |||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | |||
| builder.startObject("allocation_id"); | |||
| builder.field("id", id); | |||
| builder.startObject(ALLOCATION_ID_OBJECT_KEY); | |||
There was a problem hiding this comment.
can we use the same static Fields we use in other places?
static final class Fields {
static final XContentBuilderString _INDEX = new XContentBuilderString("_index");
static final XContentBuilderString _TYPE = new XContentBuilderString("_type");
static final XContentBuilderString _ID = new XContentBuilderString("_id");
static final XContentBuilderString _VERSION = new XContentBuilderString("_version");
static final XContentBuilderString FOUND = new XContentBuilderString("found");
static final XContentBuilderString FIELDS = new XContentBuilderString("fields");
}
There was a problem hiding this comment.
oh can we please not add this useless class?
There was a problem hiding this comment.
This is what we do in all other classes. If you don't like it, fine, but can you say why so we'll stop doing it in other places?
|
left some minor comments. |
| } | ||
| builder.endObject(); | ||
| return builder; | ||
| } | ||
|
|
||
| public static AllocationId fromXContent(XContentParser parser) throws IOException { |
There was a problem hiding this comment.
can we use DocumentParser.java for this?
There was a problem hiding this comment.
not sure how DocumentParser helps here. Extra class Fields with XContentBuilderString does not help here as we match exact string names in fromXContent method (Fields is only used in cases where there is only toXContent). I implemented it in exactly the same way as it's done for ShardStateMetaData ...
There was a problem hiding this comment.
+1 on using static strings - I didn't realize that the XContentStrings things are not usable when parsing.
There was a problem hiding this comment.
this is a new method and should not copy old clumsy ways of implementing things. AFAIK DocumentParser is perfect for this and less error prone
There was a problem hiding this comment.
sorry I meant org.elasticsearch.common.xcontent.ObjectParser all the time my fault
|
pushed new changes. |
| @@ -100,6 +111,8 @@ public void toXContent(XContentBuilder builder, ShardStateMetaData shardStateMet | |||
| builder.field(VERSION_KEY, shardStateMetaData.version); | |||
| builder.field(PRIMARY_KEY, shardStateMetaData.primary); | |||
| builder.field(INDEX_UUID_KEY, shardStateMetaData.indexUUID); | |||
| builder.field(ALLOCATION_ID_KEY); | |||
| shardStateMetaData.allocationId.toXContent(builder, ToXContent.EMPTY_PARAMS); | |||
There was a problem hiding this comment.
why not use field(String name, ToXContent xContent)?
|
LGTM. Left one minor comment. |
ywelsch
force-pushed
the
feature/persist-allocid-shardstate
branch
from
8f62b87 to
fccad13
Compare
…tate Persist allocation ID with shard state metadata on nodes
…llocation id Such a write can happen when upgrading shard state metadata using the MultiDataPathUpgrader Relates to #14831
|
IMO this was merged too early. I left some more comments on the relevant places |
lcawl
added
:Distributed Indexing/Distributed
clintongormley
added
:Distributed Coordination/Allocation
Implements first step of #14739