Skip to content
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 API to upgrade old Lucene indices to the latest version #7922

Closed
wants to merge 9 commits into from

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Sep 29, 2014

I'm putting this up now to get early feedback. I am still working on testing, but the implementation is basically there. I've also attempted to add docs (and modify optimize docs as necessary).

This branch does the following:

  • Add the new API at the rest layer, being backed by the optimize API with upgrade flag, and segments api to find upgrade status.
  • Add upgrade flag to optimize API, and deprecate force flag (will remove in master)

closes #7884

I'm putting this up now to get early feedback.  I am still working on
testing, but the implementation is basically there.  I've also attempted
to add docs (and modify optimize docs as necessary).

This branch does the following:
* Add the new API at the rest layer, being backed by the optimize API
* with upgrade flag, and segments api to find upgrade status.
* Add `upgrade` flag to optimize API, and deprecate `force` flag (will
* remove in master)

closes elastic#7884
}

// this is a silly class which should just be a standalone function, but java doesn't even have a standard Pair that could
// be used to return 2 values from a function...
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure not having a Pair class was intentional because it forces you to write one of these classes and document what the fields mean. Probably should expose the fields with public methods instead of make them public - another Java-ism.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, even better than a pair would be allowing multiple return values. But modern languages don't seem to understand that you can push more than one return value onto the stack...

"Exposing the fields with public methods" is what, IMO, makes average java code more complex than it needs to be. By that, I don't mean encapsulation is bad (when it is useful), but simply that too many people follow a "rule" and encapsulate everything, not giving any thought to whether encapsulation is necessary. Here, it is in a private inner class, which serves this extremely limited purpose. But if it was public, I would still not use encapsulation, I would just make these final and use temps inside the constructor.

Sorry for the soapbox...

Copy link
Contributor

Choose a reason for hiding this comment

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

we have Tuple.java if you are up for it

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't aware of Tuple.java! I will switch to that.

@nik9000
Copy link
Member

nik9000 commented Sep 30, 2014

I'm worried that this (like the optimize api in general) could cause an overwhelming amount of IO. I'm not really sure what to do about that or if that is even something to be handled by this pull request. Probably not.

@s1monw
Copy link
Contributor

s1monw commented Sep 30, 2014

I'm worried that this (like the optimize api in general) could cause an overwhelming amount of IO. I'm not really sure what to do about that or if that is even something to be handled by this pull request. Probably not.

that is by design, you can't avoid it we already try to minimise it as much as possible by only rewriting the segments rather than merging them in to big-ass segments.

@nik9000
Copy link
Member

nik9000 commented Sep 30, 2014

that is by design, you can't avoid it we already try to minimise it as much as possible by only rewriting the segments rather than merging them in to big-ass segments.

Fair enough.

@rjernst
Copy link
Member Author

rjernst commented Oct 4, 2014

Ok, I've added a test using the BWC framework. It checks single/all indexes for both GET and POST, as well as wait_for_completion true and false.

@rjernst rjernst added the review label Oct 4, 2014
@@ -48,7 +48,7 @@
maxNumSegments = request.maxNumSegments();
onlyExpungeDeletes = request.onlyExpungeDeletes();
flush = request.flush();
force = request.force();
upgrade = request.force() || request.upgrade();
Copy link
Contributor

Choose a reason for hiding this comment

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

should we drop force in master and only add this to 1.x I think we can just have a cleanup commit on top that is not cherry-picked or so?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just going to do this in a followup commit? I don't like to do rebase as it is error prone, and I also like to keep the history of what I actually did. Instead I do merge --squash when merging to master.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok fair enough.

@s1monw
Copy link
Contributor

s1monw commented Oct 6, 2014

I left some comments, all in all this looks very good. Once question though, we don't commit after the upgrade is done, are we not doing this on purpose or just not in this iteration?

@rjernst
Copy link
Member Author

rjernst commented Oct 6, 2014

I addressed some of the issues noted. Regarding the commit, we set the underlying optimize call to do a flush after the merge(s) are complete.

@s1monw
Copy link
Contributor

s1monw commented Oct 7, 2014

I addressed some of the issues noted. Regarding the commit, we set the underlying optimize call to do a flush after the merge(s) are complete.

yeah I somehow missed that this was fixed :)

@s1monw
Copy link
Contributor

s1monw commented Oct 7, 2014

left one comment other than that LGTM

@s1monw s1monw removed the review label Oct 7, 2014
Conflicts:
	src/main/java/org/elasticsearch/rest/action/RestActionModule.java
@rjernst rjernst closed this in c021f22 Oct 7, 2014
rjernst added a commit that referenced this pull request Oct 7, 2014
This commit does the following:
* Add the new API at the rest layer, being backed by the optimize API
  with upgrade flag, and segments api to find upgrade status.
* Add `upgrade` flag to optimize API, and deprecate `force` flag (will
  remove in master)
* Add test for both synchronous and async upgrade

closes #7884
closes #7922
rjernst added a commit that referenced this pull request Oct 7, 2014
This commit does the following:
* Add the new API at the rest layer, being backed by the optimize API
  with upgrade flag, and segments api to find upgrade status.
* Add `upgrade` flag to optimize API, and deprecate `force` flag (will
  remove in master)
* Add test for both synchronous and async upgrade

closes #7884
closes #7922
@@ -0,0 +1,46 @@
[[indices-upgrade]]

Choose a reason for hiding this comment

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

@rjernst you need to include:: this file from the parent doc, otherwise it won't be built

karmi added a commit that referenced this pull request Oct 9, 2014
* `get_upgrade` => `GET _upgrade`  -- Return the status
* `upgrade`     => `POST _upgrade` -- Perform the operation

Original specification part of c021f22.

Related: #7884, #7922
karmi added a commit that referenced this pull request Oct 9, 2014
* `get_upgrade` => `GET _upgrade`  -- Return the status
* `upgrade`     => `POST _upgrade` -- Perform the operation

Original specification part of c021f22.

Related: #7884, #7922
karmi added a commit that referenced this pull request Oct 9, 2014
* `get_upgrade` => `GET _upgrade`  -- Return the status
* `upgrade`     => `POST _upgrade` -- Perform the operation

Original specification part of c021f22.

Related: #7884, #7922
rjernst added a commit that referenced this pull request Oct 10, 2014
rjernst added a commit that referenced this pull request Oct 10, 2014
rjernst added a commit that referenced this pull request Oct 10, 2014
rjernst added a commit that referenced this pull request Oct 10, 2014
rjernst added a commit that referenced this pull request Oct 10, 2014
rjernst added a commit that referenced this pull request Oct 10, 2014
rjernst added a commit that referenced this pull request Nov 1, 2014
version.

An early version of #7966 had the ability to choose a bwc version
automatically, but this was removed before the change was committed.
However, the change was not removed from the ongoing work in #7922
and it made it in unknowningly.
rjernst added a commit that referenced this pull request Nov 1, 2014
version.

An early version of #7966 had the ability to choose a bwc version
automatically, but this was removed before the change was committed.
However, the change was not removed from the ongoing work in #7922
and it made it in unknowningly.
rjernst added a commit that referenced this pull request Nov 1, 2014
version.

An early version of #7966 had the ability to choose a bwc version
automatically, but this was removed before the change was committed.
However, the change was not removed from the ongoing work in #7922
and it made it in unknowningly.
@clintongormley clintongormley changed the title Add Upgrade API Upgrade API: Add API to upgrade old Lucene indices to the latest version Nov 3, 2014
@rjernst rjernst deleted the pr/7884 branch January 21, 2015 23:22
@clintongormley clintongormley changed the title Upgrade API: Add API to upgrade old Lucene indices to the latest version Add API to upgrade old Lucene indices to the latest version Jun 6, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
This commit does the following:
* Add the new API at the rest layer, being backed by the optimize API
  with upgrade flag, and segments api to find upgrade status.
* Add `upgrade` flag to optimize API, and deprecate `force` flag (will
  remove in master)
* Add test for both synchronous and async upgrade

closes elastic#7884
closes elastic#7922
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
* `get_upgrade` => `GET _upgrade`  -- Return the status
* `upgrade`     => `POST _upgrade` -- Perform the operation

Original specification part of c021f22.

Related: elastic#7884, elastic#7922
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
version.

An early version of elastic#7966 had the ability to choose a bwc version
automatically, but this was removed before the change was committed.
However, the change was not removed from the ongoing work in elastic#7922
and it made it in unknowningly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide api to upgrade lucene indexes to current version
4 participants