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

Simplify dynamic mappings updates. #10720

Merged
merged 1 commit into from Apr 23, 2015

Conversation

Projects
None yet
4 participants
@jpountz
Copy link
Contributor

commented Apr 22, 2015

While dynamic mappings updates are using the same code path as updates from the
API when applied on a data node since #10593, they were still using a different
code path on the master node. This commit makes dynamic updates processed the
same way as updates from the API, which also seems to do a better way at
acknowledgements (I could not reproduce the ConcurrentDynamicTemplateTests
failure anymore). It also adds more checks, like for instance that indexing on
replicas should not trigger dynamic mapping updates since they should have been
handled on the primary before.

// hand putting the mapping would start the river, which expects
// to find a _meta document
// So we have no choice but to index first and send mappings afterwards
indexService.mapperService().merge(indexRequest.type(), new CompressedString(update.toBytes()), true);

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 22, 2015

Contributor

can we assigne indexService.mapperService() to a var? just to remove the chaining :)

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 22, 2015

Author Contributor

Ok will do

this.type = type;
this.mappingSource = mappingSource;
this.listener = listener;
public void updateMappingOnMasterSynchronously(String index, String type, Mapping mappingUpdate, TimeValue timeout) throws Throwable {

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 22, 2015

Contributor

I wonder where the uuid went?

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 22, 2015

Author Contributor

It used to be necessary but now the update is essentially about calling putMapping on the IndicesAdminClient which does not need the index uuid

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 23, 2015

Contributor

so I wonder if we should pass it on though. I think it's good to validate it and if we use the client as a user of ES we just pass IndexMetaData#INDEX_UUID_NA_VALUE as the default?

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2015

left a couple of comments

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2015

LGTM I think we should add the UUID in a second step in a followup issue

Mappings: simplify dynamic mappings updates.
While dynamic mappings updates are using the same code path as updates from the
API when applied on a data node since #10593, they were still using a different
code path on the master node. This commit makes dynamic updates processed the
same way as updates from the API, which also seems to do a better way at
acknowledgements (I could not reproduce the ConcurrentDynamicTemplateTests
failure anymore). It also adds more checks, like for instance that indexing on
replicas should not trigger dynamic mapping updates since they should have been
handled on the primary before.

Close #10720

@jpountz jpountz force-pushed the jpountz:fix/simplify_mapperupdatedaction branch from 9301b56 to c6cdf77 Apr 23, 2015

jpountz added a commit that referenced this pull request Apr 23, 2015

Merge pull request #10720 from jpountz/fix/simplify_mapperupdatedaction
Mappings: simplify dynamic mappings updates.

Close #10720

@jpountz jpountz merged commit 803c393 into elastic:master Apr 23, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@kevinkluge kevinkluge removed the in progress label Apr 23, 2015

@clintongormley clintongormley changed the title Mappings: simplify dynamic mappings updates. Simplify dynamic mappings updates. Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.