Support for version and version_type #7480
Conversation
|
LGTM. It would be nice to have some non-REST tests? |
|
The change looks good to me too, but I'm wondering if version support is actually useful on term vectors? Do you have practical applications in mind? |
|
@jpountz Term vectors are near real-time, so this would make a lot more sense once TVs could be generated in real-time. Perhaps support for @rjernst I think versioning itself is already thoroughly tested in non-REST tests. So it makes sense to test versioning for TVs only through REST-tests, just like for the get API? |
|
@alexksikes I think I'm confused because the |
| @@ -392,6 +420,10 @@ public static void parseRequest(TermVectorRequest termVectorRequest, XContentPar | |||
| termVectorRequest.id = parser.text(); | |||
| } else if ("_routing".equals(currentFieldName) || "routing".equals(currentFieldName)) { | |||
| termVectorRequest.routing = parser.text(); | |||
| } else if ("_version".equals(currentFieldName) || "version".equals(currentFieldName)) { | |||
There was a problem hiding this comment.
Any reason we're setting ourselves up for two different field names rather than just picking one (prefer _version)?
There was a problem hiding this comment.
This is the same as for the get API.
|
@jpountz I think it is implemented in the mget API. But anyway, I think that feature is useful. Suppose you get a document and want later to generate its TVs. This would ensure you only generate the TVs of that document, and error out if the document has been updated in between ... |
|
@alexksikes do you still plan on merging this? |
|
@clintongormley Yes, I did run into a bug, so postponed it. |
alexksikes
force-pushed
the
feature/tv-versionning
branch
from
f48a4c3
to
f83f51a
Compare
alexksikes
force-pushed
the
feature/tv-versionning
branch
from
f83f51a
to
792fa9f
Compare
|
OK rebased on master and ready for review. |
| @@ -140,7 +140,6 @@ else if (docIdAndVersion != null) { | |||
| throw new ElasticsearchException("failed to execute term vector request", ex); | |||
| } finally { | |||
| searcher.close(); | |||
| get.release(); | |||
| } | |||
There was a problem hiding this comment.
Changes in this file make the assumption that the only thing that a Engine.GetResult.close release are the underlying searcher, I think it's too fragile
|
@alexksikes Just left one comment, other than that the change looks good. Like @rjernst I think we should have Java tests, which have better randomization than REST tests and allow to test the Java client API (for instance right now, there are no call sites for |
|
LGTM |
clintongormley
changed the title
clintongormley
added
:Search/Search
This commit adds support for version and version_type to the Term Vectors API. This could be useful in the following case whereby the user gets a document and later wants to generate its TVs. With version, this would ensure that only the TVs of that particular document are generated, and error out if the document has been updated in between.