-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Add BWC layer to seq no infra and enable BWC tests #22185
Changes from 14 commits
a9d297d
adabd89
4cacc8b
c2cb646
02b223c
2db7210
634d92f
4ea5dd9
26596ff
001f7de
cc9a486
f2ca825
a945030
f4dada9
059638b
2a15b15
647d5ee
76338bf
896482f
f73d165
2cc2610
bc60297
2ecbac0
7577b12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,5 @@ html_docs | |
|
||
# random old stuff that we should look at the necessity of... | ||
/tmp/ | ||
backwards/ | ||
eclipse-build | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,6 @@ public void writeTo(StreamOutput out) throws IOException { | |
|
||
@Override | ||
public String toString() { | ||
return "flush {" + super.toString() + "}"; | ||
return "flush {" + shardId + "}"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,12 +19,14 @@ | |
|
||
package org.elasticsearch.action.support.replication; | ||
|
||
import org.elasticsearch.Version; | ||
import org.elasticsearch.action.bulk.BulkShardRequest; | ||
import org.elasticsearch.action.delete.DeleteRequest; | ||
import org.elasticsearch.action.index.IndexRequest; | ||
import org.elasticsearch.action.support.WriteRequest; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.index.seqno.SequenceNumbersService; | ||
import org.elasticsearch.index.shard.ShardId; | ||
|
||
import java.io.IOException; | ||
|
@@ -36,6 +38,9 @@ | |
public abstract class ReplicatedWriteRequest<R extends ReplicatedWriteRequest<R>> extends ReplicationRequest<R> implements WriteRequest<R> { | ||
private RefreshPolicy refreshPolicy = RefreshPolicy.NONE; | ||
|
||
long seqNo; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extra whitespaces? can this be private or protected? |
||
|
||
/** | ||
* Constructor for deserialization. | ||
*/ | ||
|
@@ -62,11 +67,32 @@ public RefreshPolicy getRefreshPolicy() { | |
public void readFrom(StreamInput in) throws IOException { | ||
super.readFrom(in); | ||
refreshPolicy = RefreshPolicy.readFrom(in); | ||
if (in.getVersion().onOrAfter(Version.V_6_0_0_alpha1_UNRELEASED)) { | ||
seqNo = in.readVLong(); | ||
} else { | ||
seqNo = SequenceNumbersService.UNASSIGNED_SEQ_NO; | ||
} | ||
} | ||
|
||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
super.writeTo(out); | ||
refreshPolicy.writeTo(out); | ||
if (out.getVersion().onOrAfter(Version.V_6_0_0_alpha1_UNRELEASED)) { | ||
out.writeVLong(seqNo); | ||
} | ||
} | ||
|
||
/** | ||
* Returns the sequence number for this operation. The sequence number is assigned while the operation | ||
* is performed on the primary shard. | ||
*/ | ||
public long seqNo() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. get/set prefix if possible :) |
||
return seqNo; | ||
} | ||
|
||
/** sets the sequence number for this operation. should only be called on the primary shard */ | ||
public void seqNo(long seqNo) { | ||
this.seqNo = seqNo; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -283,7 +283,7 @@ protected List<ShardRouting> getShards(ShardId shardId, ClusterState state) { | |
} | ||
|
||
private void decPendingAndFinishIfNeeded() { | ||
assert pendingActions.get() > 0; | ||
assert pendingActions.get() > 0 : "pending action count goes bellow 0 for request [" + request + "]"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/bellow/below There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. thx. |
||
if (pendingActions.decrementAndGet() == 0) { | ||
finish(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
|
||
import org.apache.logging.log4j.message.ParameterizedMessage; | ||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.Version; | ||
import org.elasticsearch.action.ActionListener; | ||
import org.elasticsearch.action.ActionListenerResponseHandler; | ||
import org.elasticsearch.action.ActionResponse; | ||
|
@@ -983,16 +984,26 @@ public ReplicaResponse(String allocationId, long localCheckpoint) { | |
|
||
@Override | ||
public void readFrom(StreamInput in) throws IOException { | ||
super.readFrom(in); | ||
localCheckpoint = in.readZLong(); | ||
allocationId = in.readString(); | ||
if (in.getVersion().onOrAfter(Version.V_6_0_0_alpha1_UNRELEASED)) { | ||
super.readFrom(in); | ||
localCheckpoint = in.readZLong(); | ||
allocationId = in.readString(); | ||
} else { | ||
// we use to read empty responses | ||
Empty.INSTANCE.readFrom(in); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just remove this? it's doing nothing so a comment is fine? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure |
||
} | ||
} | ||
|
||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
super.writeTo(out); | ||
out.writeZLong(localCheckpoint); | ||
out.writeString(allocationId); | ||
if (out.getVersion().onOrAfter(Version.V_6_0_0_alpha1_UNRELEASED)) { | ||
super.writeTo(out); | ||
out.writeZLong(localCheckpoint); | ||
out.writeString(allocationId); | ||
} else { | ||
// we use to write empty responses | ||
Empty.INSTANCE.writeTo(out); | ||
} | ||
} | ||
|
||
@Override | ||
|
@@ -1016,10 +1027,9 @@ public void performOn(ShardRouting replica, ReplicaRequest request, ActionListen | |
listener.onFailure(new NoNodeAvailableException("unknown node [" + nodeId + "]")); | ||
return; | ||
} | ||
transportService.sendRequest(node, transportReplicaAction, | ||
new ConcreteShardRequest<>(request, replica.allocationId().getId()), transportOptions, | ||
// Eclipse can't handle when this is <> so we specify the type here. | ||
new ActionListenerResponseHandler<ReplicaResponse>(listener, ReplicaResponse::new)); | ||
final ConcreteShardRequest<ReplicaRequest> concreteShardRequest = | ||
new ConcreteShardRequest<>(request, replica.allocationId().getId()); | ||
sendReplicaRequest(concreteShardRequest, node, listener); | ||
} | ||
|
||
@Override | ||
|
@@ -1060,6 +1070,14 @@ public void onFailure(Exception shardFailedError) { | |
} | ||
} | ||
|
||
/** sends the give replica request to the supplied nodes */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/give/given There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check |
||
protected void sendReplicaRequest(ConcreteShardRequest<ReplicaRequest> concreteShardRequest, DiscoveryNode node, | ||
ActionListener<ReplicationOperation.ReplicaResponse> listener) { | ||
transportService.sendRequest(node, transportReplicaAction, concreteShardRequest, transportOptions, | ||
// Eclipse can't handle when this is <> so we specify the type here. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this comment still valid? I see that you removed the type.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch. @jpountz maybe you can help verify eclipse is happy with this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indeed it is not happy about it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thx @jpountz, I'll revert the change |
||
new ActionListenerResponseHandler<>(listener, ReplicaResponse::new)); | ||
} | ||
|
||
/** a wrapper class to encapsulate a request when being sent to a specific allocation id **/ | ||
public static final class ConcreteShardRequest<R extends TransportRequest> extends TransportRequest { | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,7 +204,7 @@ public void writeTo(StreamOutput out) throws IOException { | |
// timestamp | ||
out.writeBoolean(false); // enabled | ||
out.writeString(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format()); | ||
out.writeOptionalString(null); | ||
out.writeOptionalString("now"); // old default | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/old/5.x/ |
||
out.writeOptionalBoolean(null); | ||
} | ||
out.writeBoolean(hasParentField()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? this is where our bwc index creation python tool stores their versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... maybe we need a different solution then - the problem is that with this line the this file was ignored by git.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call the folder bwc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s1monw I pushed another commit updating gitignore to be more explicit about the backwords folder (rather than using a global glob pattern). I also updated the comments around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++