Remove some dead code #31993

cbuescher · 2018-07-12T11:00:03Z

Removing some dead code or supressing warnings where apropriate. Most of the
time the variable tested for null is dereferenced earlier or never used before.

elasticmachine · 2018-07-12T11:00:04Z

Pinging @elastic/es-core-infra

cbuescher · 2018-07-12T11:00:30Z

...ats/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParser.java

@@ -145,9 +144,6 @@ private MultiValuesSourceParser(boolean formattable, ValuesSourceType valuesSour
        if (fields != null) {
            factory.fields(fields);
        }
-        if (valueType != null) {


reason: valueType is never changed

cbuescher · 2018-07-12T11:01:13Z

server/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java

@@ -179,37 +178,33 @@ void start() {
                final DiscoveryNode node = nodes[i];
                final String nodeId = node.getId();
                try {
-                    if (node == null) {


reason: node is used before, cannot be null here

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

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?

@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.

+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.

cbuescher · 2018-07-12T11:01:44Z

server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java

@@ -130,10 +130,6 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s
                CircleBuilder.TYPE);
        }

-        if (shapeType == null) {


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

cbuescher · 2018-07-12T11:02:21Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

@@ -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) {


reason: md is used in L1477 already

cbuescher · 2018-07-12T11:03:35Z

...rc/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriter.java

@@ -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


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.

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.

@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

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.

Removing some dead code or supressing warnings where apropriate. Most of the time the variable tested for null is dereferenced earlier or never used before.

cbuescher · 2018-07-26T09:11:14Z

Thanks everybody, would anybody care to do a final review/approval of these changes?

nik9000

LGTM

@throws

* master: Remove reference to non-existent store type (#32418) [TEST] Mute failing FlushIT test Fix ordering of bootstrap checks in docs (#32417) [TEST] Mute failing InternalEngineTests#testSeqNoAndCheckpoints [TEST] Mute failing testConvertLongHexError bump lucene version after backport Upgrade to Lucene-7.5.0-snapshot-608f0277b0 (#32390) [Kerberos] Avoid vagrant update on precommit (#32416) TESTS: Move netty leak detection to paranoid level (#32354) [DOCS] Fixes formatting of scope object in job resource Copy missing segment attributes in getSegmentInfo (#32396) AbstractQueryTestCase should run without type less often (#28936) INGEST: Fix Deprecation Warning in Script Proc. (#32407) Switch x-pack/plugin to new style Requests (#32327) Docs: Correcting a typo in tophits (#32359) Build: Stop double generating buildSrc pom (#32408) TEST: Avoid triggering merges in FlushIT Fix missing JavaDoc for @throws in several places in KerberosTicketValidator. Switch x-pack full restart to new style Requests (#32294) Release requests in cors handler (#32364) Painless: Clean Up PainlessClass Variables (#32380) Docs: Fix callouts in put license HL REST docs (#32363) [ML] Consistent pattern for strict/lenient parser names (#32399) Update update-settings.asciidoc (#31378) Remove some dead code (#31993) Introduce index store plugins (#32375) Rank-Eval: Reduce scope of an unchecked supression Make sure _forcemerge respects `max_num_segments`. (#32291) TESTS: Fix Buf Leaks in HttpReadWriteHandlerTests (#32377) Only enforce password hashing check if FIPS enabled (#32383)

cbuescher added >non-issue review :Core/Infra/Core Core issues without another label v7.0.0 labels Jul 12, 2018

cbuescher commented Jul 12, 2018

View reviewed changes

Remove some dead code

ed73956

Removing some dead code or supressing warnings where apropriate. Most of the time the variable tested for null is dereferenced earlier or never used before.

cbuescher force-pushed the bury-the-dead branch from 0270f11 to ed73956 Compare July 12, 2018 12:46

Christoph Büscher added 4 commits July 17, 2018 14:17

iter§

dced325

Merge branch 'master' into bury-the-dead

6de83e7

Merge branch 'master' into bury-the-dead

3d23098

Merge branch 'master' into bury-the-dead

1cffb66

nik9000 approved these changes Jul 26, 2018

View reviewed changes

cbuescher merged commit 35ae871 into elastic:master Jul 26, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some dead code #31993

Remove some dead code #31993

cbuescher commented Jul 12, 2018

elasticmachine commented Jul 12, 2018

cbuescher Jul 12, 2018

cbuescher Jul 12, 2018

nik9000 Jul 12, 2018

cbuescher Jul 12, 2018

cbuescher Jul 17, 2018

bleskes Jul 26, 2018

cbuescher Jul 12, 2018

cbuescher Jul 12, 2018

cbuescher Jul 12, 2018

rjernst Jul 12, 2018

cbuescher Jul 17, 2018

droberts195 Jul 17, 2018

cbuescher commented Jul 26, 2018

nik9000 left a comment

Remove some dead code #31993

Remove some dead code #31993

Conversation

cbuescher commented Jul 12, 2018

elasticmachine commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jul 26, 2018

nik9000 left a comment

Choose a reason for hiding this comment