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

Backport reindex to 2.x #17060

Merged
merged 6 commits into from Mar 14, 2016

Conversation

Projects
None yet
5 participants
@nik9000
Contributor

nik9000 commented Mar 10, 2016

Reindex is two related APIs:

  • _reindex copies documents from one index to another.
  • _update_by_query updates documents in one index.

nik9000 added some commits Mar 1, 2016

Reindex modules
Creates _reindex and _update_by_query APIs which can be used to copy data
from one index to another (_reindex) and to update documents in its current
index (_update_by_query). These APIs are bundled into the reindex module.

This is a backport of
#16861
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Mar 10, 2016

Contributor

Unlike @imotov's lovely backport of task management (#16959) I didn't break out the commits that adapted this from master to 2.x. Sorry! I'll try to call out the interesting bits to review.

Contributor

nik9000 commented Mar 10, 2016

Unlike @imotov's lovely backport of task management (#16959) I didn't break out the commits that adapted this from master to 2.x. Sorry! I'll try to call out the interesting bits to review.

@@ -306,6 +306,7 @@
<include>org/elasticsearch/node/NodeMocksPlugin.class</include>
<include>org/elasticsearch/node/MockNode.class</include>
<include>org/elasticsearch/common/io/PathUtilsForTesting.class</include>
<include>org/elasticsearch/rest/NoOpClient.class</include>

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

In master I moved NoOpClient into the test framework so it could be shared by other tests. In 2.x there isn't a test framework to move it to so I just exported it in the test jar. That is kind of the test framework anyway.

@nik9000

nik9000 Mar 10, 2016

Contributor

In master I moved NoOpClient into the test framework so it could be shared by other tests. In 2.x there isn't a test framework to move it to so I just exported it in the test jar. That is kind of the test framework anyway.

@@ -40,7 +44,14 @@
/**
* Represents a failure.
*/
public static class Failure {
public static class Failure implements Writeable<Failure>, ToXContent {

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

I use this failure in reindex directly so I have it implement Writeable and ToXContent for easy reuse. This change should be backwards compatible.

@nik9000

nik9000 Mar 10, 2016

Contributor

I use this failure in reindex directly so I have it implement Writeable and ToXContent for easy reuse. This change should be backwards compatible.

@@ -668,7 +675,11 @@ public void process(MetaData metaData, @Nullable MappingMetaData mappingMd, bool
@Override
public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
type = in.readString();
if (in.getVersion().before(Version.V_2_3_0)) {

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

I believe this to be the one API change that needs backwards compatibility in this whole proposal. The reindex API itself is entirely new so it will fail to run at all if it is sent to an old node version. A 2.3 versioned node should be able to coordinate a reindex for any 2.x versioned cluster though I haven't tested it.

@nik9000

nik9000 Mar 10, 2016

Contributor

I believe this to be the one API change that needs backwards compatibility in this whole proposal. The reindex API itself is entirely new so it will fail to run at all if it is sent to an old node version. A 2.3 versioned node should be able to coordinate a reindex for any 2.x versioned cluster though I haven't tested it.

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

can we add some validation that it's really not null?

@s1monw

s1monw Mar 13, 2016

Contributor

can we add some validation that it's really not null?

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

I'm not sure what you mean. Like in the validate method? It has that already.

@nik9000

nik9000 Mar 13, 2016

Contributor

I'm not sure what you mean. Like in the validate method? It has that already.

@@ -89,6 +99,28 @@ public void onFailure(Throwable e) {
return task;
}
public final Task execute(Request request, final TaskListener<Response> listener) {

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Reindex needs this so it can log something when a wait_for_completion=false reindex request comes in. This is exactly how it works in master.

@nik9000

nik9000 Mar 10, 2016

Contributor

Reindex needs this so it can log something when a wait_for_completion=false reindex request comes in. This is exactly how it works in master.

public static SearchRequest parseSearchRequest(RestRequest request, ParseFieldMatcher parseFieldMatcher) {
String[] indices = Strings.splitStringByCommaToArray(request.param("index"));
SearchRequest searchRequest = new SearchRequest(indices);
public static void parseSearchRequest(SearchRequest searchRequest, RestRequest request, ParseFieldMatcher parseFieldMatcher,

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

This diverges a bit from how master works because most of the parsing can't be done on the coordinating node. So this change is smaller than master. Here we just change this API so that it can modify a SearchRequest rather than create one. This lets reindex create a SearchRequest, change the defaults on it to be reindex's defaults, and then pass the search request through this API so that REST parameters still work in the same way. Mostly. Reindex does quite a bit of hacking on the body to transform bits of it's API into one that this search request likes. We'll get to that later.

@nik9000

nik9000 Mar 10, 2016

Contributor

This diverges a bit from how master works because most of the parsing can't be done on the coordinating node. So this change is smaller than master. Here we just change this API so that it can modify a SearchRequest rather than create one. This lets reindex create a SearchRequest, change the defaults on it to be reindex's defaults, and then pass the search request through this API so that REST parameters still work in the same way. Mostly. Reindex does quite a bit of hacking on the body to transform bits of it's API into one that this search request likes. We'll get to that later.

@@ -45,6 +45,10 @@ public final RestResponse buildResponse(Response response, XContentBuilder build
builder.startObject();
response.toXContent(builder, channel.request());
builder.endObject();
return new BytesRestResponse(RestStatus.OK, builder);
return new BytesRestResponse(getStatus(response), builder);

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

This seemed like a simple place to implement request status. I'm frankly surprised no one did this before. Anyway, this is exactly how it works in master.

@nik9000

nik9000 Mar 10, 2016

Contributor

This seemed like a simple place to implement request status. I'm frankly surprised no one did this before. Anyway, this is exactly how it works in master.

[[docs-reindex]]
==== Reindex API
`_reindex`'s most basic form just copies documents from one index to another.

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Both asciidoc files in the PR are exactly as they were in #16861. They will get better when we merge the rest of the PRs that have been merged to master into 2.x. We'll go over them one last time before release (#16618) so maybe we should just skip these?

@nik9000

nik9000 Mar 10, 2016

Contributor

Both asciidoc files in the PR are exactly as they were in #16861. They will get better when we merge the rest of the PRs that have been merged to master into 2.x. We'll go over them one last time before release (#16618) so maybe we should just skip these?

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

I guess it's fine to have them here we can still catch up with #16618 once it's in?

@s1monw

s1monw Mar 13, 2016

Contributor

I guess it's fine to have them here we can still catch up with #16618 once it's in?

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

I have a bunch of other PRs to backport for reindex that will help this a lot. I don't expect those PRs to have their own backport PRs because I expect those backports to be trivial.

#16618, I think, will help even more. But right now the branches are so out of sync these docs are just here to make the branches closer.

@nik9000

nik9000 Mar 13, 2016

Contributor

I have a bunch of other PRs to backport for reindex that will help this a lot. I don't expect those PRs to have their own backport PRs because I expect those backports to be trivial.

#16618, I think, will help even more. But right now the branches are so out of sync these docs are just here to make the branches closer.

@@ -247,11 +247,15 @@
<include>api/indices.analyze.json</include>
<include>api/indices.create.json</include>
<include>api/indices.refresh.json</include>
<include>api/indices.put_mapping.json</include>

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

These weird incantations are required to allow plugins to use certain APIs in REST tests. This wasn't something that we particularly liked so we fixed it in master a long time ago.

@nik9000

nik9000 Mar 10, 2016

Contributor

These weird incantations are required to allow plugins to use certain APIs in REST tests. This wasn't something that we particularly liked so we fixed it in master a long time ago.

Response extends BulkIndexByScrollResponse>
extends AbstractAsyncBulkByScrollAction<Request, Response> {
private final ScriptService scriptService;

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Same as last class - all changes are mechanical.

@nik9000

nik9000 Mar 10, 2016

Contributor

Same as last class - all changes are mechanical.

* task can't totally validate until it starts but this is better than
* nothing.
*/
ActionRequestValidationException validationException = internalRequest.validate();

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Here I had to drop the "offset doesn't work in this context" validation because I'd have to parse the search source to get it and that doesn't seem worth it.

@nik9000

nik9000 Mar 10, 2016

Contributor

Here I had to drop the "offset doesn't work in this context" validation because I'd have to parse the search source to get it and that doesn't seem worth it.

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

No. Not here. Sorry. Was confused. This class is roughly the same as was merged to master.

@nik9000

nik9000 Mar 10, 2016

Contributor

No. Not here. Sorry. Was confused. This class is roughly the same as was merged to master.

@Override
public ActionRequestValidationException validate() {
ActionRequestValidationException e = searchRequest.validate();
if (maxRetries < 0) {

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

here is where I dropped the offset validation.

@nik9000

nik9000 Mar 10, 2016

Contributor

here is where I dropped the offset validation.

searchRequest.source(DEFAULT_SOURCE);
}
try {
Map<String, Object> newSource = XContentHelper.convertToMap(DEFAULT_SOURCE, true).v2();

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

OK! This is one of the places where reindex has to expand the source of the search request so it can set it up. In this case we layer the defaults "under" the search source as sent by the user. This method is called right before we start processing the request on the coordinating node.

@nik9000

nik9000 Mar 10, 2016

Contributor

OK! This is one of the places where reindex has to expand the source of the search request so it can set it up. In this case we layer the defaults "under" the search source as sent by the user. This method is called right before we start processing the request on the coordinating node.

@Override
protected Request beforeExecute(Request request) {
// In 2.x the search's "source" isn't built until
request.getSearchRequest().source(source.internalBuilder());

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Just like all the request builders that interact with search requests in 2.x, this builder must build the source explicitly or the request is totally empty and everything fails. This is done both here and above, in the request method, because that is how SearchRequestBuilder does it. It works though I'm not totally sure why we have to places. I don't lose sleep over it because it is a 2.x hack.

@nik9000

nik9000 Mar 10, 2016

Contributor

Just like all the request builders that interact with search requests in 2.x, this builder must build the source explicitly or the request is totally empty and everything fails. This is done both here and above, in the request method, because that is how SearchRequestBuilder does it. It works though I'm not totally sure why we have to places. I don't lose sleep over it because it is a 2.x hack.

/**
* Task storing information about a currently running BulkByScroll request.
*/
public class BulkByScrollTask extends CancellableTask {

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Surprisingly (or not) the task didn't need any touch ups to go to 2.x. After @imotov's wonderful backport of task management it just worked.

@nik9000

nik9000 Mar 10, 2016

Contributor

Surprisingly (or not) the task didn't need any touch ups to go to 2.x. After @imotov's wonderful backport of task management it just worked.

}
@Override
protected RestStatus getStatus(R response) {

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Remember that work I did to RestToXContentListener to allow you to override getStatus 12 linear feat of code above this? Here is the payoff.

@nik9000

nik9000 Mar 10, 2016

Contributor

Remember that work I did to RestToXContentListener to allow you to override getStatus 12 linear feat of code above this? Here is the payoff.

}
public void onModule(ActionModule actionModule) {
actionModule.registerAction(ReindexAction.INSTANCE, TransportReindexAction.class);

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

These methods are different than their brothers on the master branch but not in important ways. They just declare the actions in the normal way for 2.x.

@nik9000

nik9000 Mar 10, 2016

Contributor

These methods are different than their brothers on the master branch but not in important ways. They just declare the actions in the normal way for 2.x.

}
private void parseRequest(XContentParser parser, ReindexRequest request) throws IOException {
String currentFieldName = null;

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

In master this is implemented using a shiny ObjectParser and it is like 20 lines of declarative configuration. In 2.x it is 90 lines of switch statements!

@nik9000

nik9000 Mar 10, 2016

Contributor

In master this is implemented using a shiny ObjectParser and it is like 20 lines of declarative configuration. In 2.x it is 90 lines of switch statements!

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

oh boy I can totally see that going bad 💃

@s1monw

s1monw Mar 13, 2016

Contributor

oh boy I can totally see that going bad 💃

* it to set its own defaults which differ from SearchRequest's
* defaults. Then the parse can override them.
*/
UpdateByQueryRequest internalRequest = new UpdateByQueryRequest(new SearchRequest());

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

The implementation of this whole class is different in 2.x than it is in master. Almost totally different. Well, in both branches this class must do the whole "blow the source into a map and change it and then turn it back into source so we can throw it through some internal API" trick, but in master it is very sorry to have to be so nasty and expects that it won't need that hack when the whole search request has an ObjectParser that is can just use. In 2.x object parsers aren't a thing so we are much more shameless about manipulating the request as a map. We have to do much more manipulation because of our inability to get at the parsed guts of the request. All this is in service of keeping the REST api exactly the same in master and 2.x.

@nik9000

nik9000 Mar 10, 2016

Contributor

The implementation of this whole class is different in 2.x than it is in master. Almost totally different. Well, in both branches this class must do the whole "blow the source into a map and change it and then turn it back into source so we can throw it through some internal API" trick, but in master it is very sorry to have to be so nasty and expects that it won't need that hack when the whole search request has an ObjectParser that is can just use. In 2.x object parsers aren't a thing so we are much more shameless about manipulating the request as a map. We have to do much more manipulation because of our inability to get at the parsed guts of the request. All this is in service of keeping the REST api exactly the same in master and 2.x.

@Inject
public TransportReindexAction(Settings settings, ThreadPool threadPool, ActionFilters actionFilters,
IndexNameExpressionResolver indexNameExpressionResolver, ClusterService clusterService, ScriptService scriptService,

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

I don't recall this needing anything other than mechanical cleanups.

@nik9000

nik9000 Mar 10, 2016

Contributor

I don't recall this needing anything other than mechanical cleanups.

private final ScriptService scriptService;
@Inject
public TransportUpdateByQueryAction(Settings settings, ThreadPool threadPool, ActionFilters actionFilters,

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Same here. It needed new parameters to @Inject but that isn't a big deal. Also needed more type hints to make 1.7 happy, but not big deal there either.

@nik9000

nik9000 Mar 10, 2016

Contributor

Same here. It needed new parameters to @Inject but that isn't a big deal. Also needed more type hints to make 1.7 happy, but not big deal there either.

public void testScriptAddingJunkToCtxIsError() {
try {
applyScript(new Consumer<Map<String, Object>>() {

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Like Task Management's backports I opted to directly port the functional style used in master even if it is super bloaty in java 1.7.

@nik9000

nik9000 Mar 10, 2016

Contributor

Like Task Management's backports I opted to directly port the functional style used in master even if it is super bloaty in java 1.7.

public void testStatusHatesNegatives() {
try {
new BulkByScrollTask.Status(-1, 0, 0, 0, 0, 0, 0, 0, null);

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Each one of these is 1 lines in java 8 😢

@nik9000

nik9000 Mar 10, 2016

Contributor

Each one of these is 1 lines in java 8 😢

@@ -0,0 +1,366 @@
---
"Response format for created":

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

From here on out it is just miles of REST tests, exactly copied from master. The only thing missing is validation that you don't use "from".

@nik9000

nik9000 Mar 10, 2016

Contributor

From here on out it is just miles of REST tests, exactly copied from master. The only thing missing is validation that you don't use "from".

It then uses rest tests to test reindex and groovy together!
-->
<artifactId>smoke-test-reindex-with-groovy</artifactId>

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

Oh! Not just rest tests. This is like 10 lines in master with gradle. In 2.x in is a copy and paste fest.

@nik9000

nik9000 Mar 10, 2016

Contributor

Oh! Not just rest tests. This is like 10 lines in master with gradle. In 2.x in is a copy and paste fest.

dest:
index: new_twitter
script:
inline: ctx._source.user = "other" + ctx._source.user

This comment has been minimized.

@nik9000

nik9000 Mar 10, 2016

Contributor

I've intentionally not ported master to use painless so this will match master exactly.

@nik9000

nik9000 Mar 10, 2016

Contributor

I've intentionally not ported master to use painless so this will match master exactly.

@nik9000 nik9000 added the review label Mar 10, 2016

@nik9000 nik9000 referenced this pull request Mar 10, 2016

Closed

Backport Reindex to 2.3 tracker #17031

7 of 7 tasks complete
static final String CAUSE_FIELD = "cause";
static final String STATUS_FIELD = "status";
public static final Failure PROTOTYPE = new Failure(null, null, null, null);

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

can we just have a Failure(StreamInput in) instead of the prototype I don't know where this pattern came from but I see it in a lot of place

@s1monw

s1monw Mar 13, 2016

Contributor

can we just have a Failure(StreamInput in) instead of the prototype I don't know where this pattern came from but I see it in a lot of place

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

I think it is a side effect of Writeable's contact. I don't like it either but a backport isn't the place to remove it because it'll stick around in master. I've filed #17085 so we can talk about it.

@nik9000

nik9000 Mar 13, 2016

Contributor

I think it is a side effect of Writeable's contact. I don't like it either but a backport isn't the place to remove it because it'll stick around in master. I've filed #17085 so we can talk about it.

This comment has been minimized.

@s1monw

s1monw Mar 14, 2016

Contributor

it's not I don't know how this materialized?

@s1monw

s1monw Mar 14, 2016

Contributor

it's not I don't know how this materialized?

@Override
public Failure readFrom(StreamInput in) throws IOException {
return new Failure(in.readString(), in.readString(), in.readOptionalString(), in.readThrowable());

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

just use a new Failure(in); here

@s1monw

s1monw Mar 13, 2016

Contributor

just use a new Failure(in); here

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

Filed as an issue so I'll do it on the master branch and backport it rather than just do it here.

@nik9000

nik9000 Mar 13, 2016

Contributor

Filed as an issue so I'll do it on the master branch and backport it rather than just do it here.

}
public static class Status implements Task.Status {
public static final Status PROTOTYPE = new Status(0, 0, 0, 0, 0, 0, 0, 0, null);

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

can we just have another ctor that takes StreamInput instead?

@s1monw

s1monw Mar 13, 2016

Contributor

can we just have another ctor that takes StreamInput instead?

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

IIRC this was is used by NamedWriteableRegistry but now I can't find it. I suspect that means I'm missing something in the backport.....

@nik9000

nik9000 Mar 13, 2016

Contributor

IIRC this was is used by NamedWriteableRegistry but now I can't find it. I suspect that means I'm missing something in the backport.....

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

I don't that the backport is missing it - I think both master and this backport will fail if you try to fetch the status from a reindex that isn't the coordinating node. I've added it to my list of things to do on Monday morning.

@nik9000

nik9000 Mar 13, 2016

Contributor

I don't that the backport is missing it - I think both master and this backport will fail if you try to fetch the status from a reindex that isn't the coordinating node. I've added it to my list of things to do on Monday morning.

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

To be clear: I think we should merge this as is and fix it in master and backport the fix to 2.x just like we're going to backport the other things that happened to reindex after we merged it to master.

@nik9000

nik9000 Mar 13, 2016

Contributor

To be clear: I think we should merge this as is and fix it in master and backport the fix to 2.x just like we're going to backport the other things that happened to reindex after we merged it to master.

@Override
protected void doExecute(Task task, ReindexRequest request, ActionListener<ReindexResponse> listener) {
validateAgainstAliases(request.getSearchRequest(), request.getDestination(), indexNameExpressionResolver, autoCreateIndex,

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

should we here or in the superclass fail if the cluster has not fully upgraded to 2.3? just as a safety guard I think that would be a good check in several places otherwise I can see us debugging weird issues DiscoveryNodes#smallestNonClientNodeVersion() has a neat method to check.

@s1monw

s1monw Mar 13, 2016

Contributor

should we here or in the superclass fail if the cluster has not fully upgraded to 2.3? just as a safety guard I think that would be a good check in several places otherwise I can see us debugging weird issues DiscoveryNodes#smallestNonClientNodeVersion() has a neat method to check.

This comment has been minimized.

@bleskes

bleskes Mar 13, 2016

Member

I wondered in the past whether we should add something to the transport registration logic indicating which action were added on what version, so we can automatically disable (new) actions if any of the nodes in the cluster is too old. Feels like this kind of things belong in a generic infra so action implementers won't need to worry about them...

@bleskes

bleskes Mar 13, 2016

Member

I wondered in the past whether we should add something to the transport registration logic indicating which action were added on what version, so we can automatically disable (new) actions if any of the nodes in the cluster is too old. Feels like this kind of things belong in a generic infra so action implementers won't need to worry about them...

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

@imotov had the same idea. Maybe it is a thing we should think about in
5.x.

Oh! I left a comment earlier about resolving issues but I just forgot
resolve the "don't run this on a mixed cluster" thing. I'll get that in the
morning.
On Mar 13, 2016 5:11 PM, "Boaz Leskes" notifications@github.com wrote:

In
modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportReindexAction.java
#17060 (comment)
:

  • @Inject
  • public TransportReindexAction(Settings settings, ThreadPool threadPool, ActionFilters actionFilters,
  •        IndexNameExpressionResolver indexNameExpressionResolver, ClusterService clusterService, ScriptService scriptService,
    
  •        AutoCreateIndex autoCreateIndex, Client client, TransportService transportService) {
    
  •    super(settings, ReindexAction.NAME, threadPool, transportService, actionFilters, indexNameExpressionResolver,
    
  •            ReindexRequest.class);
    
  •    this.clusterService = clusterService;
    
  •    this.scriptService = scriptService;
    
  •    this.autoCreateIndex = autoCreateIndex;
    
  •    this.client = client;
    
  • }
  • @override
  • protected void doExecute(Task task, ReindexRequest request, ActionListener listener) {
  •    validateAgainstAliases(request.getSearchRequest(), request.getDestination(), indexNameExpressionResolver, autoCreateIndex,
    

I wondered in the past whether we should add something to the transport
registration logic indicating which action were added on what version, so
we can automatically disable (new) actions if any of the nodes in the
cluster is too old. Feels like this kind of things belong in a generic
infra so action implementers won't need to worry about them...


Reply to this email directly or view it on GitHub
https://github.com/elastic/elasticsearch/pull/17060/files#r55943844.

@nik9000

nik9000 Mar 13, 2016

Contributor

@imotov had the same idea. Maybe it is a thing we should think about in
5.x.

Oh! I left a comment earlier about resolving issues but I just forgot
resolve the "don't run this on a mixed cluster" thing. I'll get that in the
morning.
On Mar 13, 2016 5:11 PM, "Boaz Leskes" notifications@github.com wrote:

In
modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportReindexAction.java
#17060 (comment)
:

  • @Inject
  • public TransportReindexAction(Settings settings, ThreadPool threadPool, ActionFilters actionFilters,
  •        IndexNameExpressionResolver indexNameExpressionResolver, ClusterService clusterService, ScriptService scriptService,
    
  •        AutoCreateIndex autoCreateIndex, Client client, TransportService transportService) {
    
  •    super(settings, ReindexAction.NAME, threadPool, transportService, actionFilters, indexNameExpressionResolver,
    
  •            ReindexRequest.class);
    
  •    this.clusterService = clusterService;
    
  •    this.scriptService = scriptService;
    
  •    this.autoCreateIndex = autoCreateIndex;
    
  •    this.client = client;
    
  • }
  • @override
  • protected void doExecute(Task task, ReindexRequest request, ActionListener listener) {
  •    validateAgainstAliases(request.getSearchRequest(), request.getDestination(), indexNameExpressionResolver, autoCreateIndex,
    

I wondered in the past whether we should add something to the transport
registration logic indicating which action were added on what version, so
we can automatically disable (new) actions if any of the nodes in the
cluster is too old. Feels like this kind of things belong in a generic
infra so action implementers won't need to worry about them...


Reply to this email directly or view it on GitHub
https://github.com/elastic/elasticsearch/pull/17060/files#r55943844.

@Override
protected void copyRouting(IndexRequest index, SearchHit doc) {
String routingSpec = mainRequest.getDestination().routing();
if (routingSpec == null) {

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

I know it's random here but can we do this in if / if else / else blocks instead of returns?

@s1monw

s1monw Mar 13, 2016

Contributor

I know it's random here but can we do this in if / if else / else blocks instead of returns?

* Stuffing a number into the map will have converted it to
* some Number.
*/
Number fromNumber;

This comment has been minimized.

@s1monw

s1monw Mar 13, 2016

Contributor

this can be final?

@s1monw

s1monw Mar 13, 2016

Contributor

this can be final?

This comment has been minimized.

@nik9000

nik9000 Mar 13, 2016

Contributor

If you think it'd be easier to read that way. I don't though.

@nik9000

nik9000 Mar 13, 2016

Contributor

If you think it'd be easier to read that way. I don't though.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Mar 13, 2016

Contributor

@nik9000 I did a very high level pass on this and I think it looks great, we reviewed most of the code already in master and it has been backing, I added a comment regarding failing any reindex job while there is any node <= 2.3 in the cluster which I think should happen as early as possible but also within the actions. It's just too risky to assume this can work on anything but a fully upgraded 2.3 cluster. I think we should even fail in the rest layer as well...

Contributor

s1monw commented Mar 13, 2016

@nik9000 I did a very high level pass on this and I think it looks great, we reviewed most of the code already in master and it has been backing, I added a comment regarding failing any reindex job while there is any node <= 2.3 in the cluster which I think should happen as early as possible but also within the actions. It's just too risky to assume this can work on anything but a fully upgraded 2.3 cluster. I think we should even fail in the rest layer as well...

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Mar 13, 2016

Contributor

Thanks for doing it! I'll have a more thorough read through soon. I agree
with stopping the whole request in a mixed cluster. It just isn't worth the
trouble.
On Mar 13, 2016 9:05 AM, "Simon Willnauer" notifications@github.com wrote:

@nik9000 https://github.com/nik9000 I did a very high level pass on
this and I think it looks great, we reviewed most of the code already in
master and it has been backing, I added a comment regarding failing any
reindex job while there is any node <= 2.3 in the cluster which I think
should happen as early as possible but also within the actions. It's just
too risky to assume this can work on anything but a fully upgraded 2.3
cluster. I think we should even fail in the rest layer as well...


Reply to this email directly or view it on GitHub
#17060 (comment)
.

Contributor

nik9000 commented Mar 13, 2016

Thanks for doing it! I'll have a more thorough read through soon. I agree
with stopping the whole request in a mixed cluster. It just isn't worth the
trouble.
On Mar 13, 2016 9:05 AM, "Simon Willnauer" notifications@github.com wrote:

@nik9000 https://github.com/nik9000 I did a very high level pass on
this and I think it looks great, we reviewed most of the code already in
master and it has been backing, I added a comment regarding failing any
reindex job while there is any node <= 2.3 in the cluster which I think
should happen as early as possible but also within the actions. It's just
too risky to assume this can work on anything but a fully upgraded 2.3
cluster. I think we should even fail in the rest layer as well...


Reply to this email directly or view it on GitHub
#17060 (comment)
.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Mar 13, 2016

Contributor

@s1monw: OK! I fixed two issues you raised, I don't want to fix two others because I like my way better, I created an issue to discuss a pattern that I used and neither of us really like (#17085), and one of your questions found an issue that is in master as well so I'd like to fix it there and backport separately.

Contributor

nik9000 commented Mar 13, 2016

@s1monw: OK! I fixed two issues you raised, I don't want to fix two others because I like my way better, I created an issue to discuss a pattern that I used and neither of us really like (#17085), and one of your questions found an issue that is in master as well so I'd like to fix it there and backport separately.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Mar 14, 2016

Contributor

@s1monw I believe this is ready for another round.

Contributor

nik9000 commented Mar 14, 2016

@s1monw I believe this is ready for another round.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Mar 14, 2016

Contributor

LGTM

Contributor

s1monw commented Mar 14, 2016

LGTM

@nik9000 nik9000 merged commit 78f7808 into elastic:2.x Mar 14, 2016

1 check passed

CLA Commit author has signed the CLA
Details

@nik9000 nik9000 removed the review label Mar 14, 2016

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Mar 14, 2016

Contributor

Hurray! Thanks for all the review @s1monw and @bleskes!

Contributor

nik9000 commented Mar 14, 2016

Hurray! Thanks for all the review @s1monw and @bleskes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment