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

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 1 commit
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/MultiValuesSourceParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ private MultiValuesSourceParser(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 MultiValuesSourceParser(boolean formattable, ValuesSourceType valuesSour
if (fields != null) {
factory.fields(fields);
}
if (valueType != 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: valueType is never changed

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 @@ -1478,11 +1478,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
3 changes: 2 additions & 1 deletion ...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 @@ -59,6 +59,7 @@ public FieldConfigWriter(AnalysisConfig config, Set<MlFilter> filters, List<Sche
/**
* Write the Ml autodetect field options to the outputIndex stream.
*/
@SuppressWarnings("unused") // CATEGORIZATION_TOKENIZATION_IN_JAVA seems to be used for performance testing
Copy link
Member Author

Choose a reason for hiding this comment

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

L70-73 shows up as dead, but from the comment on the constant it appears this is used occassionaly, so maybe supressing and leaving a comment is a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of having this constant, could the if condition be commented out with a comment it should be uncommented when needed for debugging? If code must be changed to enable this, then it seems a constant isn't really helping anything here, and adding the unused suppression adds error room for other bugs.

Copy link
Member Author

Choose a reason for hiding this comment

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

@droberts195 can probably comment. As far as I see the CATEGORIZATION_TOKENIZATION_IN_JAVA is used in several places throughout the ML test code, but needs to be enabled manually. I thing supressing the warning and commenting here is sufficient

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's used in 9 places. The alternative would be to get rid of the constant and comment out the code in the 9 places, but then somebody who wanted to run the test would have to remember all 9 places to uncomment. It's nice to just be able to change 1 constant.

I guess the scope of suppression could be minimised by adding a new method writeCategorizationFilters and just applying the suppression of warnings to that new method. writeScheduledEvents is already broken into a separate method that checks a (different) condition and maybe writes one piece of config.

public void write() throws IOException {
StringBuilder contents = new StringBuilder();

Expand Down