Skip to content

Remove Dead Code and Duplication from Metadata #64938

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

Merged
merged 3 commits into from
Dec 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService;

import java.io.IOException;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
Expand Down Expand Up @@ -82,13 +81,8 @@ protected void doMasterOperation(final GetIndexRequest request, String[] concret
switch (feature) {
case MAPPINGS:
if (!doneMappings) {
try {
mappingsResult = state.metadata().findMappings(concreteIndices, indicesService.getFieldFilter());
doneMappings = true;
} catch (IOException e) {
listener.onFailure(e);
return;
}
mappingsResult = state.metadata().findMappings(concreteIndices, indicesService.getFieldFilter());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not also remove the if (!doneXXXX) from the two blocks below in the same go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I can remove those with the strange way these requests work (reading features from an array we could have duplicate entries)?

doneMappings = true;
}
break;
case ALIASES:
Expand Down Expand Up @@ -123,8 +117,6 @@ protected void doMasterOperation(final GetIndexRequest request, String[] concret
throw new IllegalStateException("feature [" + feature + "] is not valid");
}
}
listener.onResponse(
new GetIndexResponse(concreteIndices, mappingsResult, aliasesResult, settings, defaultSettings, dataStreams)
);
listener.onResponse(new GetIndexResponse(concreteIndices, mappingsResult, aliasesResult, settings, defaultSettings, dataStreams));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,12 @@
import org.elasticsearch.action.support.master.info.TransportClusterInfoAction;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.MappingMetadata;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService;

import java.io.IOException;

public class TransportGetMappingsAction extends TransportClusterInfoAction<GetMappingsRequest, GetMappingsResponse> {

private static final Logger logger = LogManager.getLogger(TransportGetMappingsAction.class);
Expand All @@ -55,12 +51,6 @@ public TransportGetMappingsAction(TransportService transportService, ClusterServ
protected void doMasterOperation(final GetMappingsRequest request, String[] concreteIndices, final ClusterState state,
final ActionListener<GetMappingsResponse> listener) {
logger.trace("serving getMapping request based on version {}", state.version());
try {
ImmutableOpenMap<String, MappingMetadata> result =
state.metadata().findMappings(concreteIndices, indicesService.getFieldFilter());
listener.onResponse(new GetMappingsResponse(result));
} catch (IOException e) {
listener.onFailure(e);
}
listener.onResponse(new GetMappingsResponse(state.metadata().findMappings(concreteIndices, indicesService.getFieldFilter())));
}
}
18 changes: 2 additions & 16 deletions server/src/main/java/org/elasticsearch/cluster/ClusterState.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package org.elasticsearch.cluster;

import com.carrotsearch.hppc.cursors.ObjectCursor;
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import org.elasticsearch.Version;
import org.elasticsearch.cluster.block.ClusterBlock;
Expand Down Expand Up @@ -323,7 +322,7 @@ public enum Metric {
ROUTING_NODES("routing_nodes"),
CUSTOMS("customs");

private static Map<String, Metric> valueToEnum;
private static final Map<String, Metric> valueToEnum;

static {
valueToEnum = new HashMap<>();
Expand Down Expand Up @@ -365,7 +364,6 @@ public String toString() {
}

@Override
@SuppressWarnings("unchecked")
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
EnumSet<Metric> metrics = Metric.parseString(params.param("metric", "_all"), true);

Expand Down Expand Up @@ -646,19 +644,7 @@ public void writeTo(StreamOutput out) throws IOException {
routingTable.writeTo(out);
nodes.writeTo(out);
blocks.writeTo(out);
// filter out custom states not supported by the other node
int numberOfCustoms = 0;
for (final ObjectCursor<Custom> cursor : customs.values()) {
if (VersionedNamedWriteable.shouldSerialize(out, cursor.value)) {
numberOfCustoms++;
}
}
out.writeVInt(numberOfCustoms);
for (final ObjectCursor<Custom> cursor : customs.values()) {
if (VersionedNamedWriteable.shouldSerialize(out, cursor.value)) {
out.writeNamedWriteable(cursor.value);
}
}
VersionedNamedWriteable.writeVersionedWritables(out, customs);
if (out.getVersion().before(Version.V_8_0_0)) {
out.writeVInt(-1); // used to be minimumMasterNodesOnPublishingMaster, which was used in 7.x for BWC with 6.x
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,7 @@ private ImmutableOpenMap<String, List<AliasMetadata>> findAliases(final String[]
*
*/
public ImmutableOpenMap<String, MappingMetadata> findMappings(String[] concreteIndices,
Function<String, Predicate<String>> fieldFilter)
throws IOException {
Function<String, Predicate<String>> fieldFilter) {
assert concreteIndices != null;
if (concreteIndices.length == 0) {
return ImmutableOpenMap.of();
Expand Down Expand Up @@ -560,22 +559,7 @@ public String resolveWriteIndexRouting(@Nullable String routing, String aliasOrI
if (writeIndex == null) {
throw new IllegalArgumentException("alias [" + aliasOrIndex + "] does not have a write index");
}
AliasMetadata aliasMd = writeIndex.getAliases().get(result.getName());
if (aliasMd.indexRouting() != null) {
if (aliasMd.indexRouting().indexOf(',') != -1) {
throw new IllegalArgumentException("index/alias [" + aliasOrIndex + "] provided with routing value ["
+ aliasMd.getIndexRouting() + "] that resolved to several routing values, rejecting operation");
}
if (routing != null) {
if (!routing.equals(aliasMd.indexRouting())) {
throw new IllegalArgumentException("Alias [" + aliasOrIndex + "] has index routing associated with it ["
+ aliasMd.indexRouting() + "], and was provided with routing value [" + routing + "], rejecting operation");
}
}
// Alias routing overrides the parent routing (if any).
return aliasMd.indexRouting();
}
return routing;
return resolveRouting(routing, aliasOrIndex, writeIndex.getAliases().get(result.getName()));
}

/**
Expand All @@ -596,7 +580,10 @@ public String resolveIndexRouting(@Nullable String routing, String aliasOrIndex)
if (result.getIndices().size() > 1) {
rejectSingleIndexOperation(aliasOrIndex, result);
}
AliasMetadata aliasMd = alias.getFirstAliasMetadata();
return resolveRouting(routing, aliasOrIndex, alias.getFirstAliasMetadata());
}

private static String resolveRouting(@Nullable String routing, String aliasOrIndex, AliasMetadata aliasMd) {
if (aliasMd.indexRouting() != null) {
if (aliasMd.indexRouting().indexOf(',') != -1) {
throw new IllegalArgumentException("index/alias [" + aliasOrIndex + "] provided with routing value [" +
Expand Down Expand Up @@ -709,10 +696,6 @@ public ImmutableOpenMap<String, Custom> customs() {
return this.customs;
}

public ImmutableOpenMap<String, Custom> getCustoms() {
return this.customs;
}

/**
* The collection of index deletions in the cluster.
*/
Expand Down Expand Up @@ -748,28 +731,6 @@ public int getTotalOpenIndexShards() {
return this.totalOpenIndexShards;
}

/**
* Identifies whether the array containing type names given as argument refers to all types
* The empty or null array identifies all types
*
* @param types the array containing types
* @return true if the provided array maps to all types, false otherwise
*/
public static boolean isAllTypes(String[] types) {
return types == null || types.length == 0 || isExplicitAllType(types);
}

/**
* Identifies whether the array containing type names given as argument explicitly refers to all types
* The empty or null array doesn't explicitly map to all types
*
* @param types the array containing index names
* @return true if the provided array explicitly maps to all types, false otherwise
*/
public static boolean isExplicitAllType(String[] types) {
return types != null && types.length == 1 && ALL.equals(types[0]);
}

/**
* @param concreteIndex The concrete index to check if routing is required
* @return Whether routing is required according to the mapping for the specified index and type
Expand Down Expand Up @@ -973,19 +934,7 @@ public void writeTo(StreamOutput out) throws IOException {
for (ObjectCursor<IndexTemplateMetadata> cursor : templates.values()) {
cursor.value.writeTo(out);
}
// filter out custom states not supported by the other node
int numberOfCustoms = 0;
for (final ObjectCursor<Custom> cursor : customs.values()) {
if (VersionedNamedWriteable.shouldSerialize(out, cursor.value)) {
numberOfCustoms++;
}
}
out.writeVInt(numberOfCustoms);
for (final ObjectCursor<Custom> cursor : customs.values()) {
if (VersionedNamedWriteable.shouldSerialize(out, cursor.value)) {
out.writeNamedWriteable(cursor.value);
}
}
VersionedNamedWriteable.writeVersionedWritables(out, customs);
}

public static Builder builder() {
Expand Down Expand Up @@ -1215,8 +1164,7 @@ public Builder indexGraveyard(final IndexGraveyard indexGraveyard) {
}

public IndexGraveyard indexGraveyard() {
IndexGraveyard graveyard = (IndexGraveyard) getCustom(IndexGraveyard.TYPE);
return graveyard;
return (IndexGraveyard) getCustom(IndexGraveyard.TYPE);
}

public Builder updateSettings(Settings settings, String... indices) {
Expand Down Expand Up @@ -1275,10 +1223,6 @@ public Builder persistentSettings(Settings settings) {
return this;
}

public DiffableStringMap hashesOfConsistentSettings() {
return this.hashesOfConsistentSettings;
}

public Builder hashesOfConsistentSettings(DiffableStringMap hashesOfConsistentSettings) {
this.hashesOfConsistentSettings = hashesOfConsistentSettings;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@

package org.elasticsearch.common.io.stream;

import com.carrotsearch.hppc.cursors.ObjectCursor;
import org.elasticsearch.Version;
import org.elasticsearch.common.collect.ImmutableOpenMap;

import java.io.IOException;

/**
* A {@link NamedWriteable} that has a minimum version associated with it.
Expand Down Expand Up @@ -50,4 +54,27 @@ static <T extends VersionedNamedWriteable> boolean shouldSerialize(final StreamO
return out.getVersion().onOrAfter(custom.getMinimalSupportedVersion());
}

/**
* Writes all those values in the given map to {@code out} that pass the version check in {@link #shouldSerialize} as a list.
*
* @param out stream to write to
* @param customs map of customs
* @param <T> type of customs in map
*/
static <T extends VersionedNamedWriteable> void writeVersionedWritables(StreamOutput out, ImmutableOpenMap<String, T> customs)
throws IOException {
// filter out custom states not supported by the other node
int numberOfCustoms = 0;
for (final ObjectCursor<T> cursor : customs.values()) {
if (shouldSerialize(out, cursor.value)) {
numberOfCustoms++;
}
}
out.writeVInt(numberOfCustoms);
for (final ObjectCursor<T> cursor : customs.values()) {
if (shouldSerialize(out, cursor.value)) {
out.writeNamedWriteable(cursor.value);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public class ElasticsearchMappings {
private ElasticsearchMappings() {
}

static String[] mappingRequiresUpdate(ClusterState state, String[] concreteIndices, Version minVersion) throws IOException {
static String[] mappingRequiresUpdate(ClusterState state, String[] concreteIndices, Version minVersion) {
List<String> indicesToUpdate = new ArrayList<>();

ImmutableOpenMap<String, MappingMetadata> currentMapping = state.metadata().findMappings(concreteIndices,
Expand Down Expand Up @@ -156,14 +156,7 @@ public static void addDocMappingIfMissing(String alias,
String[] concreteIndices = indexAbstraction.getIndices().stream().map(IndexMetadata::getIndex).map(Index::getName)
.toArray(String[]::new);

String[] indicesThatRequireAnUpdate;
try {
indicesThatRequireAnUpdate = mappingRequiresUpdate(state, concreteIndices, Version.CURRENT);
} catch (IOException e) {
listener.onFailure(e);
return;
}

final String[] indicesThatRequireAnUpdate = mappingRequiresUpdate(state, concreteIndices, Version.CURRENT);
if (indicesThatRequireAnUpdate.length > 0) {
try {
String mapping = mappingSupplier.get();
Expand Down