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

Remove some dead code #31993

Merged
merged 5 commits into from
Jul 26, 2018
Merged

Remove some dead code #31993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ed73956
Remove some dead code
Jul 12, 2018
dced325
iter§
Jul 17, 2018
6de83e7
Merge branch 'master' into bury-the-dead
Jul 17, 2018
3d23098
Merge branch 'master' into bury-the-dead
Jul 24, 2018
1cffb66
Merge branch 'master' into bury-the-dead
Jul 24, 2018
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
4 changes: 0 additions & 4 deletions .../src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ private ArrayValuesSourceParser(boolean formattable, ValuesSourceType valuesSour
throws IOException {

List<String> fields = null;
ValueType valueType = null;
String format = null;
Map<String, Object> missingMap = null;
Map<ParseField, Object> otherOptions = new HashMap<>();
Expand Down Expand Up @@ -145,9 +144,6 @@ private ArrayValuesSourceParser(boolean formattable, ValuesSourceType valuesSour
if (fields != null) {
factory.fields(fields);
}
if (valueType != null) {
factory.valueType(valueType);
}
if (format != null) {
factory.format(format);
}
Expand Down
57 changes: 26 additions & 31 deletions server/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.FailedNodeException;
import org.elasticsearch.action.NoSuchNodeException;
import org.elasticsearch.action.support.ActionFilters;
import org.elasticsearch.action.support.HandledTransportAction;
import org.elasticsearch.cluster.ClusterState;
Expand Down Expand Up @@ -179,37 +178,33 @@ void start() {
final DiscoveryNode node = nodes[i];
final String nodeId = node.getId();
try {
if (node == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

reason: node is used before, cannot be null here

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if that use is actually a bug....

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. That use is nodeId = node.getId() in the line above. If this use shouldn't be there, how would we get the nodeId for the NoSuchNodeException that is thrown right in this block?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bleskes maybe you are familiar enough with this code to be able to judge whether node can be null here at all? From what I see so far, the nodes array is retrieved earlier in the method through request.concreteNodes(). Following the call hierarchy it is unlikely this can contain null references, but I don't think it is enforced anywhere. Then again, the reference in L179 hasn't led to NPEs since at least mid-2016 when it was added, so maybe its save to assume node cannot be null here.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 remove the node == null check. IMO if we pass a null in there it's bug higher up in the chain which we should learn about and fix where it came from.

onFailure(idx, nodeId, new NoSuchNodeException(nodeId));
} else {
TransportRequest nodeRequest = newNodeRequest(nodeId, request);
if (task != null) {
nodeRequest.setParentTask(clusterService.localNode().getId(), task.getId());
}

transportService.sendRequest(node, transportNodeAction, nodeRequest, builder.build(),
new TransportResponseHandler<NodeResponse>() {
@Override
public NodeResponse newInstance() {
return newNodeResponse();
}

@Override
public void handleResponse(NodeResponse response) {
onOperation(idx, response);
}

@Override
public void handleException(TransportException exp) {
onFailure(idx, node.getId(), exp);
}

@Override
public String executor() {
return ThreadPool.Names.SAME;
}
});
TransportRequest nodeRequest = newNodeRequest(nodeId, request);
if (task != null) {
nodeRequest.setParentTask(clusterService.localNode().getId(), task.getId());
}

transportService.sendRequest(node, transportNodeAction, nodeRequest, builder.build(),
new TransportResponseHandler<NodeResponse>() {
@Override
public NodeResponse newInstance() {
return newNodeResponse();
}

@Override
public void handleResponse(NodeResponse response) {
onOperation(idx, response);
}

@Override
public void handleException(TransportException exp) {
onFailure(idx, node.getId(), exp);
}

@Override
public String executor() {
return ThreadPool.Names.SAME;
}
});
} catch (Exception e) {
onFailure(idx, nodeId, e);
}
Expand Down
6 changes: 1 addition & 5 deletions server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/
package org.elasticsearch.common.geo.parsers;

import org.locationtech.jts.geom.Coordinate;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.geo.GeoPoint;
Expand All @@ -29,6 +28,7 @@
import org.elasticsearch.common.unit.DistanceUnit;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.mapper.GeoShapeFieldMapper;
import org.locationtech.jts.geom.Coordinate;

import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -130,10 +130,6 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s
CircleBuilder.TYPE);
}

if (shapeType == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

reason: shapeType is tested for null earlier, throws exception already there

throw new ElasticsearchParseException("shape type [{}] not included", shapeType);
}

if (shapeType.equals(GeoShapeType.GEOMETRYCOLLECTION)) {
return geometryCollections;
}
Expand Down
7 changes: 2 additions & 5 deletions server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1554,11 +1554,8 @@ public void restore() throws IOException {
filesToRecover.add(fileInfo);
recoveryState.getIndex().addFileDetail(fileInfo.name(), fileInfo.length(), false);
if (logger.isTraceEnabled()) {
if (md == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

reason: md is used in L1477 already

logger.trace("[{}] [{}] recovering [{}] from [{}], does not exists in local store", shardId, snapshotId, fileInfo.physicalName(), fileInfo.name());
} else {
logger.trace("[{}] [{}] recovering [{}] from [{}], exists in local store but is different", shardId, snapshotId, fileInfo.physicalName(), fileInfo.name());
}
logger.trace("[{}] [{}] recovering [{}] from [{}], exists in local store but is different", shardId, snapshotId,
fileInfo.physicalName(), fileInfo.name());
}
}

Expand Down
16 changes: 10 additions & 6 deletions ...main/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.xpack.ml.MachineLearning;
import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent;
import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig;
import org.elasticsearch.xpack.core.ml.job.config.DefaultDetectorDescription;
import org.elasticsearch.xpack.core.ml.job.config.DetectionRule;
import org.elasticsearch.xpack.core.ml.job.config.Detector;
import org.elasticsearch.xpack.core.ml.job.config.MlFilter;
import org.elasticsearch.xpack.core.ml.utils.MlStrings;
import org.elasticsearch.xpack.ml.MachineLearning;

import java.io.IOException;
import java.io.OutputStreamWriter;
Expand Down Expand Up @@ -66,11 +66,7 @@ public void write() throws IOException {
writeFilters(contents);
writeDetectors(contents);
writeScheduledEvents(contents);

if (MachineLearning.CATEGORIZATION_TOKENIZATION_IN_JAVA == false) {
writeAsEnumeratedSettings(CATEGORIZATION_FILTER_PREFIX, config.getCategorizationFilters(),
contents, true);
}
writeCategorizationFilters(contents);

// As values are written as entire settings rather than part of a
// clause no quoting is needed
Expand All @@ -80,6 +76,14 @@ public void write() throws IOException {
writer.write(contents.toString());
}

@SuppressWarnings("unused") // CATEGORIZATION_TOKENIZATION_IN_JAVA is used for performance testing
private void writeCategorizationFilters(StringBuilder contents) {
if (MachineLearning.CATEGORIZATION_TOKENIZATION_IN_JAVA == false) {
writeAsEnumeratedSettings(CATEGORIZATION_FILTER_PREFIX, config.getCategorizationFilters(),
contents, true);
}
}

private void writeDetectors(StringBuilder contents) throws IOException {
int counter = 0;
for (Detector detector : config.getDetectors()) {
Expand Down