ingest: Add ignore_missing property to foreach filter (#22147) #31578
Conversation
colings86
added
>enhancement
:Data Management/Ingest Node
|
Pinging @elastic/es-core-infra |
| @@ -48,6 +48,23 @@ public void testCreate() throws Exception { | |||
| assertThat(forEachProcessor.getProcessor(), Matchers.sameInstance(processor)); | |||
There was a problem hiding this comment.
can we add an assertion that ignore_missing is false by default?
| IngestDocument ingestDocument = new IngestDocument( | ||
| "_index", "_type", "_id", null, null, null, Collections.emptyMap() | ||
| ); | ||
| ForEachProcessor processor = new ForEachProcessor("_tag", "_ingest._value", new AbstractProcessor("noop") { |
There was a problem hiding this comment.
just so we are more explicit, maybe it would be clearer to re-use the test-processor to validate that things
are not executed?
maybe something like this
TestProcessor testProcessor = new TestProcessor(doc -> {});
ForEachProcessor processor = new ForEachProcessor("_tag", "_ingest._value", testProcessor);
processor.execute(ingestDocument);
assertThat(testProcessor.getInvokedCounter(), equalTo(0));
original-brownbear
force-pushed
the
22147
branch
from
33427ff
to
93d6923
Compare
|
@talevy thanks for the review! Do I backport this one to any branch? |
|
@original-brownbear to Since this is a small enough change that I don't expect to cause merge-conflicts, it is |
| import org.elasticsearch.ingest.TestProcessor; | ||
| import org.elasticsearch.ingest.TestTemplateService; | ||
| import org.elasticsearch.script.TemplateScript; | ||
| import org.elasticsearch.test.ESTestCase; |
There was a problem hiding this comment.
Please change the import order in your IDE so that these sort before java.util and that would have led to these import orderings not changing. For example:
It's annoying to see these change in PR after PR based on different IDE settings.
There was a problem hiding this comment.
Done :), sorry about that
| this.ignoreMissing = ignoreMissing; | ||
| } | ||
|
|
||
| boolean isIgnoreMissing() { |
There was a problem hiding this comment.
Why was this method added? It doesn't seem to add anything, other than being checked in tests, but the tests are directly setting it when constructing the processor.
There was a problem hiding this comment.
@rjernst tbh. I only added this since the GeoIP processor had the same getter (as did all other fields in this processor) so I just stuck with the style. You're right though, this is effectively test only code leaking in :(
* 6.x: Fix broken backport of #31578 by adjusting constructor (#31587) ingest: Add ignore_missing property to foreach filter (#22147) (#31578) Add package pre-install check for java binary (#31343) Docs: Clarify sensitive fields watcher encryption (#31551) Watcher: Remove never executed code (#31135) Improve test times for tests using `RandomObjects::addFields` (#31556) Revert "Remove RestGetAllAliasesAction (#31308)" REST high-level client: add simulate pipeline API (#31158) Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507) Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527) [Test] Add full cluster restart test for Rollup (#31533) Enhance thread context uniqueness assertion fix writeIndex evaluation for aliases (#31562) Add x-opaque-id to search slow logs (#31539) Watcher: Fix put watch action (#31524) [DOCS] Significantly improve SQL docs turn GetFieldMappingsResponse to ToXContentObject (#31544) TEST: Unmute testHistoryUUIDIsGenerated Ingest Attachment: Upgrade Tika to 1.18 (#31252) TEST: Correct the assertion arguments order (#31540)
* master: ingest: Add ignore_missing property to foreach filter (#22147) (#31578) Fix a formatting issue in the docvalue_fields documentation. (#31563) reduce log level at gradle configuration time [TEST] Close additional clients created while running yaml tests (#31575) Docs: Clarify sensitive fields watcher encryption (#31551) Watcher: Remove never executed code (#31135) Add support for switching distribution for all integration tests (#30874) Improve robustness of geo shape parser for malformed shapes (#31449) QA: Create xpack yaml features (#31403) Improve test times for tests using `RandomObjects::addFields` (#31556) [Test] Add full cluster restart test for Rollup (#31533) Enhance thread context uniqueness assertion [DOCS] Fix heading format errors (#31483) fix writeIndex evaluation for aliases (#31562) Add x-opaque-id to search slow logs (#31539) Watcher: Fix put watch action (#31524) Add package pre-install check for java binary (#31343) Reduce number of raw types warnings (#31523) Migrate scripted metric aggregation scripts to ScriptContext design (#30111) turn GetFieldMappingsResponse to ToXContentObject (#31544) Close xcontent parsers (partial) (#31513) Ingest Attachment: Upgrade Tika to 1.18 (#31252) TEST: Correct the assertion arguments order (#31540)
I think that this may have been taken too literally/not explained clearly enough:
It is not sufficient to have a clean cherry-pick and then it's okay to push. As you saw, compilation can break easy for a variety of reasons (e.g., a common one is an import that can be removed in master but is still needed in the backport branch). There are commonly refactorings in the codebase that only land in master which can impact how easy it is to backport a change, even if it cherry-picks cleanly. Rather, we would prefer that the full test suite is run for all commits before pushing. Yes, this can be burdensome because of the length of our test suite but the cost of a broken build on the number of people that work in this codebase (the Elasticsearch and ML teams) is quite high. We have ways to help with this, for example, you can open a backport PR and let infra CI churn on the build. Or, we have tools that you can use locally on desktop-class hardware that can help iterate on these backport builds (I will reach out via another channel about this to help get you set up for this). We also have a dream (that is actively in the requirements phase) of a backport bot that will help alleviate the pain of this even more. For now though, let's make sure that we are running tests before pushing to any shared branch. |
|
true, thanks @jasontedor. my bad for not amending that |
* elastic/master: (57 commits) HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (elastic#31215) Remove legacy MetaDataStateFormat (elastic#31603) Add explain API to high-level REST client (elastic#31387) Preserve thread context when connecting to remote cluster (elastic#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (elastic#31494) [TEST] call yaml client close method from test suite (elastic#31591) ingest: Add ignore_missing property to foreach filter (elastic#22147) (elastic#31578) Fix a formatting issue in the docvalue_fields documentation. (elastic#31563) reduce log level at gradle configuration time [TEST] Close additional clients created while running yaml tests (elastic#31575) Docs: Clarify sensitive fields watcher encryption (elastic#31551) Watcher: Remove never executed code (elastic#31135) Add support for switching distribution for all integration tests (elastic#30874) Improve robustness of geo shape parser for malformed shapes (elastic#31449) QA: Create xpack yaml features (elastic#31403) Improve test times for tests using `RandomObjects::addFields` (elastic#31556) ...
Adds the
ignore_missingfunctionality to the foreach filter to fix #22147.Tried to keep it as close to the way this is implemented in the GeoIP filter as possible.