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.

Sign up for GitHub

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

Jump to bottom

Better logic on sending mapping update new type introduction #6669

Closed
wants to merge 5 commits into from
Closed

Better logic on sending mapping update new type introduction #6669

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3e5e018
better logic on sending mapping update new type introduction
kimchy Jul 1, 2014
9e413e3
better logging in test
kimchy Jul 1, 2014
e406dcb
doc + simplify logic around using mapper modified
kimchy Jul 1, 2014
bbbab96
fix test to make sure the search requests loads the field data on all…
kimchy Jul 2, 2014
ad63dae
fix exception handling, bring back formatting
kimchy Jul 2, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 0 additions & 7 deletions src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
View file
Open in desktop
Expand Up @@ -281,8 +281,6 @@ protected ParseContext initialValue() {

private final Object mappersMutex = new Object();

private boolean initMappersAdded = true;

public DocumentMapper(String index, @Nullable Settings indexSettings, DocumentMapperParser docMapperParser,
RootObjectMapper rootObjectMapper,
ImmutableMap<String, Object> meta,
Expand Down Expand Up @@ -482,11 +480,6 @@ public ParsedDocument parse(SourceToParse source, @Nullable ParseListener listen
parser = XContentHelper.createParser(source.source());
}
context.reset(parser, new ParseContext.Document(), source, listener);
// on a newly created instance of document mapper, we always consider it as new mappers that have been added
if (initMappersAdded) {
context.setMappingsModified();
initMappersAdded = false;
}

// will result in START_OBJECT
int countDownTokens = 0;
Expand Down
9 changes: 5 additions & 4 deletions src/main/java/org/elasticsearch/index/mapper/MapperService.java
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@
import org.elasticsearch.ElasticsearchGenerationException;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.compress.CompressedString;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.io.Streams;
Expand Down Expand Up @@ -399,10 +400,10 @@ public DocumentMapper documentMapper(String type) {
return mappers.get(type);
}

public DocumentMapper documentMapperWithAutoCreate(String type) {
public Tuple<DocumentMapper, Boolean> documentMapperWithAutoCreate(String type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add quick documentation of what the boolean in the tuple means?

DocumentMapper mapper = mappers.get(type);
if (mapper != null) {
return mapper;
return Tuple.tuple(mapper, Boolean.FALSE);
}
if (!dynamic) {
throw new TypeMissingException(index, type, "trying to auto create mapping, but dynamic mapping is disabled");
Expand All @@ -411,10 +412,10 @@ public DocumentMapper documentMapperWithAutoCreate(String type) {
synchronized (typeMutex) {
mapper = mappers.get(type);
if (mapper != null) {
return mapper;
return Tuple.tuple(mapper, Boolean.FALSE);
}
merge(type, null, true);
return mappers.get(type);
return Tuple.tuple(mappers.get(type), Boolean.TRUE);
}
}

Expand Down
4 changes: 4 additions & 0 deletions src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java
View file
Open in desktop
Expand Up @@ -131,6 +131,10 @@ public boolean mappingsModified() {
return mappingsModified;
}

public void setMappingsModified() {
this.mappingsModified = true;
}

@Override
public String toString() {
StringBuilder sb = new StringBuilder();
Expand Down
21 changes: 14 additions & 7 deletions src/main/java/org/elasticsearch/index/shard/service/InternalIndexShard.java
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.lucene.Lucene;
Expand Down Expand Up @@ -369,9 +370,12 @@ private IndexShardState changeState(IndexShardState newState, String reason) {
@Override
public Engine.Create prepareCreate(SourceToParse source, long version, VersionType versionType, Engine.Operation.Origin origin, boolean canHaveDuplicates, boolean autoGeneratedId) throws ElasticsearchException {
long startTime = System.nanoTime();
DocumentMapper docMapper = mapperService.documentMapperWithAutoCreate(source.type());
ParsedDocument doc = docMapper.parse(source);
return new Engine.Create(docMapper, docMapper.uidMapper().term(doc.uid().stringValue()), doc, version, versionType, origin, startTime, state != IndexShardState.STARTED || canHaveDuplicates, autoGeneratedId);
Tuple<DocumentMapper, Boolean> docMapper = mapperService.documentMapperWithAutoCreate(source.type());
ParsedDocument doc = docMapper.v1().parse(source);
if (docMapper.v2()) {
doc.setMappingsModified();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These upper lines are duplicated in 3 methods, would it make sense to extract the logic to a helper method?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return new Engine.Create(docMapper.v1(), docMapper.v1().uidMapper().term(doc.uid().stringValue()), doc, version, versionType, origin, startTime, state != IndexShardState.STARTED || canHaveDuplicates, autoGeneratedId);
}

@Override
Expand All @@ -390,9 +394,12 @@ public ParsedDocument create(Engine.Create create) throws ElasticsearchException
@Override
public Engine.Index prepareIndex(SourceToParse source, long version, VersionType versionType, Engine.Operation.Origin origin, boolean canHaveDuplicates) throws ElasticsearchException {
long startTime = System.nanoTime();
DocumentMapper docMapper = mapperService.documentMapperWithAutoCreate(source.type());
ParsedDocument doc = docMapper.parse(source);
return new Engine.Index(docMapper, docMapper.uidMapper().term(doc.uid().stringValue()), doc, version, versionType, origin, startTime, state != IndexShardState.STARTED || canHaveDuplicates);
Tuple<DocumentMapper, Boolean> docMapper = mapperService.documentMapperWithAutoCreate(source.type());
ParsedDocument doc = docMapper.v1().parse(source);
if (docMapper.v2()) {
doc.setMappingsModified();
}
return new Engine.Index(docMapper.v1(), docMapper.v1().uidMapper().term(doc.uid().stringValue()), doc, version, versionType, origin, startTime, state != IndexShardState.STARTED || canHaveDuplicates);
}

@Override
Expand All @@ -416,7 +423,7 @@ public ParsedDocument index(Engine.Index index) throws ElasticsearchException {
@Override
public Engine.Delete prepareDelete(String type, String id, long version, VersionType versionType, Engine.Operation.Origin origin) throws ElasticsearchException {
long startTime = System.nanoTime();
DocumentMapper docMapper = mapperService.documentMapperWithAutoCreate(type);
DocumentMapper docMapper = mapperService.documentMapperWithAutoCreate(type).v1();
return new Engine.Delete(type, id, docMapper.uidMapper().term(type, id), version, versionType, origin, startTime, false);
}

Expand Down
14 changes: 9 additions & 5 deletions src/main/java/org/elasticsearch/percolator/PercolatorService.java
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@
import org.elasticsearch.cluster.action.index.MappingUpdatedAction;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.component.AbstractComponent;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
Expand Down Expand Up @@ -276,10 +277,13 @@ private ParsedDocument parseRequest(IndexService documentIndexService, Percolate
}

MapperService mapperService = documentIndexService.mapperService();
DocumentMapper docMapper = mapperService.documentMapperWithAutoCreate(request.documentType());
doc = docMapper.parse(source(parser).type(request.documentType()).flyweight(true));
Tuple<DocumentMapper, Boolean> docMapper = mapperService.documentMapperWithAutoCreate(request.documentType());
doc = docMapper.v1().parse(source(parser).type(request.documentType()).flyweight(true));
if (docMapper.v2()) {
doc.setMappingsModified();
}
if (doc.mappingsModified()) {
mappingUpdatedAction.updateMappingOnMaster(request.index(), docMapper, documentIndexService.indexUUID());
mappingUpdatedAction.updateMappingOnMaster(request.index(), docMapper.v1(), documentIndexService.indexUUID());
}
// the document parsing exists the "doc" object, so we need to set the new current field.
currentFieldName = parser.currentName();
Expand Down Expand Up @@ -386,8 +390,8 @@ private ParsedDocument parseFetchedDoc(PercolateContext context, BytesReference
try {
parser = XContentFactory.xContent(fetchedDoc).createParser(fetchedDoc);
MapperService mapperService = documentIndexService.mapperService();
DocumentMapper docMapper = mapperService.documentMapperWithAutoCreate(type);
doc = docMapper.parse(source(parser).type(type).flyweight(true));
Tuple<DocumentMapper, Boolean> docMapper = mapperService.documentMapperWithAutoCreate(type);
doc = docMapper.v1().parse(source(parser).type(type).flyweight(true));

if (context.highlight() != null) {
doc.setSource(fetchedDoc);
Expand Down
70 changes: 55 additions & 15 deletions src/test/java/org/elasticsearch/index/fielddata/DisabledFieldDataFormatTests.java
View file
Open in desktop
Expand Up @@ -19,31 +19,35 @@

package org.elasticsearch.index.fielddata;

import com.google.common.base.Predicate;
import org.elasticsearch.action.search.SearchPhaseExecutionException;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.common.settings.ImmutableSettings;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.index.mapper.MapperService.SmartNameFieldMappers;
import org.elasticsearch.index.service.IndexService;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.search.aggregations.AggregationBuilders;
import org.elasticsearch.search.aggregations.Aggregator.SubAggCollectionMode;
import org.elasticsearch.test.ElasticsearchIntegrationTest;
import org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope;

import java.util.Set;

import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertFailures;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;

@ClusterScope(randomDynamicTemplates = false)
public class DisabledFieldDataFormatTests extends ElasticsearchIntegrationTest {

@Override
protected int numberOfReplicas() {
return 0;
}

public void test() throws Exception {
createIndex("test");
prepareCreate("test").addMapping("type", "s", "type=string").execute().actionGet();
ensureGreen();
logger.info("indexing data start");
for (int i = 0; i < 10; ++i) {
client().prepareIndex("test", "type", Integer.toString(i)).setSource("s", "value" + i).execute().actionGet();
}
logger.info("indexing data end");

refresh();

Expand Down Expand Up @@ -89,19 +93,55 @@ public void test() throws Exception {
}
}

private void updateFormat(String format) throws Exception {
private void updateFormat(final String format) throws Exception {
logger.info(">> put mapping start {}", format);
client().admin().indices().preparePutMapping("test").setType("type").setSource(
XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("properties")
.startObject("properties")
.startObject("s")
.field("type", "string")
.startObject("fielddata")
.field("format", format)
.endObject()
.endObject()
.endObject()
.field("type", "string")
.startObject("fielddata")
.field("format", format)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you keep the formatting? I tend to find it easier to read when formatted

.endObject()
.endObject()
.endObject()).execute().actionGet();
.endObject()
.endObject()
.endObject()).execute().actionGet();
logger.info(">> put mapping end {}", format);
boolean applied = awaitBusy(new Predicate<Object>() {
@Override
public boolean apply(Object input) {
try {
Set<String> nodes = internalCluster().nodesInclude("test");
if (nodes.isEmpty()) { // we expect at least one node to hold an index, so wait if not allocated yet
return false;
}
for (String node : nodes) {
IndicesService indicesService = internalCluster().getInstance(IndicesService.class, node);
IndexService indexService = indicesService.indexService("test");
if (indexService == null) {
return false;
}
final SmartNameFieldMappers mappers = indexService.mapperService().smartName("s");
if (mappers == null || !mappers.hasMapper()) {
return false;
}
final String currentFormat = mappers.mapper().fieldDataType().getFormat(ImmutableSettings.EMPTY);
if (!format.equals(currentFormat)) {
return false;
}
}
} catch (Exception e) {
logger.info("got exception waiting for concrete mappings", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to fail the test if we get an exception here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think returning false and logging it is good enough

return false;
}
return true;
}
});
logger.info(">> put mapping verified {}, applies {}", format, applied);
if (!applied) {
fail();
}
}

}
2 changes: 1 addition & 1 deletion src/test/java/org/elasticsearch/index/mapper/source/DefaultSourceMappingTests.java
View file
Open in desktop
Expand Up @@ -176,7 +176,7 @@ public void testDefaultMappingAndNoMappingWithMapperService() throws Exception {
MapperService mapperService = MapperTestUtils.newMapperService();
mapperService.merge(MapperService.DEFAULT_MAPPING, new CompressedString(defaultMapping), true);

DocumentMapper mapper = mapperService.documentMapperWithAutoCreate("my_type");
DocumentMapper mapper = mapperService.documentMapperWithAutoCreate("my_type").v1();
assertThat(mapper.type(), equalTo("my_type"));
assertThat(mapper.sourceMapper().enabled(), equalTo(false));
}
Expand Down