fix index refresh in test within 20_mix_typeless_typeful

[talevy]

the test "Implicitly create a typeless ... typed template"
fails occasionally because the index operation hasn't
propogated to update the index mapping in time for the
following assertion about a dynamically mapped field "bar".

error failed with:

field [test-1.mappings.my_type.properties.bar] doesn't have a true value
Expected: not null
     but: was null

refreshing the index should resolve this timing issue.

[elasticmachine]

Pinging @elastic/es-search

[jaymode]

LGTM

[jtibshirani]

Thanks @talevy for debugging this! To check I understand, it looks like the dynamic mapping update triggered by the index request isn't visible to the get_mapping call. Does an index refresh also ensure that mapping updates are available? As a side note, I came across this other test PR which ran into a similar issue: #38045

[talevy]

thanks for bringing that up Julie. In fact, I see no reason refresh should fix this problem, and I also don't see adding bar to the mapping as changing the nature of the test.

updated to add bar into the mapping

[dakrone]

To check I understand, it looks like the dynamic mapping update triggered by the index request isn't visible to the get_mapping call.

Yep, that is the cause.

Does an index refresh also ensure that mapping updates are available?

It helps, but doesn't enforce it. Rather this is a timing issue with indexing and mapping updates. Mapping update visibility is asynchronous from the client's perspective (see #38873) so this hopefully will help.

[talevy]

@jaymode I updated the code. mind re-reviewing?

[jpountz]

Thanks all for looking into this, I had missed that dynamic mapping updates were no longer acked!

Since we are now adding the field to the mappings in the template, the refresh is irrelevant I think?

Not including the mapping definition in the template was intentional, I wanted to test how Elasticsearch behaves when there is a typed template while an index and mappings are implicitly created via a typeless index call. I'm wondering whether there is something that we can do to force the mapping update to be visible to the client, eg. would running another put-mapping request give this guarantee?

[talevy]

Since we are now adding the field to the mappings in the template, the refresh is irrelevant I think?

it does seem to be irrelevant

Not including the mapping definition in the template was intentional, I wanted to test how Elasticsearch behaves when there is a typed template while an index and mappings are implicitly created via a typeless index call.

ah! I should have stuck with my gut. thanks.

I'm wondering whether there is something that we can do to force the mapping update to be visible to the client, eg. would running another put-mapping request give this guarantee?

I'll take a look at the code. that would obviously give a timing buffer, but I'm not sure whether the
update prioritization will always work in our favor.

[talevy]

@jpountz
now that I am looking at it again, I think I misunderstood the bit about PutMapping.

The test is interested in how the direct index-mapping and the index-template-mapping
interact. Doing an explicit create-index-request with the mapping for bar would be ok?

or is it specifically interested in dynamic-mapping behavior

[talevy]

ah nevermind. that is exactly what

"Create a typed index while there is a typeless template" is testing

[talevy]

I will update to put the same mapping as is done dynamically before getting the mapping. I think that is reasonable since the dynamic-mapping update just issues a regular put-mapping-request itself

[talevy]

I muted the tests until we figure this out

[talevy]

build failed due to: #29880

run elasticsearch-ci/2

[talevy]

@jpountz do you have any further thoughts? My change is competing with reality, and probably even an empty put-mapping-request would due the trick due to timing, but 🤷‍♂️

I checked the internal update-mapping-request and it looked like it passed in a type, so that is why the request here uses types

Fix has changed

[jaymode]

I wonder if this should be moved from a rest yml test to an IT? This way we can add an awaitsBusy for the mapping update and not need the hacks of an empty put mapping or anything else.

[talevy]

@jaymode yes. yes it should since there does not seem to be another way of correctly fixing this. I'll update shortly. thanks!

[jpountz]

If there is a simple way that we could wait for the mapping update to become visible, this would be my preference. In general I prefer REST tests over IT as I find them more realistic.

[jaymode]

In general I prefer REST tests over IT as I find them more realistic.

@jpountz can you expand on this? If the IT runs against an external cluster and makes calls using an HTTP client, is it really less realistic?

[jpountz]

Apologies, I had assumed you meant a test with an internal cluster.

[jpountz]

I just had a quick discussion with @ywelsch and @DaveCTurner who told me that calling the cluster health API with waitForEvents = NORMAL would ensure that the mapping update is visible to get-mappings.

[talevy]

Just got back to this. thanks for the update Adrien. I missed the notification...

updated to use the cluster.health call to block. Much nicer than having to create new rest tests.

[jpountz]

Thanks Tal!