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

version set in ingest pipeline #27573

Merged
merged 15 commits into from Feb 21, 2018

Conversation

PnPie
Copy link
Contributor

@PnPie PnPie commented Nov 28, 2017

As discussed in #27242, I make this PR firstly for support of setting document version and version type in set processor of an ingest pipeline.
If it's okay, I'm really happy to try to create a new processor later for version set if needed.

Add support for setting document version and version type in set
processor of an ingest pipeline.
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@DaveCTurner DaveCTurner added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v6.2.0 v7.0.0 labels Nov 28, 2017
@DaveCTurner
Copy link
Contributor

@elasticmachine please test this

@PnPie
Copy link
Contributor Author

PnPie commented Nov 28, 2017

I see the test fails because there are some lines longer than 140 characters. So I modified it.

@rjernst
Copy link
Member

rjernst commented Dec 21, 2017

@elasticmachine test this please

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@rjernst
Copy link
Member

rjernst commented Dec 22, 2017

@elasticmachine test this please

@rjernst
Copy link
Member

rjernst commented Dec 22, 2017

@elasticmachine test this please

@PnPie
Copy link
Contributor Author

PnPie commented Dec 24, 2017

Hello,
Seems the build failed cause some problems encountered while installing AnalysisIcuPlugin ? But I didn't reproduced it locally by running build for :docs subproject.

21:36:49 * What went wrong:
21:36:49 Execution failed for task ':docs:integTestCluster#installAnalysisIcuPlugin'.
21:36:49 > A problem occurred starting process 'command '/var/lib/jenkins/workspace/elastic+elasticsearch+pull-request/docs/build/cluster/integTestCluster node0/elasticsearch-7.0.0-alpha1-SNAPSHOT/bin/elasticsearch-plugin''

@rjernst
Copy link
Member

rjernst commented Jan 9, 2018

@PnPie Sorry I did not see the test failure output over the holidays. I will run them again.

@elasticmachine test this please

@rjernst
Copy link
Member

rjernst commented Jan 10, 2018

@PnPie can you please merge the latest master into your branch? We have started using the gradle wrapper, and CI is expecting the wrapper to exist in order to run tests.

@PnPie
Copy link
Contributor Author

PnPie commented Jan 10, 2018

Hello @rjernst ,
I just merged master into this.

@rjernst
Copy link
Member

rjernst commented Jan 16, 2018

@PnPie I'm sorry to have to ask again, but there was large move of files from core to server, so this needs another merge with master.

@PnPie
Copy link
Contributor Author

PnPie commented Jan 16, 2018

@rjernst there is no problem, I merged again. And just feel free to push to this branch if needed :)

@colings86 colings86 added v6.3.0 and removed v6.2.0 labels Jan 22, 2018
@rjernst
Copy link
Member

rjernst commented Jan 24, 2018

@elasticmachine test this please

@martijnvg
Copy link
Member

@elasticmachine test this please

(previous build results were removed)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Hey @PnPie, I took a look at your PR and left some suggestions. Can you also add a yml test file in ./modules/ingest-common/src/test/resources/rest-api-spec/test/ingest to tests version and version_type for set and append processors?

values.add(value);
appendProcessor = createAppendProcessor(randomMetaData.getFieldName(), value);
int valuesSize = randomIntBetween(0, 10);
if (randomMetaData == IngestDocument.MetaData.VERSION) {
Copy link
Member

Choose a reason for hiding this comment

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

Can version and version_type be tested in a dedicated test? I think that makes these tests easier to read.

@@ -101,10 +102,28 @@ public void testSetExistingNullFieldWithOverrideDisabled() throws Exception {

public void testSetMetadata() throws Exception {
IngestDocument.MetaData randomMetaData = randomFrom(IngestDocument.MetaData.values());
Processor processor = createSetProcessor(randomMetaData.getFieldName(), "_value", true);
Processor processor;
Copy link
Member

Choose a reason for hiding this comment

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

Same as previous comment.

@@ -73,6 +73,17 @@ public IngestDocument(String index, String type, String id, String routing, Stri
this.ingestMetadata.put(TIMESTAMP, ZonedDateTime.now(ZoneOffset.UTC));
}

public IngestDocument(String index, String type, String id, String routing,String parent,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a new constructor, can the existing constructor be modified to also accept version and versionType? I know this requires changing more tests, but I think this class should only have two constructors.

@PnPie
Copy link
Contributor Author

PnPie commented Feb 10, 2018

Hello @martijnvg :)
I did the changes you mentioned but I'm not really familiar with the yml test, so I pushed sth. I did and I may have some questions.
For the version type, seems we can't get it directly from a document ? All we can do is to add a parameter to GET request (ex. version_type=internal) to get documents with specified version_type ? But can we do this in yml test ?

And I also find that specifying version_type in a GET request seems doesn't work. I created an index with a document all simple, but I can GET the document by specifying whatever for version_type, shall we have a bug here ?

index: test
type: test
id: 1
- match: { _version: 3 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here for a GET request, can we put parameters to specify version_type for example ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, version_type and version can be specified below the id parameter.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thanks for updating the pr @PnPie!

All we can do is to add a parameter to GET request (ex. version_type=internal) to get documents with specified version_type ? But can we do this in yml test ?

This can be specified in yaml tests. For example look at ./elasticsearch/rest-api-spec/src/main/resources/rest-api-spec/test/get/90_versions.yml file for how version_type can be used in yaml tests.

And I also find that specifying version_type in a GET request seems doesn't work. I created an index with a document all simple, but I can GET the document by specifying whatever for version_type, shall we have a bug here ?

I don't think there is a bug here, a version conflict is being thrown if the provided version does not match with the version that a document has. The difference in behavior between version_type is subtle. (most notable is that external version type fails if expected version is unequal to actual version whereas external_gte version_type fails if external version is smaller than the actual version)

After thinking more about version_type and version, I think it does not make sense to specifically test these meta fields with the append processor, because only a single version / version_type can be specified per document. So can you remove the tests you added for append processor?

index: test
type: test
id: 1
- match: { _version: 3 }
Copy link
Member

Choose a reason for hiding this comment

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

Yes, version_type and version can be specified below the id parameter.

{
"append" : {
"field" : "_version",
"value": [3, 6, 9]
Copy link
Member

Choose a reason for hiding this comment

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

Only a single version / version_type can be specified per document, so maybe it doesn't make sense to test version / version_type with append processor.

remove version/version type tests for append processor
complete yml tests
@PnPie
Copy link
Contributor Author

PnPie commented Feb 13, 2018

Hi @martijnvg,
Thanks for your response, I updated the PR and tested in yml file to set version_type to external (by default it should be internal), and verify if it's good or not.

And also in ./elasticsearch/rest-api-spec/src/main/resources/rest-api-spec/test/get/90_versions.yml, I see we test versions using GET request for this moment, and the versioning system is to control the update of document, isn't it ? Maybe we should test it rather by updating the document (index the same document) ? Because for a GET request, specifying the version_type doesn't really make sense, does it ?

@martijnvg
Copy link
Member

Hey @PnPie, I think we just need to test whether we can set version and version_type in an ingest pipeline. So the following scenarios should be tested:

  • Create pipeline that just sets a _version to 1, index a document with this pipeline, which should fail.
  • Create pipeline that sets version to 1 and set version_type to external. Then index like in the previous scenario and all indexing requests should succeed.

That is it, no need test anything else.

@martijnvg
Copy link
Member

@elasticmachine test this please

@martijnvg
Copy link
Member

@PnPie If you like I can make these changes to the yaml test? Then we can get this change merged in.

@PnPie
Copy link
Contributor Author

PnPie commented Feb 16, 2018

Hello @martijnvg,
Sorry for the late response, seems there are still some serialization problem for version_type according to the tests. And I also find that when we set version using set processor, I have an error of "cannot cast Integer to Long" because the value we set is considered as a Integer, but the version is a Long ?

And for the yml test, yes you can push directly on the branch, because maybe I didn't well get your point, the first scenario you mentioned, just set _version to 1, it should not fail ?

…out hassle.

When applying the version to the IndexRequest just cast to Number and fetch its long value.
Simplify yaml versions to just tests to see that both version and version_type can be used from an ingest pipeline.
@martijnvg
Copy link
Member

@PnPie No problem. I addressed the issues that caused the build to fail in the latest commit. So hopefully we can get this PR in soon.

And for the yml test, yes you can push directly on the branch, because maybe I didn't well get your point, the first scenario you mentioned, just set _version to 1, it should not fail ?

Yes, so the first pipeline with use internal versioning. The first version of a document is always -2 I believe, so setting the expected version to 1 will make it fail. In case of external versioning ES is less strict when it comes to expecting what version it expected. This way we can see whether setting version type and version in a pipeline actually works.

@martijnvg
Copy link
Member

@elasticmachine test this please

@PnPie
Copy link
Contributor Author

PnPie commented Feb 16, 2018

Okay @martijnvg thx for the fix ! I just thought the initial version of a document is 1 ...

@martijnvg
Copy link
Member

@elasticmachine test this please

@martijnvg
Copy link
Member

@elasticmachine test this please

@martijnvg martijnvg merged commit 7d8fb69 into elastic:master Feb 21, 2018
@martijnvg
Copy link
Member

@PnPie Thank you for your contribution!

martijnvg pushed a commit that referenced this pull request Feb 21, 2018
Add support version and version_type in ingest pipelines

Add support for setting document version and version type in set
processor of an ingest pipeline.
martijnvg added a commit that referenced this pull request Feb 22, 2018
* es/master: (143 commits)
  Revert "Disable BWC tests for build issues"
  Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724)
  Build: Consolidate archives and packages configuration (#28760)
  Skip some plugins service tests on Windows
  Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749)
  Disable BWC tests for build issues
  Ensure that azure stream has socket privileges (#28751)
  [DOCS] Fixed broken link.
  Pass InputStream when creating XContent parser (#28754)
  [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677
  Delay path expansion on Windows
  [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween
  [Tests] Extract the testing logic for Google Cloud Storage (#28576)
  [Docs] Update links to java9 docs (#28750)
  version set in ingest pipeline (#27573)
  Revert "Add startup logging for standalone tests"
  Tests: don't wait for completion while trying to get completed task
  Add 5.6.9 snapshot version
  [Docs] Java high-level REST client : clean up (#28703)
  Updated distribution outputs in contributing docs
  ...
martijnvg added a commit that referenced this pull request Feb 22, 2018
* es/6.x: (140 commits)
  Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724)
  Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749)
  Revert "Disable BWC tests for build issues"
  Skip some plugins service tests on Windows
  Build: Consolidate archives and packages configuration (#28760)
  Ensure that azure stream has socket privileges (#28773)
  Disable BWC tests for build issues
  Pass InputStream when creating XContent parser (#28754)
  [DOCS] Fixed broken link.
  [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677
  Delay path expansion on Windows
  [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween
  REST high-level client: add support for Rollover Index API (#28698)
  [Tests] Extract the testing logic for Google Cloud Storage (#28576)
  Moved Grok helper code to a separate Gradle module and let ingest-common module depend on it.
  version set in ingest pipeline (#27573)
  [Docs] Update links to java9 docs (#28750)
  Revert "Add startup logging for standalone tests"
  Tests: don't wait for completion while trying to get completed task
  Add 5.6.9 snapshot version
  ...
sebasjm pushed a commit to sebasjm/elasticsearch that referenced this pull request Mar 10, 2018
Add support version and version_type in ingest pipelines

Add support for setting document version and version type in set
processor of an ingest pipeline.
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants